harden: the background must stop reading the shared state singleton — one root cause, five sites, and the fixes keep adding new ones #324
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Root-cause issue. Five defects now trace to one thing:
src/background/index.jsreads and writes the module-levelstatesingleton insrc/shared/state.js, which the MV3 service worker never populates and which nothing makes fail loudly when read unpopulated. It silently servesDEFAULT_STATE.The five, in the order they surfaced:
DEFAULT_STATEover the profile, destroying every wallet. Fixed.eth_chainId/net_versionreported mainnet to a page whose user was on Sepolia. In flight.getProvider()gets a mainnet static hint on a cold worker, so every non-mainnet dApp send is prepared wrong and then refused by the wallet's own verifier.state.rpcUrlatsrc/background/index.js:1380, several awaits after itsloadState()at:1306, so a concurrent committed chain switch can move the endpoint under it. Pre-existing, never filed separately.loadState()on a page-callable path detaches the objectsbackgroundRefresh()is mutating in place across its network round trip, sosaveState()persists pre-refresh balances and still stampslastBalanceRefresh, suppressing a redo.src/content/inpage.js:244sendseth_chainIdon every page load, so any page can trigger it.Item 5 is the reason this issue exists rather than a sixth point fix. The remedy applied to the first four — add a
loadState()before the access — is what created it. Each singleton fix is a new opportunity to mutate shared state at a moment some other in-flight handler depended on it not moving. Fixing site six the same way will produce site seven.What is actually wrong
Two things, and only the second is hard:
getState(), a fresh storage read returning a detached object, used byeth_accounts,wallet_getPermissions,handleConnectionRequestandgetRpcUrl. Handlers that use it have never produced a defect here.Definition of done
statesingleton. Every handler uses a per-callgetState()read, and every write is a read-modify-write that does not publish a shared object other in-flight work holds.no-restricted-importson the singleton export from background files, or equivalent) that failsmake check. A convention nobody can violate beats a convention everyone remembers.backgroundRefresh()no longer mutates address objects in place across a network round trip while another handler may replace them.DEFAULT_STATE— at minimum in the background context.chrome.storage.localstructured-clone onget, as the real API does. The aliasing stub intests/coldWorkerChainId.test.jshides this entire defect class and made a real defect invisible; audit every storage stub intests/for the same flaw.make checkgreen.Note for sequencing: #304 and #311 touch the same persistence layer. Whoever takes this should take those or coordinate closely; three units rewriting state handling in parallel will conflict badly.
The stub audit this issue asks for has been done. Eight test files carry the aliasing flaw, so the DoD item is scoped, not open-ended.
Both
getandsetalias:tests/txStatus.test.jstests/alarms.test.jstests/settingsUtcTimestamps.test.jstests/coldWorkerChainSwitch.test.jstests/networkEndpoints.test.jsAliasing
getwith a no-opset:tests/state.test.jstests/chainSwitchGate.test.jstests/backgroundApproval.test.jsTwo are worth singling out.
tests/coldWorkerChainSwitch.test.jsis the direct sibling this issue's evidence came from — the stub fixed in #319 was cloned from it, and the original still aliases. Andtests/networkEndpoints.test.js:124feeds one module's live in-memory object back in as the next module's "persisted bytes".Why this is load-bearing rather than tidiness:
loadState()assigns nested references straight out of the get result (state.networkEndpoints = saved.networkEndpoints). Over an aliasing stub, a test can assert "the endpoint was persisted" and pass on a build that never callssaveState()at all. Measured, not theorised: with the aliasinggetrestored over the defective handler #319 fixed, the suite passes 794/794.So these eight files may be asserting less than they appear to, and re-running them proves nothing until the stubs clone. Fix the stubs first, then see which assertions still hold.