fix: timeout chrome runtime messages for Ledger #31766
Merged
+70
−104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikesposito
commented
Apr 9, 2025
| @@ -9,12 +9,22 @@ import { | |||
| OffscreenCommunicationTarget, | |||
| } from '../../../../shared/constants/offscreen-communication'; | |||
|
|
|||
| const DEFAULT_MESSAGE_TIMEOUT = 4000; | |||
|
|
|||
| const SIGNING_TIMEOUT = 20_000; | |||
There was a problem hiding this comment.
This should be a large value, or the user won't have time to grab the device and accept the signature request
There was a problem hiding this comment.
Though also not too large, or we'd wait forever for the user signature, never releasing KeyringController's mutex.
There was a problem hiding this comment.
Hmm. Tricky problem. I can see it taking more than 20 seconds if someone's closely analyzing the signature.
There was a problem hiding this comment.
Perhaps we should leave the confirmation screen open while the approval on the device is pending? And have no timeout, so users have as much time as they need. But if they want to cancel it, they can close the screen and we can cancel the operation and release the mutex then.
There was a problem hiding this comment.
As there is a risk of UX degradation, I assigned a timeout only for messages that don't require a user action (i.e. attemptMakeApp and updateTransportMethod) in 92cfd72
mikesposito
commented
Apr 10, 2025
| message: IFrameMessage<TAction>, | ||
| timeout = DEFAULT_MESSAGE_TIMEOUT, | ||
| ): Promise<ResponsePayload> { | ||
| if (!this.isDeviceConnected) { |
There was a problem hiding this comment.
Hmm, this.isDeviceConnected seems to always be false
There was a problem hiding this comment.
I'm removing this check for now.
Builds ready [92cfd72]
UI Startup Metrics (1218 ± 64 ms)
|
Builds ready [ecfcd13]
UI Startup Metrics (1209 ± 59 ms)
|
Builds ready [8d59d2f]
UI Startup Metrics (1209 ± 59 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [a439859]
UI Startup Metrics (1217 ± 65 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
davidmurdoch
requested changes
Apr 15, 2025
| let hasResponse = false; | ||
|
|
||
| if (timeout) { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
timer should be cleaned if resolve/reject happens elsewhere (in the chrome.runtime.sendMessage callback). You also won't need the hasResponse variable at all.
There was a problem hiding this comment.
Good point! I changed the way this it's done in e402758
| resolve(response.payload); | ||
| } else { | ||
| reject(response.payload.error); | ||
| reject(response?.payload?.error || 'Unknown Ledger error'); |
There was a problem hiding this comment.
reject value is sometimes an Error and sometimes a string. It should always be an Error.
There was a problem hiding this comment.
as response is any, we don't know what response?.payload?.error really is. I wrapped it in an Error instance in 1508807 so we are sure that we always reject with an Error
davidmurdoch
approved these changes
Apr 22, 2025
|
^ Well, other than the CI failures 😞 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the following deadlock issue when the Ledger iframe endpoint is unavailable (timeout or error) while using a chrome MV3: #31436
Iframe messages that require user actions to be resolved (e.g., signing) are intentionally left with no timeout for now.
The equivalent issue for MV2/firefox has been fixed separately
Related issues
Fixes: #31436
Manual testing steps
To reproduce the issue
/etc/hosts) to force the iframe to fail:127.0.0.1 metamask.github.iomainbranch)yarn start, to run an MV3 buildTo test the fix
/etc/hosts) to force the iframe to fail:127.0.0.1 metamask.github.ioyarn startand reload the extensionScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist