fix: wait for ledger offscreen iframe load#26225
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #26225 +/- ##
========================================
Coverage 70.13% 70.13%
========================================
Files 1417 1418 +1
Lines 49444 49446 +2
Branches 13835 13835
========================================
+ Hits 34676 34678 +2
Misses 14768 14768 ☔ View full report in Codecov by Sentry. |
Builds ready [9387929]
Page Load Metrics (177 ± 213 ms)
Bundle size diffs
|
Builds ready [668640b]
Page Load Metrics (79 ± 7 ms)
Bundle size diffs
|
Gudahtt
left a comment
There was a problem hiding this comment.
Mostly looks good, but I noticed the behavior does not seem ideal in the case of a timeout. Adjusting the timeout duration should make things better in that case I think.
Builds ready [a30faa1]
Page Load Metrics (78 ± 8 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| const ledgerInitTimeout = new Promise((_, reject) => { | ||
| setTimeout(() => { | ||
| reject(new Error('Ledger initialization timed out')); | ||
| }, OFFSCREEN_LEDGER_INIT_TIMEOUT); | ||
| }); | ||
| await Promise.race([initLedger(), ledgerInitTimeout]); |
There was a problem hiding this comment.
Elegant, succinct, nice :)
| initializePostMessageStream(); | ||
| initTrezor(); | ||
| initLattice(); |
There was a problem hiding this comment.
Just out of curiosity, would my understanding be correct that the post message stream needs to be synchronously the first task to complete, and then the order doesn't matter between trezor, lattice, and ledger?
Would there be any potential performance, feature, or security gain from initializing the hardware wallets concurrently? Feel free to disregard if this doesn't make any sense, since I don't have much of an understanding of this domain.
There was a problem hiding this comment.
They are effectively executed concurrently in this case, because we're not await-ing anything here. Good question though, in case we did want to await these other steps as well for similar reasons as for Ledger. I think we do want them initializing concurrently to speed up the process.
There was a problem hiding this comment.
Oh that's super cool.😎 I believe these 3 lines would be processed by the call stack instead of the task queue so they would at least always be executed sequentially and in order. I wonder if we could get true parallelism with web workers for this or other similar collections of tasks where speed gains matter. Interesting food for thought!
There was a problem hiding this comment.
I believe they all either spawn an iframe in the offscreen page (i.e. Snaps, Ledger) or setup page event listeners (i.e. Trezor, Lattice): they are all executed sequentially, though we don't wait for the Snap iframe to be fully loaded to move on and init the rest.
The order is not strict, but I thought it made sense to load Ledger as last since it is the only one we explicitly wait for, and this gives a chance to the Snaps iframe to load in the meantime - while if we, for example, were to load Ledger as first, the rest would have to wait for it to either resolve or timeout.
There was a problem hiding this comment.
Good question though, in case we did want to await these other steps as well for similar reasons as for Ledger
Since Trezor and Lattice only require an event listener to be set on chrome.runtime, they are effectively already sync steps that we can't really parallelize (and they should be already fast).
The only component we are initializing out of band is ProxySnapExecutor
There was a problem hiding this comment.
It would be nice to explore the possibility of only setting an event listener for Ledger as well, and spawning the iframe later in time only in case a Ledger Keyring is added to the wallet, and so when the iframe is really needed - because we currently add a Ledger iframe and a LedgerKeyring to the wallet even if the user never interacted with a ledger device for no clear reason
e764bdf
Builds ready [e764bdf]
Page Load Metrics (2224 ± 232 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
Builds ready [cee0470]
Page Load Metrics (1673 ± 54 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| }); | ||
| } | ||
|
|
||
| chrome.runtime.sendMessage({ |
There was a problem hiding this comment.
Hmm, this still makes extension bootup and every other service using the offscreen document dependent on the Ledger iframe. You also would not be able to unlock before 4s has passed or the Ledger iframe loads.
Wondering if we should be emitting a separate event for Ledger and awaiting that in the keyring somewhere not on init as the longer-term solution.
There was a problem hiding this comment.
I think we should evaluate how useful it is to always add a Ledger keyring to the user's keychain on unlock, even if the user never interacted with a ledger device. If we establish that this is not needed, then as you suggested the best thing to do would be to initialize Ledger only when needed (e.g. when the Keyring is explicitly added by a user), so that we don't have to wait for it before unlocking
Description
In order to instantiate a functioning communication between the offscreen iframe for Ledger and the LedgerKeyring (through
LedgerOffscreenBridge), we need to make sure that the iframe is loaded before sending any message to it.We currently wait for the offscreen page to load, but the iframe load is completely async and it will most likely be ready after the rest of the offscreen page, leaving messages proxied to the iframe hanging forever.
On a higher level, this is dangerous because everytime we try to send a message to the Ledger iframe the
KeyringControllercontroller-level mutex is locked, and any other operation will wait for its release to proceed - this creates a deadlock situation in the case the iframe does not respond to a message.This PR makes Ledger iframe initialization into a Promise, and the offscreen page will wait for it to be resolved before sending the "ready" signal back to the extension. To avoid waiting forever, the Ledger initialisation promise races with a 5s timeout: in case of timeout, interactions with Ledger accounts will not work during the entire session (until the offscreen page is re-initialised, and another attempt is made to init Ledger).
Note that the UI setup is not affected by this change, since the Offscreen initialisation is awaited only when unlocking the wallet.
Related issues
Progresses: #26840
Manual testing steps
N/A
Screenshots/Recordings
N/A
Pre-merge author checklist
Pre-merge reviewer checklist