[Bug]: Ledger offscreen communication can cause deadlocks with KeyringController >^16 #26840
Assignees
Labels
Comments
7 tasks
mikesposito
added a commit
that referenced
this issue
Sep 2, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **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 `KeyringController` controller-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. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/26225?quickstart=1) ## **Related issues** Progresses: #26840 ## **Manual testing steps** N/A ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> N/A ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
gauthierpetetin
added
the
Sev2-normal
|
#26225 fixes this issue for Chrome |
7 tasks
7 tasks
7 tasks
|
This is currently blocked by this issue: https://github.com/MetaMask/accounts-planning/issues/615 |
jpuri
pushed a commit
that referenced
this issue
Nov 13, 2024
…27688) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR bumps the `@metamask/eth-ledger-bridge-keyring` dependency to `^5.0.1`. [](https://codespaces.new/MetaMask/metamask-extension/pull/27688?quickstart=1) ## **Related issues** Unblocks: #26840 ## **Manual testing steps** This changes directly impacts Ledger devices: 1. Add one or more ledger accounts 2. Sign message 3. Sign typed data 4. Sign transaction 5. Remove Ledger accounts ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** **Add account and sign** https://github.com/user-attachments/assets/d685a416-6931-4aae-b207-e263ac235064 **Forget device** https://github.com/user-attachments/assets/0e11216a-04e1-4166-af4e-e7847d1b2809 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
](https://codespaces.new/MetaMask/metamask-extension/pull/26225?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/27688?quickstart=1){kind=link}
|
A combination of PRs and releases have addressed this specific issue related to Ledger close by #27688 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
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 every time we try to send a message to the Ledger iframe the KeyringController controller-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.
Expected behavior
Setting the Ledger transport should never result in a
KeyringControllerdeadlock, to the point that we can reapply #25435 safelyScreenshots/Recordings
No response
Steps to reproduce
The issue becomes apparent when using the latest
withKeyringmethod from KeyringController - and for this reason the linked PR was reverted.By re-applying those changes, the issue can be seen when the unlock action happens fast enough to be done before the offscreen document is fully loaded. This can sometimes happen when running e2e tests locally:
yarn start:testyarn user-actions-benchmark:chromeError messages or log output
No response
Detection stage
On the development branch
Version
The PR that made the issue evident was reverted, but the goal is to be able to reapply it to
develop(#25435)Build type
None
Browser
Chrome, Firefox
Operating system
Windows, MacOS, Linux
Hardware wallet
Ledger
Additional context
No response
Severity
No response
The text was updated successfully, but these errors were encountered: