From f2166179805c6130631949f63db241a5848b9236 Mon Sep 17 00:00:00 2001 From: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:51:39 -0400 Subject: [PATCH 1/2] feat: reuse ledger transport for multiple operations cp-13.43.0 (#45158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Ledger integration is presenting an odd UX where the operation "Account Import" never settles, the possible root cause for this issue is a failure during a device roundtrip operation using WebHID, since this operation runs under a keyring mutex with no timeout we never see the promise resolve/reject and therefore the operation never finishes. Three changes are introduced to mitigate this issue: - Revert of https://github.com/MetaMask/metamask-extension/pull/45048. A previous temporal fix. - Add timeouts to `LedgerOffscreenBridge` as a safety net - `getPublicKey` now has a 30s timeout — no user confirmation needed - `deviceSignTransaction`, `deviceSignMessage`, `deviceSignTypedData` now has a 5 min timeout - needs on device confirmation - Timeout rejection message updated to: `Ledger device did not respond to "" within ms` - The timeout are arbitrary - Reuse transport across actions in `LedgerLegacyHandler` - `handleAction` no longer calls await `this.closeTransport()` in its finally. Instead it cancels any pending idle close at entry and schedules a new one after the action — so consecutive actions reuse the open WebHID transport instead of open/close churning (the churn that probably desyncs WebHID). - Added `scheduleIdleClose()`/`clearIdleClose()` helpers. - `closeTransport()` now calls `clearIdleClose()` (a manual disconnect/close supersedes a pending idle close). - `destroy()` clears the idle timer before tearing down listeners/transport. - The HID disconnect listener still calls `closeTransport()` immediately on unplug. CHANGELOG entry: reuse ledger transport for multiple operations Fixes: https://github.com/MetaMask/metamask-extension/issues/45027 Fixes: https://consensyssoftware.atlassian.net/browse/MUL-2108 1. Connect a Ledger device and import multiple accounts (at least 7). 2. Forget device and disconnect 3. Connect the device again and try to import multiple accounts (more accounts than imported in step 1). 1. Using the imported Ledger accounts try to execute different operations like message sign, swap, bridge, and send https://github.com/user-attachments/assets/7a52c1cb-ea98-4118-97ad-468908261b92 - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] 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. --- > [!NOTE] > **Medium Risk** > Changes core Ledger offscreen transport lifecycle, mutex/serialization, and hardware account unlock paths; mistakes could cause device lock errors, premature timeouts on signing, or stuck transports. > > **Overview** > Addresses stuck Ledger flows (e.g. account import) by **keeping the WebHID transport open** across bursts of offscreen actions (5s idle close) instead of closing after every call, with **`forceReset`** to drop a hung transport when recovery is needed. > > **`LedgerOffscreenBridge`** adds action-specific timeouts (30s for `getPublicKey`, 5min for signing) and clearer timeout errors; **`unlockHardwareWalletAccount`** drops its separate account-creation timeout in favor of that stack. > > **`ledger-router`** serializes concurrent offscreen messages on one promise chain, races each action against 60s / 330s backstops (sign vs read), calls **`forceReset`** on timeout, and swallows late rejections so the chain can continue. DMK stub forwards **`forceReset`** to legacy. Tests cover concurrency, timeout recovery, bootstrap error swallowing, and init/switch races. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 70e07e579904746e4532a5078470110adbe6e082. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot). --------- Co-authored-by: Cursor --- .../hardware-wallets/ledger-dmk.test.ts | 22 ++ app/offscreen/hardware-wallets/ledger-dmk.ts | 5 + .../hardware-wallets/ledger-router.test.ts | 278 +++++++++++++++++- .../hardware-wallets/ledger-router.ts | 107 ++++++- app/offscreen/hardware-wallets/ledger.test.ts | 240 ++++++++++++++- app/offscreen/hardware-wallets/ledger.ts | 85 +++++- .../ledger-offscreen-bridge.test.ts | 67 ++++- .../ledger-offscreen-bridge.ts | 78 +++-- app/scripts/metamask-controller.js | 43 +-- app/scripts/metamask-controller.test.js | 64 ---- 10 files changed, 824 insertions(+), 165 deletions(-) diff --git a/app/offscreen/hardware-wallets/ledger-dmk.test.ts b/app/offscreen/hardware-wallets/ledger-dmk.test.ts index ecafded316a3..b83ad081a46d 100644 --- a/app/offscreen/hardware-wallets/ledger-dmk.test.ts +++ b/app/offscreen/hardware-wallets/ledger-dmk.test.ts @@ -4,6 +4,7 @@ import { LedgerDmkBridgeHandler } from './ledger-dmk'; const mockLegacyInit = jest.fn(); const mockLegacyDestroy = jest.fn(); const mockLegacyHandleAction = jest.fn(); +const mockLegacyForceReset = jest.fn(); jest.mock('./ledger', () => ({ // eslint-disable-next-line @typescript-eslint/naming-convention @@ -12,6 +13,7 @@ jest.mock('./ledger', () => ({ init: mockLegacyInit, destroy: mockLegacyDestroy, handleAction: mockLegacyHandleAction, + forceReset: mockLegacyForceReset, })), })); @@ -85,4 +87,24 @@ describe('LedgerDmkBridgeHandler', () => { expect(result).toEqual({ ok: true }); }); }); + + describe('forceReset', () => { + it('forwards the reset to the underlying legacy handler when initialised', async () => { + const handler = new LedgerDmkBridgeHandler(); + await handler.init(); + + handler.forceReset(); + + expect(mockLegacyForceReset).toHaveBeenCalledTimes(1); + }); + + it('is a safe no-op when called before init', () => { + const handler = new LedgerDmkBridgeHandler(); + + // The optional-chaining guard means a pre-init reset does not throw and + // does not touch the (non-existent) legacy handler. + expect(() => handler.forceReset()).not.toThrow(); + expect(mockLegacyForceReset).not.toHaveBeenCalled(); + }); + }); }); diff --git a/app/offscreen/hardware-wallets/ledger-dmk.ts b/app/offscreen/hardware-wallets/ledger-dmk.ts index 787368edb4af..371cd27f9a64 100644 --- a/app/offscreen/hardware-wallets/ledger-dmk.ts +++ b/app/offscreen/hardware-wallets/ledger-dmk.ts @@ -47,4 +47,9 @@ export class LedgerDmkBridgeHandler { return this.legacyHandler.handleAction(action, params); } + + /** Forwards a forced transport reset to the underlying legacy handler. */ + forceReset(): void { + this.legacyHandler?.forceReset(); + } } diff --git a/app/offscreen/hardware-wallets/ledger-router.test.ts b/app/offscreen/hardware-wallets/ledger-router.test.ts index e05dfd150035..f68d5b734a49 100644 --- a/app/offscreen/hardware-wallets/ledger-router.test.ts +++ b/app/offscreen/hardware-wallets/ledger-router.test.ts @@ -7,15 +7,18 @@ import { const mockDmkInit = jest.fn(); const mockDmkDestroy = jest.fn(); const mockDmkHandleAction = jest.fn(); +const mockDmkForceReset = jest.fn(); const mockLegacyInit = jest.fn(); const mockLegacyDestroy = jest.fn(); const mockLegacyHandleAction = jest.fn(); +const mockLegacyForceReset = jest.fn(); type MockHandler = { init: jest.Mock; destroy: jest.Mock; handleAction: jest.Mock; + forceReset: jest.Mock; }; let mockDmkInstance: MockHandler; @@ -28,6 +31,7 @@ jest.mock('./ledger-dmk', () => { init: mockDmkInit, destroy: mockDmkDestroy, handleAction: mockDmkHandleAction, + forceReset: mockDmkForceReset, }; return mockDmkInstance; }), @@ -50,6 +54,7 @@ jest.mock('./ledger', () => ({ init: mockLegacyInit, destroy: mockLegacyDestroy, handleAction: mockLegacyHandleAction, + forceReset: mockLegacyForceReset, }; return mockLegacyInstance; }), @@ -65,6 +70,8 @@ type LegacyModule = typeof import('./ledger'); let initLedger: RouterModule['default']; let switchLedgerHandler: RouterModule['switchLedgerHandler']; let bootstrapLedger: RouterModule['bootstrapLedger']; +let READ_ACTION_TIMEOUT_MS: RouterModule['READ_ACTION_TIMEOUT_MS']; +let SIGN_ACTION_TIMEOUT_MS: RouterModule['SIGN_ACTION_TIMEOUT_MS']; let mockedDmkCtor: jest.Mock; let mockedLegacyCtor: jest.Mock; @@ -136,6 +143,8 @@ describe('LedgerRouter', () => { initLedger = router.default; switchLedgerHandler = router.switchLedgerHandler; bootstrapLedger = router.bootstrapLedger; + READ_ACTION_TIMEOUT_MS = router.READ_ACTION_TIMEOUT_MS; + SIGN_ACTION_TIMEOUT_MS = router.SIGN_ACTION_TIMEOUT_MS; // eslint-disable-next-line @typescript-eslint/no-require-imports const dmkModule = require('./ledger-dmk') as DmkModule; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -219,12 +228,12 @@ describe('LedgerRouter', () => { ); expect(result).toBe(true); + + await flushAsync(); expect(mockDmkHandleAction).toHaveBeenCalledWith( LedgerAction.getPublicKey, { hdPath: "m/44'/60'/0'/0/0" }, ); - - await flushAsync(); expect(sendResponse).toHaveBeenCalledWith({ success: true, payload: 'dmk-result', @@ -239,12 +248,11 @@ describe('LedgerRouter', () => { getListener()(makeMessage(LedgerAction.getPublicKey), {}, sendResponse); + await flushAsync(); expect(mockLegacyHandleAction).toHaveBeenCalledWith( LedgerAction.getPublicKey, undefined, ); - - await flushAsync(); expect(sendResponse).toHaveBeenCalledWith({ success: true, payload: 'legacy-result', @@ -279,6 +287,219 @@ describe('LedgerRouter', () => { payload: { error: expect.objectContaining({ message: 'bad' }) }, }); }); + + it('serializes concurrent actions so the second runs only after the first resolves', async () => { + await initLedger(LedgerHandlerMode.Legacy); + let resolveFirst!: (value: unknown) => void; + const firstPending = new Promise((r) => { + resolveFirst = r; + }); + mockLegacyHandleAction.mockReturnValueOnce(firstPending); + mockLegacyHandleAction.mockResolvedValueOnce('second-result'); + + const sendResponse1 = jest.fn(); + const sendResponse2 = jest.fn(); + + getListener()( + makeMessage(LedgerAction.getPublicKey, { hdPath: 'a' }), + {}, + sendResponse1, + ); + getListener()( + makeMessage(LedgerAction.getPublicKey, { hdPath: 'b' }), + {}, + sendResponse2, + ); + + await flushAsync(); + // First action is in flight; the second must not have started yet, and + // neither response has been sent. + expect(mockLegacyHandleAction).toHaveBeenCalledTimes(1); + expect(sendResponse1).not.toHaveBeenCalled(); + expect(sendResponse2).not.toHaveBeenCalled(); + + resolveFirst('first-result'); + await flushAsync(); + + expect(mockLegacyHandleAction).toHaveBeenCalledTimes(2); + expect(sendResponse1).toHaveBeenCalledWith({ + success: true, + payload: 'first-result', + }); + expect(sendResponse2).toHaveBeenCalledWith({ + success: true, + payload: 'second-result', + }); + }); + + it('rejects a wedged action after the timeout, force-resets the handler, and frees the chain', async () => { + const sendResponse1 = jest.fn(); + const sendResponse2 = jest.fn(); + jest.useFakeTimers(); + try { + await initLedger(LedgerHandlerMode.Legacy); + + // First action never resolves (wedged offscreen WebHID round-trip); + // second action resolves normally once it gets to run. + mockLegacyHandleAction + .mockReturnValueOnce( + new Promise(() => { + /* never resolves */ + }), + ) + .mockResolvedValueOnce('after-recovery'); + + getListener()( + makeMessage(LedgerAction.getPublicKey, { hdPath: 'a' }), + {}, + sendResponse1, + ); + getListener()( + makeMessage(LedgerAction.getPublicKey, { hdPath: 'b' }), + {}, + sendResponse2, + ); + + // Let the chain reach the wedged action. + await Promise.resolve(); + expect(sendResponse1).not.toHaveBeenCalled(); + + // Cross the read-action backstop: the link rejects and the + // handler is force-reset (synchronously, within the timer callback). + jest.advanceTimersByTime(READ_ACTION_TIMEOUT_MS); + expect(mockLegacyForceReset).toHaveBeenCalledTimes(1); + } finally { + // Always restore real timers so later tests don't hang on faked setTimeout. + jest.useRealTimers(); + } + + // Drain the microtask chain now that timers are real. + await flushAsync(); + expect(sendResponse1).toHaveBeenCalledWith({ + success: false, + payload: { + error: expect.objectContaining({ + message: expect.stringContaining('timed out'), + }), + }, + }); + expect(sendResponse2).toHaveBeenCalledWith({ + success: true, + payload: 'after-recovery', + }); + }); + + it('swallows the late rejection of a timed-out action (no unhandled rejection)', async () => { + // After the timeout fires and forceReset closes the transport, the + // abandoned handleAction promise typically rejects. That rejection must + // be consumed (not surface as unhandled) and must not change the + // already-sent timeout response. + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { + unhandled.push(reason); + }; + process.on('unhandledRejection', onUnhandled); + + let rejectAction!: (error: unknown) => void; + const sendResponse = jest.fn(); + jest.useFakeTimers(); + try { + await initLedger(LedgerHandlerMode.Legacy); + mockLegacyHandleAction.mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectAction = reject; + }), + ); + + getListener()( + makeMessage(LedgerAction.getPublicKey, { hdPath: 'a' }), + {}, + sendResponse, + ); + + // Let the chain reach the wedged action, then cross the read-action + // backstop. forceReset runs synchronously inside the timer. + await Promise.resolve(); + jest.advanceTimersByTime(READ_ACTION_TIMEOUT_MS); + expect(mockLegacyForceReset).toHaveBeenCalledTimes(1); + } finally { + jest.useRealTimers(); + } + + // Drain the microtask chain: the timeout rejection reaches sendResponse. + await flushAsync(); + expect(sendResponse).toHaveBeenCalledWith({ + success: false, + payload: { + error: expect.objectContaining({ + message: expect.stringContaining('timed out'), + }), + }, + }); + + // The transport now closes (forceReset ran); the wedged action rejects + // late. This rejection must be consumed, not surface as unhandled. + rejectAction(new Error('transport closed')); + await flushAsync(); + process.off('unhandledRejection', onUnhandled); + + expect(unhandled).toHaveLength(0); + // The timeout response stands; the late rejection did not overwrite it. + expect(sendResponse).toHaveBeenCalledTimes(1); + }); + + it('uses the longer sign-action backstop for signing actions (330s, not 60s)', async () => { + // Signing actions (signTransaction/signPersonalMessage/signTypedData) + // require user confirmation on the device and use a longer backstop than + // read actions. A wedged sign action must NOT time out at the 60s read + // backstop — only at the 330s sign backstop. + jest.useFakeTimers(); + const sendResponse = jest.fn(); + try { + await initLedger(LedgerHandlerMode.Legacy); + mockLegacyHandleAction.mockReturnValueOnce( + new Promise(() => { + /* never resolves */ + }), + ); + + getListener()( + makeMessage(LedgerAction.signTransaction, { + hdPath: 'a', + tx: '0x0', + }), + {}, + sendResponse, + ); + + // Let the chain reach the wedged action. + await Promise.resolve(); + + // Cross the read-action backstop: a signing action is still pending. + jest.advanceTimersByTime(READ_ACTION_TIMEOUT_MS); + expect(mockLegacyForceReset).not.toHaveBeenCalled(); + expect(sendResponse).not.toHaveBeenCalled(); + + // Cross the remaining sign-action backstop: now it times + // out and force-resets the handler. + jest.advanceTimersByTime( + SIGN_ACTION_TIMEOUT_MS - READ_ACTION_TIMEOUT_MS, + ); + expect(mockLegacyForceReset).toHaveBeenCalledTimes(1); + } finally { + jest.useRealTimers(); + } + + await flushAsync(); + expect(sendResponse).toHaveBeenCalledWith({ + success: false, + payload: { + error: expect.objectContaining({ + message: expect.stringContaining('timed out'), + }), + }, + }); + }); }); describe('switchLedgerHandler', () => { @@ -361,6 +582,37 @@ describe('LedgerRouter', () => { payload: 'legacy-result', }); }); + + it('awaits an in-flight initLedger before switching, avoiding a duplicate handler', async () => { + // Start a Legacy init and hold it in flight so a switch arriving + // mid-init must wait for `initInProgress` to settle before creating the + // new (DMK) handler. Without the guard, the switch would see + // `activeHandler === null` and boot a second Legacy handler. + let resolveLegacyInit!: () => void; + mockLegacyInit.mockReturnValueOnce( + new Promise((resolve) => { + resolveLegacyInit = resolve; + }), + ); + + const initPromise = initLedger(LedgerHandlerMode.Legacy); + // initLedger assigns `initInProgress` synchronously before suspending. + const switchPromise = switchLedgerHandler(LedgerHandlerMode.DMK); + + // The switch is parked on the in-flight init; no DMK handler is created yet. + await Promise.resolve(); + expect(mockedDmkCtor).not.toHaveBeenCalled(); + + // Let the initial init complete; the switch can now proceed. + resolveLegacyInit(); + await Promise.all([initPromise, switchPromise]); + + // Exactly one Legacy handler (from initLedger) and one DMK handler (from + // the switch); the Legacy handler was destroyed after the atomic swap. + expect(mockedLegacyCtor).toHaveBeenCalledTimes(1); + expect(mockedDmkCtor).toHaveBeenCalledTimes(1); + expect(mockLegacyDestroy).toHaveBeenCalledTimes(1); + }); }); describe('bootstrapLedger', () => { @@ -371,6 +623,24 @@ describe('LedgerRouter', () => { expect(mockLegacyInit).toHaveBeenCalledWith(); expect(mockedDmkCtor).not.toHaveBeenCalled(); }); + + it('swallows init failure and logs instead of throwing', async () => { + // A real device failure during bootstrap must not reject the bootstrap + // promise (the offscreen document would otherwise be left in a broken + // state); it is logged so the failure is observable from DevTools. + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + mockLegacyInit.mockRejectedValueOnce(new Error('init boom')); + + await expect(bootstrapLedger()).resolves.toBeUndefined(); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + '[ledger-router] bootstrapLedger failed:', + expect.objectContaining({ message: 'init boom' }), + ); + consoleErrorSpy.mockRestore(); + }); }); describe('initLedger handler lifecycle', () => { diff --git a/app/offscreen/hardware-wallets/ledger-router.ts b/app/offscreen/hardware-wallets/ledger-router.ts index cb00bc84eb46..41d630a5a6cf 100644 --- a/app/offscreen/hardware-wallets/ledger-router.ts +++ b/app/offscreen/hardware-wallets/ledger-router.ts @@ -15,6 +15,12 @@ type LedgerHandler = { action: LedgerAction, params?: Record, ): Promise; + /** + * Best-effort synchronous reset of the underlying transport, invoked when an + * action has wedged past its timeout. Drops transport/app references so the + * next action opens a fresh transport instead of queuing behind the hung one. + */ + forceReset?: () => void; }; /** The currently-active ledger handler (DMK bridge or legacy). */ @@ -30,6 +36,73 @@ type ChromeMessageListener = Parameters< /** Reference to the router's own chrome.runtime.onMessage listener. */ let messageListener: ChromeMessageListener | null = null; +/** + * Serializes all Ledger actions through a single promise chain so concurrent + * messages never overlap on the shared transport (which would reject with + * `TransportLocked`). Each link races `handleAction` against a timeout so a + * wedged offscreen WebHID round-trip rejects (unblocking later messages) + * instead of stalling the chain forever; on timeout the handler is + * force-reset so retries can open a fresh transport. + */ +let actionChain: Promise = Promise.resolve(); + +/** Backstop timeout for non-signing actions (above the 30s bridge read timeout). */ +export const READ_ACTION_TIMEOUT_MS = 60_000; +/** Backstop timeout for signing actions (above the 300s bridge sign timeout). */ +export const SIGN_ACTION_TIMEOUT_MS = 330_000; +const SIGN_ACTIONS = new Set([ + LedgerAction.signTransaction, + LedgerAction.signPersonalMessage, + LedgerAction.signTypedData, +]); + +function actionTimeoutMs(action: LedgerAction): number { + return SIGN_ACTIONS.has(action) + ? SIGN_ACTION_TIMEOUT_MS + : READ_ACTION_TIMEOUT_MS; +} + +/** + * Race a Ledger action against a timeout without dropping the losing promise. + * + * Unlike `Promise.race`, this attaches a handler to the action promise so its + * eventual settlement is always consumed. When the timeout wins, the in-flight + * `handleAction` typically rejects a moment later once `forceReset` closes the + * transport; without the attached handler that late rejection would surface as + * unhandled. Mirrors `withTrezorDeviceTimeout`. On timeout the handler is + * force-reset so the next action opens a fresh transport instead of queuing + * behind the hung one. + * @param handler + * @param action + * @param params + */ +function raceActionWithTimeout( + handler: LedgerHandler, + action: LedgerAction, + params: Record | undefined, +): Promise { + return new Promise((resolve, reject) => { + const timeoutMs = actionTimeoutMs(action); + const timer = setTimeout(() => { + handler.forceReset?.(); + reject( + new Error(`Ledger action "${action}" timed out after ${timeoutMs}ms`), + ); + }, timeoutMs); + + handler.handleAction(action, params).then( + (result) => { + clearTimeout(timer); + resolve(result); + }, + (error: unknown) => { + clearTimeout(timer); + reject(error); + }, + ); + }); +} + /** * Tracks the in-flight `initLedger` call. When `switchLedgerHandler` is * invoked while `initLedger` has not yet finished, it awaits this promise @@ -80,22 +153,24 @@ function ensureMessageListener(): void { ? (message.params as Record) : undefined; - activeHandler - .handleAction(action, params) - .then((result) => { - sendResponse({ - success: true, - payload: result, - }); - }) - .catch((error: unknown) => { - sendResponse({ - success: false, - payload: { - error: serializeLedgerError(error), - }, - }); - }); + // Chain onto the in-flight action so concurrent messages never overlap on + // the shared transport. Race the offscreen link against a timeout so a + // wedged action rejects (freeing the chain) instead of stalling it; on + // timeout, force-reset the handler so retries open a fresh transport. + const handler = activeHandler; + actionChain = actionChain + .then(() => raceActionWithTimeout(handler, action, params)) + .then( + (result) => { + sendResponse({ success: true, payload: result }); + }, + (error: unknown) => { + sendResponse({ + success: false, + payload: { error: serializeLedgerError(error) }, + }); + }, + ); return true; }; diff --git a/app/offscreen/hardware-wallets/ledger.test.ts b/app/offscreen/hardware-wallets/ledger.test.ts index 9992c31ada1b..0782b0024a6f 100644 --- a/app/offscreen/hardware-wallets/ledger.test.ts +++ b/app/offscreen/hardware-wallets/ledger.test.ts @@ -194,6 +194,13 @@ describe('Ledger Offscreen', () => { mockCreate.mockResolvedValue(mockTransport); }); + afterEach(() => { + // handleAction now schedules a deferred idle close instead of closing + // immediately; clear any leaked timers so they don't fire in later tests. + jest.clearAllTimers(); + jest.useRealTimers(); + }); + describe('init', () => { it('sets up device listeners', async () => { const handler = new LedgerLegacyHandler(); @@ -401,6 +408,89 @@ describe('Ledger Offscreen', () => { }); consoleSpy.mockRestore(); }); + + it('reuses an already-connected transport from openConnected without create()', async () => { + mockOpenConnected.mockResolvedValue(mockTransport); + + await handler.handleAction(LedgerAction.makeApp); + + expect(mockOpenConnected).toHaveBeenCalled(); + expect(mockCreate).not.toHaveBeenCalled(); + }); + + it('reuses the open transport when getAppConfiguration still responds', async () => { + mockGetAppConfiguration.mockResolvedValue({ version: '1.0.0' }); + + await handler.handleAction(LedgerAction.makeApp); + expect(mockCreate).toHaveBeenCalledTimes(1); + mockCreate.mockClear(); + mockGetAppConfiguration.mockClear(); + + // Second makeApp finds an existing responsive app and skips reopening. + await handler.handleAction(LedgerAction.makeApp); + + expect(mockGetAppConfiguration).toHaveBeenCalled(); + expect(mockCreate).not.toHaveBeenCalled(); + }); + + it('reconnects when the existing app is no longer responsive', async () => { + mockGetAppConfiguration.mockResolvedValue({ version: '1.0.0' }); + await handler.handleAction(LedgerAction.makeApp); + expect(mockCreate).toHaveBeenCalledTimes(1); + mockCreate.mockClear(); + mockTransportClose.mockClear(); + + // The device stopped responding: makeApp must close and reopen. + mockGetAppConfiguration.mockRejectedValueOnce( + new Error('disconnected'), + ); + await handler.handleAction(LedgerAction.makeApp); + + expect(mockTransportClose).toHaveBeenCalled(); + expect(mockCreate).toHaveBeenCalledTimes(1); + }); + + it('deduplicates concurrent makeApp calls (opens only one transport)', async () => { + mockOpenConnected.mockResolvedValue(null); + let resolveCreate!: () => void; + mockCreate.mockReturnValue( + new Promise((resolve) => { + resolveCreate = () => resolve(mockTransport); + }), + ); + + const a = handler.handleAction(LedgerAction.makeApp); + const b = handler.handleAction(LedgerAction.makeApp); + + // Both calls share the single pendingMakeApp; only one create() in flight. + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(mockCreate).toHaveBeenCalledTimes(1); + + resolveCreate(); + await Promise.all([a, b]); + + expect(mockCreate).toHaveBeenCalledTimes(1); + }); + }); + + describe('getAppNameAndVersion', () => { + it('parses the app name and version from the raw transport response', async () => { + const name = 'Ethereum'; + const version = '1.2.3'; + const response = Buffer.concat([ + Buffer.from([0x00, name.length]), + Buffer.from(name, 'ascii'), + Buffer.from([version.length]), + Buffer.from(version, 'ascii'), + ]); + mockTransportSend.mockResolvedValue(response); + + const responseObj = await sendAction(LedgerAction.getAppNameAndVersion); + + expect(responseObj.success).toBe(true); + expect(responseObj.payload).toEqual({ appName: name, version }); + expect(mockTransportSend).toHaveBeenCalledWith(0xb0, 0x01, 0x00, 0x00); + }); }); describe('getAppConfiguration', () => { @@ -479,6 +569,44 @@ describe('Ledger Offscreen', () => { expect(response.payload).toEqual(defaultSignature); }); + it('returns an error when hdPath is missing', async () => { + const consoleSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + + const response = await sendAction(LedgerAction.signTransaction, { + tx: '0x0', + }); + + expect(response.success).toBe(false); + expect(response.payload).toEqual({ + error: expect.objectContaining({ + message: 'Missing hdPath or tx parameter', + }), + }); + expect(mockClearSignTransaction).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + it('returns an error when tx is missing', async () => { + const consoleSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + + const response = await sendAction(LedgerAction.signTransaction, { + hdPath: "m/44'/60'/0'/0/0", + }); + + expect(response.success).toBe(false); + expect(response.payload).toEqual({ + error: expect.objectContaining({ + message: 'Missing hdPath or tx parameter', + }), + }); + expect(mockClearSignTransaction).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + it('calls clearSignTransaction with "erc20: true" for ERC20 approve selector', async () => { await sendAction(LedgerAction.signTransaction, { hdPath: "m/44'/60'/0'/0/0", @@ -803,37 +931,133 @@ describe('Ledger Offscreen', () => { }); describe('transport cleanup', () => { - const flushPromises = () => - new Promise((resolve) => setTimeout(resolve, 0)); - - it('closes transport after a successful action', async () => { + it('keeps the transport open across consecutive actions and closes it after the idle timeout', async () => { mockGetAddress.mockResolvedValue({ publicKey: '04abcd1234', address: '0x1234567890abcdef', chainCode: 'chaincode123', }); + jest.useFakeTimers(); + await sendAction(LedgerAction.getPublicKey, { hdPath: "m/44'/60'/0'/0/0", }); - await flushPromises(); + // Transport opened once, not closed yet (idle timer pending). + expect(mockCreate).toHaveBeenCalledTimes(1); + expect(mockTransportClose).not.toHaveBeenCalled(); - expect(mockTransportClose).toHaveBeenCalled(); + // Second action within the idle window reuses the open transport. + await sendAction(LedgerAction.getPublicKey, { + hdPath: "m/44'/60'/1'/0/0", + }); + expect(mockCreate).toHaveBeenCalledTimes(1); + expect(mockTransportClose).not.toHaveBeenCalled(); + + // After the idle timeout, the transport is finally closed. + jest.advanceTimersByTime(5_000); + expect(mockTransportClose).toHaveBeenCalledTimes(1); + + jest.useRealTimers(); }); - it('closes transport after a failed action', async () => { + it('closes transport after a failed action once idle', async () => { const consoleSpy = jest .spyOn(console, 'error') .mockImplementation(() => undefined); mockGetAddress.mockRejectedValue(new Error('Device error')); + jest.useFakeTimers(); + await sendAction(LedgerAction.getPublicKey, { hdPath: "m/44'/60'/0'/0/0", }); - await flushPromises(); + // The transport stays open immediately after the failure; only the idle + // timer is (re)scheduled. + expect(mockTransportClose).not.toHaveBeenCalled(); + jest.advanceTimersByTime(5_000); expect(mockTransportClose).toHaveBeenCalled(); + consoleSpy.mockRestore(); + jest.useRealTimers(); + }); + + it('waits for an in-flight idle close to settle before opening a new transport', async () => { + mockGetAddress.mockResolvedValue({ + publicKey: '04abcd1234', + address: '0x1234567890abcdef', + chainCode: 'chaincode123', + }); + + // Hold the idle close's `transport.close()` in flight so we can prove + // a new action waits for it instead of racing to open a new transport. + let resolveClose!: () => void; + mockTransportClose.mockReturnValue( + new Promise((resolve) => { + resolveClose = resolve; + }), + ); + + jest.useFakeTimers(); + + // First action opens transport T1. + await sendAction(LedgerAction.getPublicKey, { + hdPath: "m/44'/60'/0'/0/0", + }); + expect(mockCreate).toHaveBeenCalledTimes(1); + + // Idle window elapses: T1.close() starts and stays pending. + jest.advanceTimersByTime(5_000); + expect(mockTransportClose).toHaveBeenCalledTimes(1); + expect(mockCreate).toHaveBeenCalledTimes(1); + + // A new action arrives while T1.close() is still pending. It must NOT + // reopen the transport until the close settles. + const secondAction = sendAction(LedgerAction.getPublicKey, { + hdPath: "m/44'/60'/1'/0/0", + }); + // handleAction has run its synchronous preamble and is now suspended at + // `await closeInProgress`; the transport is not reopened yet. + expect(mockCreate).toHaveBeenCalledTimes(1); + + // Let the close settle; the new action proceeds to open T2. + resolveClose(); + await secondAction; + expect(mockCreate).toHaveBeenCalledTimes(2); + + jest.useRealTimers(); + }); + + it('forceReset drops the transport synchronously and fire-and-forgets the close', async () => { + mockGetAddress.mockResolvedValue({ + publicKey: '04abcd1234', + address: '0x1234567890abcdef', + chainCode: 'chaincode123', + }); + + // Open a transport via an action. + await sendAction(LedgerAction.getPublicKey, { + hdPath: "m/44'/60'/0'/0/0", + }); + expect(mockCreate).toHaveBeenCalledTimes(1); + + mockTransportClose.mockClear(); + (handler as unknown as { forceReset: () => void }).forceReset(); + + // References are dropped synchronously; close is best-effort. + expect( + (handler as unknown as { transport: unknown }).transport, + ).toBeNull(); + expect((handler as unknown as { ethApp: unknown }).ethApp).toBeNull(); + expect(mockTransportClose).toHaveBeenCalledTimes(1); + + // A subsequent action opens a fresh transport. + mockGetAddress.mockClear(); + await sendAction(LedgerAction.getPublicKey, { + hdPath: "m/44'/60'/1'/0/0", + }); + expect(mockCreate).toHaveBeenCalledTimes(2); }); }); diff --git a/app/offscreen/hardware-wallets/ledger.ts b/app/offscreen/hardware-wallets/ledger.ts index 6ca2877859a5..6bf82cf85c85 100644 --- a/app/offscreen/hardware-wallets/ledger.ts +++ b/app/offscreen/hardware-wallets/ledger.ts @@ -24,6 +24,9 @@ import { } from '../../../shared/constants/offscreen-communication'; import { LEDGER_USB_VENDOR_ID } from '../../../shared/constants/hardware-wallets'; +/** Idle grace period before closing the WebHID transport between action bursts. */ +const TRANSPORT_IDLE_TIMEOUT_MS = 5_000; + /** * Checks if WebHID API is available in this environment. * @@ -92,6 +95,15 @@ export class LedgerLegacyHandler { | ((event: { device: HIDDevice }) => void) | null = null; + // Idle timer that closes the transport between bursts of actions; cleared on + // each new action and in `destroy()`. + private idleCloseTimer: ReturnType | null = null; + + // In-flight `transport.close()` promise, if any. Tracked so a new + // `handleAction` can await it before opening a transport, avoiding an + // open/close overlap on the same HID device. + private closeInProgress: Promise | null = null; + /** * Attempts to open a transport to an already-permitted Ledger device. * This does NOT require a user gesture - it only works for devices @@ -173,16 +185,52 @@ export class LedgerLegacyHandler { * Clears state synchronously first to prevent races with reconnection. */ private async closeTransport(): Promise { + this.clearIdleClose(); const transportToClose = this.transport; this.transport = null; this.ethApp = null; - if (transportToClose) { + if (!transportToClose) { + return; + } + + // Track the close so a concurrent `handleAction` awaits it before opening + // a new transport. Assigned synchronously before the first `await`. + const closePromise = (async () => { try { await transportToClose.close(); } catch { // Ignore close errors } + })(); + this.closeInProgress = closePromise; + try { + await closePromise; + } finally { + if (this.closeInProgress === closePromise) { + this.closeInProgress = null; + } + } + } + + /** + * Best-effort synchronous reset of the transport, called by the router when + * an action has wedged past its timeout. Drops the transport/app + * references synchronously (so the next action opens a fresh transport with a + * fresh `_appAPIlock`) and fire-and-forgets the close — a hung close must + * not block recovery. + */ + forceReset(): void { + this.clearIdleClose(); + this.closeInProgress = null; + this.pendingMakeApp = null; + const transportToClose = this.transport; + this.transport = null; + this.ethApp = null; + if (transportToClose) { + Promise.resolve(transportToClose.close()).catch(() => { + /* best-effort: ignore close failures during forced reset */ + }); } } @@ -538,9 +586,8 @@ export class LedgerLegacyHandler { * Public entry point for processing Ledger actions. * * Used by the centralized ledger-router so both DMK and Legacy handlers - * expose the same `handleAction` surface for the message listener. - * Closes the underlying WebHID transport after every action so the device - * is released back to the OS even when the action fails. + * expose the same `handleAction` surface. The transport is kept open across + * actions and closed after an idle period (`TRANSPORT_IDLE_TIMEOUT_MS`). * * @param action - The Ledger action to perform (e.g. `getPublicKey`, * `signTransaction`). Must be a member of `LedgerAction`. @@ -555,10 +602,33 @@ export class LedgerLegacyHandler { action: LedgerAction, params?: Record, ): Promise { + this.clearIdleClose(); + // If the idle timer already fired, wait for the in-flight close before + // opening a transport (avoids an open/close overlap on the HID device). + if (this.closeInProgress) { + await this.closeInProgress; + } try { return await this.handleLedgerAction(action, params); } finally { - await this.closeTransport(); + this.scheduleIdleClose(); + } + } + + /** Schedules a deferred `closeTransport()` after the idle timeout. */ + private scheduleIdleClose(): void { + this.clearIdleClose(); + this.idleCloseTimer = setTimeout(() => { + this.idleCloseTimer = null; + this.closeTransport(); + }, TRANSPORT_IDLE_TIMEOUT_MS); + } + + /** Cancels a pending idle close. Called at the start of each action and in `destroy()`. */ + private clearIdleClose(): void { + if (this.idleCloseTimer) { + clearTimeout(this.idleCloseTimer); + this.idleCloseTimer = null; } } @@ -570,6 +640,7 @@ export class LedgerLegacyHandler { * Safe to call multiple times. */ async destroy(): Promise { + this.clearIdleClose(); if (this.hidConnectListener && typeof navigator !== 'undefined') { navigator.hid.removeEventListener('connect', this.hidConnectListener); this.hidConnectListener = null; @@ -583,6 +654,10 @@ export class LedgerLegacyHandler { this.hidDisconnectListener = null; } + // Wait for any in-flight idle close before our own (no-op) close. + if (this.closeInProgress) { + await this.closeInProgress; + } await this.closeTransport(); } diff --git a/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.test.ts b/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.test.ts index 5d2a4f8e6895..cebde3d6a9d2 100644 --- a/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.test.ts +++ b/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.test.ts @@ -7,7 +7,12 @@ import { LedgerAction, OffscreenCommunicationTarget, } from '../../../../shared/constants/offscreen-communication'; -import { LedgerOffscreenBridge } from './ledger-offscreen-bridge'; +import { + LedgerOffscreenBridge, + MESSAGE_TIMEOUT_MS, + GET_PUBLIC_KEY_TIMEOUT_MS, + SIGN_TIMEOUT_MS, +} from './ledger-offscreen-bridge'; type SendMessageCallback = (response: unknown) => void; @@ -271,14 +276,68 @@ describe('LedgerOffscreenBridge', () => { }); describe('timeout handling', () => { - it('rejects with "Ledger iframe timeout" after the configured timeout', async () => { + it('rejects with a descriptive timeout error after the configured timeout', async () => { jest.useFakeTimers(); const bridge = new LedgerOffscreenBridge(); const promise = bridge.attemptMakeApp(); - jest.advanceTimersByTime(5000); + // attemptMakeApp uses MESSAGE_TIMEOUT_MS; cross it. + jest.advanceTimersByTime(MESSAGE_TIMEOUT_MS + 1_000); + + await expect(promise).rejects.toThrow( + `Ledger device did not respond to "ledger-make-app" within ${MESSAGE_TIMEOUT_MS}ms`, + ); + }); + + it('getPublicKey rejects after GET_PUBLIC_KEY_TIMEOUT_MS (30s) when the offscreen never responds', async () => { + jest.useFakeTimers(); + const bridge = new LedgerOffscreenBridge(); + const promise = bridge.getPublicKey({ hdPath: "m/44'/60'/0'/0/0" }); + + // Just under the timeout: still pending. + jest.advanceTimersByTime(GET_PUBLIC_KEY_TIMEOUT_MS - 1_000); + const settled = await Promise.race([ + promise.then( + () => 'resolved', + () => 'rejected', + ), + Promise.resolve('pending'), + ]); + expect(settled).toBe('pending'); + + // Cross the threshold. + jest.advanceTimersByTime(2_000); + + await expect(promise).rejects.toThrow( + `Ledger device did not respond to "ledger-unlock" within ${GET_PUBLIC_KEY_TIMEOUT_MS}ms`, + ); + }); + + it('deviceSignTransaction rejects after SIGN_TIMEOUT_MS (5min) when the offscreen never responds', async () => { + jest.useFakeTimers(); + const bridge = new LedgerOffscreenBridge(); + const promise = bridge.deviceSignTransaction({ + hdPath: "m/44'/60'/0'/0/0", + tx: '0x0', + }); + + jest.advanceTimersByTime(SIGN_TIMEOUT_MS + 1); + + await expect(promise).rejects.toThrow( + `Ledger device did not respond to "ledger-sign-transaction" within ${SIGN_TIMEOUT_MS}ms`, + ); + }); + + it('getPublicKey still resolves if the offscreen responds before the timeout', async () => { + jest.useFakeTimers(); + const bridge = new LedgerOffscreenBridge(); + const expected = { publicKey: '04abcd', address: '0xabc' }; + const promise = bridge.getPublicKey({ hdPath: "m/44'/60'/0'/0/0" }); - await expect(promise).rejects.toThrow('Ledger iframe timeout'); + jest.advanceTimersByTime(1_000); + respond({ success: true, payload: expected }); + + await expect(promise).resolves.toEqual(expected); }); }); }); diff --git a/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.ts b/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.ts index 499067782712..bfa8b356ff82 100644 --- a/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.ts +++ b/app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.ts @@ -14,7 +14,25 @@ import { OffscreenCommunicationTarget, } from '../../../../shared/constants/offscreen-communication'; -const MESSAGE_TIMEOUT = 4000; +export const MESSAGE_TIMEOUT_MS = 4000; + +/** + * Timeout for `getPublicKey` requests sent to the offscreen document. + * + * `getPublicKey` does not require user interaction on the device (the address + * is returned without a confirmation prompt), so a relatively short timeout is + * appropriate. If the offscreen/WebHID round-trip wedges, this converts the + * otherwise-indefinite hang into a recoverable rejection. + */ +export const GET_PUBLIC_KEY_TIMEOUT_MS = 30_000; + +/** + * Timeout for signing requests sent to the offscreen document. + * + * Signing requires the user to physically confirm on the Ledger device, which + * can take longer; allow up to 5 minutes before giving up. + */ +export const SIGN_TIMEOUT_MS = 300_000; /** * The options for the LedgerOffscreenBridge are empty because the bridge @@ -73,7 +91,7 @@ export class LedgerOffscreenBridge implements Omit< { action: LedgerAction.makeApp, }, - { timeout: MESSAGE_TIMEOUT }, + { timeout: MESSAGE_TIMEOUT_MS }, ); } @@ -83,7 +101,7 @@ export class LedgerOffscreenBridge implements Omit< action: LedgerAction.updateTransport, params: { transportType }, }, - { timeout: MESSAGE_TIMEOUT }, + { timeout: MESSAGE_TIMEOUT_MS }, ); } @@ -92,7 +110,7 @@ export class LedgerOffscreenBridge implements Omit< { action: LedgerAction.getAppNameAndVersion, }, - { timeout: MESSAGE_TIMEOUT }, + { timeout: MESSAGE_TIMEOUT_MS }, ); } @@ -101,7 +119,7 @@ export class LedgerOffscreenBridge implements Omit< { action: LedgerAction.getAppConfiguration, }, - { timeout: MESSAGE_TIMEOUT }, + { timeout: MESSAGE_TIMEOUT_MS }, ); } @@ -110,10 +128,13 @@ export class LedgerOffscreenBridge implements Omit< address: string; chainCode?: string; }> { - return this.#sendMessage({ - action: LedgerAction.getPublicKey, - params, - }); + return this.#sendMessage( + { + action: LedgerAction.getPublicKey, + params, + }, + { timeout: GET_PUBLIC_KEY_TIMEOUT_MS }, + ); } deviceSignTransaction(params: { hdPath: string; tx: string }): Promise<{ @@ -121,29 +142,38 @@ export class LedgerOffscreenBridge implements Omit< s: string; r: string; }> { - return this.#sendMessage({ - action: LedgerAction.signTransaction, - params, - }); + return this.#sendMessage( + { + action: LedgerAction.signTransaction, + params, + }, + { timeout: SIGN_TIMEOUT_MS }, + ); } deviceSignMessage(params: { hdPath: string; message: string; }): Promise<{ v: number; s: string; r: string }> { - return this.#sendMessage({ - action: LedgerAction.signPersonalMessage, - params, - }); + return this.#sendMessage( + { + action: LedgerAction.signPersonalMessage, + params, + }, + { timeout: SIGN_TIMEOUT_MS }, + ); } deviceSignTypedData( params: LedgerSignTypedDataParams, ): Promise { - return this.#sendMessage({ - action: LedgerAction.signTypedData, - params, - }); + return this.#sendMessage( + { + action: LedgerAction.signTypedData, + params, + }, + { timeout: SIGN_TIMEOUT_MS }, + ); } async #sendMessage( @@ -155,7 +185,11 @@ export class LedgerOffscreenBridge implements Omit< if (timeout) { responseTimeout = setTimeout(() => { - reject(new Error('Ledger iframe timeout')); + reject( + new Error( + `Ledger device did not respond to "${message.action}" within ${timeout}ms`, + ), + ); }, timeout); } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 96e4395d2cc8..85c31be63314 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -5651,15 +5651,7 @@ export default class MetamaskController extends EventEmitter { hdPath, hdPathDescription, ) { - // `createAccounts` derives the account address from the device, so a - // locked or unresponsive device can make this call hang indefinitely. - // Unlike the pure-read hardware methods (which run on the lock-free - // `deviceRead` path), `createAccounts` mutates vault state and must run - // under the controller lock, so it cannot use `deviceRead: true`. Wrap - // it in the same UX backstop as `#withKeyringForDevice`'s device-read - // branch so a wedged device rejects with an actionable error instead of - // leaving the UI spinner (and `showLoadingIndication`) stuck forever. - const createOperation = this.#withKeyringForDevice( + const { address: unlockedAccount } = await this.#withKeyringForDevice( { name: deviceName, hdPath }, async (keyring) => { const { entropySource } = keyring; @@ -5744,39 +5736,6 @@ export default class MetamaskController extends EventEmitter { }, ); - let timeoutHandle; - let timedOut = false; - let unlockedAccount; - try { - ({ address: unlockedAccount } = await Promise.race([ - createOperation, - new Promise((_resolve, reject) => { - timeoutHandle = setTimeout(() => { - timedOut = true; - reject( - new Error( - `Hardware wallet account creation timed out for device: ${deviceName}. Make sure the device is connected and unlocked, then try again.`, - ), - ); - }, HARDWARE_DEVICE_READ_TIMEOUT_MS); - }), - ])); - } finally { - clearTimeout(timeoutHandle); - if (timedOut) { - // The abandoned create operation still holds the controller lock until - // the device call settles; observe its rejection so it never surfaces - // as an unhandled rejection. Mirrors the device-read path in - // `#withKeyringForDevice`. - createOperation.catch((error) => - log.warn( - `Abandoned hardware wallet account creation failed after timeout for device: ${deviceName}`, - error, - ), - ); - } - } - const accounts = this.accountsController.listAccounts(); const internalAccount = diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index c037edd4163d..9ffbb28e404c 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -2794,70 +2794,6 @@ describe('MetaMaskController', () => { }, ); - it('times out the abandoned account creation when the device wedges, instead of leaving the UI spinner stuck forever', async () => { - // Regression test: a locked/unresponsive device makes - // `keyring.createAccounts` hang forever. `createAccounts` mutates - // vault state so it cannot run on the lock-free `deviceRead` path, - // but the UX backstop must still reject so the UI thunk's - // `hideLoadingIndication()` runs and the spinner clears. - const withKeyringV2Spy = jest - .spyOn(metamaskController.keyringController, 'withKeyringV2') - .mockImplementation(async (selector, callback) => { - expect(selector).toStrictEqual({ type: KeyringTypeV2.Lattice }); - - return await callback({ - keyring: { - entropySource: 'test-entropy-source', - createAccounts: jest - .fn() - .mockReturnValue(new Promise(() => undefined)), - network: null, - }, - }); - }); - - // Intercept the device-read backstop timer so the test can fire it - // deterministically without faking every timer in the app. - const originalSetTimeout = global.setTimeout; - let fireDeviceReadTimeout; - const setTimeoutSpy = jest - .spyOn(global, 'setTimeout') - .mockImplementation((handler, timeout, ...args) => { - if (timeout === HARDWARE_DEVICE_READ_TIMEOUT_MS) { - fireDeviceReadTimeout = handler; - return 0; - } - return originalSetTimeout(handler, timeout, ...args); - }); - - try { - const wedgedUnlock = metamaskController.unlockHardwareWalletAccount( - accountToUnlock, - HardwareDeviceNames.lattice, - ); - // Swallow the timeout rejection asserted below so the wedged - // promise never surfaces as an unhandled rejection. - wedgedUnlock.catch(() => undefined); - - // Let `unlockHardwareWalletAccount` reach the (hanging) create call. - await new Promise((resolve) => { - const poll = () => - withKeyringV2Spy.mock.calls.length > 0 - ? resolve() - : originalSetTimeout(poll, 5); - poll(); - }); - - // The abandoned account creation is bounded by the UX backstop. - fireDeviceReadTimeout(); - await expect(wedgedUnlock).rejects.toThrow( - 'Hardware wallet account creation timed out', - ); - } finally { - withKeyringV2Spy.mockRestore(); - setTimeoutSpy.mockRestore(); - } - }); }); }); From 8cc8db861af39eda93ca890b29029be79690d6fa Mon Sep 17 00:00:00 2001 From: gantunesr <17601467+gantunesr@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:59:41 -0400 Subject: [PATCH 2/2] fix: lint --- app/scripts/metamask-controller.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 9ffbb28e404c..291684b238e0 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -2793,7 +2793,6 @@ describe('MetaMaskController', () => { }); }, ); - }); });