Serialize account and network changes with safe access and toolbar updates - #1603
Conversation
- Defer website access prompts until active settings updates finish - Add concurrency and failure regression coverage
b7e2b11 to
1bd57d8
Compare
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
The refactor correctly extracts website-access prompting out of the active-settings semaphore to avoid a dialog-lock ordering deadlock, but it introduces three structural/functional regressions: (1) the new promptForWebsiteAccesses wraps the entire connection iteration in a single try/catch so one failing prompt aborts all remaining prompts, (2) the per-connection access-classification logic is now duplicated between updateTabConnections and promptForWebsiteAccesses and can silently diverge, and (3) the lock-then-prompt-after-release orchestration is replicated as boilerplate across both public entry points in activeSettings.ts.
| if (access === 'askAccess' && connection.wantsToConnect && promptForAccessesIfNeeded && ethereum !== undefined && tokenPriceService !== undefined && resetSimulationServices !== undefined) { | ||
| const activeAddress = currentActiveAddress !== undefined ? currentActiveAddress : undefined | ||
| await askUserForAccessOnConnectionUpdate(ethereum, tokenPriceService, resetSimulationServices, websiteTabConnections, connection.socket, connection.websiteOrigin, activeAddress, settings) | ||
| // Prompts acquire the access-dialog lock, so callers holding the active-settings lock must defer this phase. |
There was a problem hiding this comment.
A single failing access prompt now aborts all remaining prompts. promptForWebsiteAccesses wraps the entire iteration over every websiteTabConnections/connection in one try/catch, so if askUserForAccessOnConnectionUpdate throws for one site (e.g. openPopupOrTab returns a missing dialog, or browser.windows.create fails), the loop terminates and every other pending askAccess connection is silently never prompted. Previously this error handling was per-tab inside updateTabConnections (run via Promise.all over updatePromises), so one tab's failed prompt did not stop the other tabs' prompts (which had already been dispatched). Catch per-connection (or continue past the wrapped call) so one failed prompt does not skip the remaining sites.
| for (const tabConnection of websiteTabConnections.values()) { | ||
| for (const connection of Object.values(tabConnection.connections)) { | ||
| if (!connection.wantsToConnect) continue | ||
| const settings = await getSettings() |
There was a problem hiding this comment.
The per-connection access-classification logic is now duplicated. updateTabConnections (for connect/disconnect) and promptForWebsiteAccesses (for prompting) each independently recompute the same decision via getActiveAddressForCurrentSignerState + hasAddressAccess/hasAccess, and now operate on different settings snapshots (the passed-in committed snapshot vs. a fresh getSettings() read after lock release). The two passes can silently diverge, and future changes to how access is resolved must be propagated to both. Extract a single 'resolve access for connection' helper that both the connect/disconnect pass and the prompt pass consume so they can never disagree.
| }) | ||
| } | ||
| if (activeSigningSafeContextChanged) await updatePopupVisualisationIfNeeded(ethereum, tokenPriceService, false, false) | ||
| await sendActiveAccountChangeToApprovedWebsitePorts(websiteTabConnections, await getSettings()) |
There was a problem hiding this comment.
The lock-then-prompt-after-release orchestration is replicated as boilerplate across changeActiveAddressAndChain and activateAddressSelection: each wraps its critical section in changeActiveAddressAndChainSemaphore, calls the *Unlocked variant that deliberately suppresses prompting, then calls promptForWebsiteAccesses only after the semaphore is released. This implicit protocol (hold the semaphore, then prompt outside it) is not expressed in the structure; any future caller that needs an address/chain transition (e.g. changeActiveRpc in this same file) must remember to reproduce it, and forgetting the post-unlock prompt silently drops access prompts. Contain the semaphore scope and the deferred prompt in a single higher-level 'act and then prompt' operation with the lock lifetime kept internal.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The diff fixes a real cross-semaphore deadlock between the access-dialog lock and the active-settings lock by deferring access prompts until after the settings semaphore is released, and it preserves the access/approval and disconnect logic via the extracted getConnectionAccess helper. The core fix is sound and no security or correctness regressions were found by the security/correctness reviewers. Two structural concerns remain: (1) access prompting is now split across two divergent orchestration paths (updateWebsiteApprovalAccesses' inline branch vs. the new exported promptForWebsiteAccesses) that sample settings differently and couple activeSettings.ts to accessManagement internals; (2) the active-settings critical section was widened to pull settings persistence and the full access reconciliation inside the single global semaphore, serializing all address/chain/simulation transitions behind one lock.
| const activeAddress = currentActiveAddress !== undefined ? currentActiveAddress : undefined | ||
| await askUserForAccessOnConnectionUpdate(ethereum, tokenPriceService, resetSimulationServices, websiteTabConnections, connection.socket, connection.websiteOrigin, activeAddress, settings) | ||
| // Prompts acquire the access-dialog lock, so callers holding the active-settings lock must defer this phase. | ||
| export async function promptForWebsiteAccesses(ethereum: EthereumClientService, tokenPriceService: TokenPriceService, resetSimulationServices: ResetSimulationServices, websiteTabConnections: WebsiteTabConnections, throwOnError = false) { |
There was a problem hiding this comment.
Access prompting now has two divergent orchestration paths. updateWebsiteApprovalAccesses retains its inline prompt branch (still reached with true by the settlement flow via finalizeWebsiteAccessChange/persistWebsiteAccessChange), while activeSettings.ts calls it with false inside the lock and then runs the new exported promptForWebsiteAccesses after releasing the lock. The two paths sample settings differently: the inline path uses the settings snapshot passed into updateWebsiteApprovalAccesses, whereas promptForWebsiteAccesses calls getSettings() per connection for the latest committed state. The same access update can therefore produce different access decisions depending on which entry point drives it, and activeSettings.ts must now sequence an accessManagement pipeline and internalize its locking contract (the 'must defer this phase' comment leaks accessManagement's internals into the settings module). Any future change to reconcile/prompt behavior must be coordinated across two modules and two code paths with subtly different semantics.
|
|
||
| const changeActiveAddressAndChainSemaphore = new Semaphore(1) | ||
| export async function changeActiveAddressAndChain( | ||
| async function runActiveSettingsChange( |
There was a problem hiding this comment.
The changeActiveAddressAndChainSemaphore critical section was widened substantially. Previously the semaphore protected only the tail of the change (message fan-out to approved ports and simulation-state resets), while settings persistence and updateWebsiteApprovalAccesses (including per-tab updateExtensionIcon calls and declarative-net-request block updates) ran outside the lock. Now runActiveSettingsChange/changeActiveAddressAndChainUnlocked run all of settings persistence, the full access reconciliation across every tab, message fan-out, and simulation reset inside the single global semaphore. Every active-address/chain/simulation transition across the entire extension now serializes behind one global lock, so any slow network/icon-driven step added to the reconciliation directly lengthens the critical section for unrelated threads that don't need the ordering at all — a scalability regression. Ordering may only be required for the persistence step; consider confirming which portion of updateWebsiteApprovalAccesses needs to be ordered versus what can be pushed back out of the lock.
|
/review |
There was a problem hiding this comment.
CI Agent Review
This changeset is a concurrency/ordering refactor of the active-settings transition path in accessManagement.ts, activeSettings.ts, and iconHandler.ts (with a new concurrency test suite). Security review found no access-control, injection, or escalation issues: port connect/disconnect decisions, address filtering for approved ports, and deferred access prompting all preserve the existing authorization semantics. The refactor correctly resolves a lock-order inversion (deferred access dialogs and icon refreshes now run after the settings semaphore is released rather than under it). One architectural concern was flagged about the new two-phase reconcile/finish contract.
| type ActiveAddressAndChainChange = { | ||
| simulationMode: boolean | ||
| activeAddress?: bigint | ||
| signingAddressSelection?: 'signer' | 'safe' |
There was a problem hiding this comment.
Architectural concern: the two-phase "reconcile / finish" contract leaks internal pipeline ordering across module boundaries. changeActiveAddressAndChainUnlocked returns a deferred work-item (accessUpdate.finish), and runActiveSettingsChange awaits that separately-returned closure. Correctness of a settings change now depends on every caller remembering to invoke finish() (which performs access prompting and icon refresh); there is nothing in the API enforcing it, so a future caller that forgets finish, or doesn't await it, silently drops user-visible work. This also couples activeSettings.ts to the low-level reconcileWebsiteApprovalAccesses because it needs to defer finish past its own settings lock, bypassing the single stable updateWebsiteApprovalAccesses entry point that all other consumers still use. Finally, because completion is conditional on no exception occurring after persistence, the pre-existing throw new Error('Signing mode can only activate the external signer or an owned Gnosis Safe.') path in activateAddressSelection can leave a persisted settings change whose access-prompt/icon finishing is silently skipped. Not a behavior bug with the current sole production consumer, but a maintenance trap and a regression in how failures leave the system partially applied; consider keeping the deferred-completion pattern fully internal to accessManagement so it is not an exported { ..., finish } object that correctness depends on.
|
/review |
There was a problem hiding this comment.
CI Agent Review
This concurrency/ordering refactor of the active-settings update path is well-tested and the concurrency guarantees (per-tab icon serialization, deferred prompting with latest-settings recheck, reconciliation inside and completion outside the ordered lock) are preserved or hardened. No security regression was found by the review agents. The one substantive concern is a maintainability regression in the new runWithWebsiteAccessUpdates completion-inversion construct, which hides deferred side effects (access dialogs, toolbar updates) inside a finally that callers neither see nor control at the call site.
| } | ||
|
|
||
| export type WebsiteAccessReconciler = (settings: Settings) => Promise<number> | ||
|
|
There was a problem hiding this comment.
Maintainability concern: runWithWebsiteAccessUpdates/reconcile(settings) introduce a deferred-completion inversion of control with hidden side effects. Invoking the reconcile: WebsiteAccessReconciler callback (e.g. inside changeActiveAddressAndChainUnlocked in activeSettings.ts) returns only a refresh-generation number but silently registers substantial deferred completion work — opening access-approval dialogs and refreshing every tab's toolbar icon/state — that is executed later in runWithWebsiteAccessUpdates's finally, after the semaphore lock is released. The caller neither sees nor awaits this work at the call site, so the completion contract lives only in the framework's finally loop and inline comments, not in the function signatures. This threads a locking/orchestration concern into a domain module (accepting an arbitrary runOrdered executor into accessManagement), forces every settings transition (changeActiveAddressAndChain, activateAddressSelection, runActiveSettingsChange) to pass a reconcile callback through its call stack, and obscures side effects such that a future maintainer could unintentionally skip, duplicate, or mis-time access prompts / toolbar updates (dropping UI completion silently). The same deadlock-free guarantee was achievable with less indirection — e.g. keeping the explicit flow and having callers invoke a named finishWebsiteAccessUpdate(...) after releasing their semaphore, as iconHandler's scoped executor already serializes toolbar writes. The concurrency guarantees themselves appear correct and well-tested; the concern is the structural/extension cost of this callback-based deferred-completion protocol.
|
/review |
KillariDev
changed the title
2 participants
Overlapping account or network changes could persist newer settings before an older transition reset services or emitted dapp events, leaving settings, live services, and notifications out of sync. This PR serializes the settings transition and keeps access dialogs and toolbar updates outside the settings lock.
Changes
Regression coverage
Covers overlapping network and account selections, signer/Safe preferences, access-approval overlap, prompt failures across connections, latest-address consent requests, delayed toolbar writes, invalid selections, and access finalization after service-reset failures.
Validation
Passed on
ed03d47b:bun run test— 1,282 tests passed, 0 failures across 151 filesbun run setup-chromebun run typecheckbun run lintbun run test:chrome-communication— reachedaccess-grantedgit diff --check