diff --git a/README.md b/README.md index a4fa830..e0ec471 100644 --- a/README.md +++ b/README.md @@ -1379,6 +1379,17 @@ carries a `schemaVersion` — `STATE_SCHEMA_VERSION` in `saveState()` and the background's `updateState()` both do, so whichever context wrote last, the record says which build's shape it is in. +Each write reads the record, changes it and writes it back, holding a lock from +the read to the write: the browser's own (`navigator.locks`), which every page +of the extension and its background share, taken through `withLock()` in +`src/shared/browserApi.js`. The popup's `saveState()`, the background's +`updateState()` and the StateRecovery reset all take it, so no write lands +between another write's read and its write, where it would be lost. Chrome has +it from version 69, older than any Chrome that runs a Manifest V3 extension, in +pages and in the service worker; Firefox from version 96, older than the 115 +`manifest/firefox.json` declares. Where it is missing, a write fails rather than +going ahead without it. + Version 1 is the shape that shipped before versions existed, so a stored record with no `schemaVersion` is version 1 rather than a defect: it loads normally and is migrated in place by being stamped on the first write. An upgrade never shows diff --git a/package.json b/package.json index 33642eb..a8ca0a0 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,11 @@ "fmt": "prettier --write .", "fmt-check": "prettier --check ." }, + "jest": { + "setupFiles": [ + "/tests/support/navigatorLocks.js" + ] + }, "devDependencies": { "@eslint/js": "10.0.1", "@tailwindcss/cli": "4.3.1", diff --git a/src/background/state.js b/src/background/state.js index 45f9f81..885e256 100644 --- a/src/background/state.js +++ b/src/background/state.js @@ -16,17 +16,19 @@ // across any number of awaits and no concurrent work can // move it. // updateState(fn) read fresh, apply fn to that fresh record, write it -// back — all inside a queue, so two background writes -// never interleave, and the read is one storage round trip -// ahead of the write rather than a page lifetime ahead of -// it (which is what made the popup's saveState() need a -// per-field merge against a baseline at all). +// back — all holding the lock every write of the stored +// record takes, so no other write, by the background or a +// page, lands in between, and the read is one storage +// round trip ahead of the write rather than a page +// lifetime ahead of it (which is what made the popup's +// saveState() need a per-field merge against a baseline +// at all). // // A handler that must both read and write therefore does its network work // against a snapshot it owns, and applies the RESULT inside updateState(). // It never publishes an object other in-flight work is holding. -const { storageGet, storageSet } = require("../shared/browserApi"); +const { storageGet, storageSet, withLock } = require("../shared/browserApi"); const { normalizePersisted } = require("../shared/persistedState"); const { STATE_SCHEMA_VERSION, @@ -51,11 +53,6 @@ async function getState() { return normalizePersisted(result.autistmask); } -// Serializes the read-modify-write turns below. Two of them interleaved would -// each read before the other wrote, and the second write would carry the first -// one's fields back to their pre-turn values. -let updateQueue = Promise.resolve(); - async function updateStateOnce(mutate) { const s = await getState(); await mutate(s); @@ -70,27 +67,21 @@ async function updateStateOnce(mutate) { // Apply `mutate` to a record read fresh from storage and write the result // back. `mutate` receives a detached, normalized profile and mutates it in -// place; it may be async, but it must not do anything slow — the window -// between the read and the write is the window in which another context's -// write is lost, and keeping it to one storage round trip is what makes a -// whole-record write safe here. Concretely: the write is the WHOLE record, so -// a popup write that lands inside that window is reverted, in every field, by -// the record this turn read before it. That is accepted because the window is -// one round trip long and the popup is not writing while the worker is; -// widening it is what would make it a real hazard. +// place; it may be async, but it must not do anything slow. The write is the +// WHOLE record, which is safe only because the lock (withLock() in +// src/shared/browserApi.js) is held from the read to the write: two +// background turns, or a turn and a page's save, never interleave +// (https://git.eeqj.de/sneak/AutistMask/issues/641). Every page's save waits +// while it is held. // // `mutate` must also not call updateState() itself, directly or through -// anything it awaits: the queue is strictly serial, so the inner turn waits on -// the outer one, which is waiting on the inner one. That deadlocks the whole -// background, not just the caller. Mutate the record you were handed. +// anything it awaits: the inner turn waits for the lock the outer one holds, +// and the outer one is waiting on the inner one. That deadlocks every write of +// the stored record, not just the caller. Mutate the record you were handed. // // Resolves with the record that was written. function updateState(mutate) { - const turn = updateQueue.then(() => updateStateOnce(mutate)); - // The queue must advance even when a turn rejects, or every update after - // it queues behind a promise that never settles. - updateQueue = turn.catch(() => {}); - return turn; + return withLock("autistmask", () => updateStateOnce(mutate)); } module.exports = { getState, updateState }; diff --git a/src/popup/views/stateRecovery.js b/src/popup/views/stateRecovery.js index f13a09f..0d56f5b 100644 --- a/src/popup/views/stateRecovery.js +++ b/src/popup/views/stateRecovery.js @@ -24,7 +24,11 @@ // than written again locally so that tests/popupElementIds.test.js sees these // lookups and holds every id below against the markup. const { $, VIEWS } = require("./helpers"); -const { storageGet, storageRemove } = require("../../shared/browserApi"); +const { + storageGet, + storageRemove, + withLock, +} = require("../../shared/browserApi"); const { log } = require("../../shared/log"); // Typed in full before anything is erased, in the same spirit as the wallet @@ -137,7 +141,9 @@ async function resetProfile() { return; } try { - await storageRemove("autistmask"); + // Under the lock every write of the stored record takes, so the erase + // cannot land between another write's read and its write and be undone. + await withLock("autistmask", () => storageRemove("autistmask")); } catch (e) { log.errorf("state recovery: reset failed:", e); setFlash("The saved data could not be erased. Nothing was changed."); diff --git a/src/shared/browserApi.js b/src/shared/browserApi.js index 472d88b..81e42de 100644 --- a/src/shared/browserApi.js +++ b/src/shared/browserApi.js @@ -219,6 +219,32 @@ function storageRemove(keys) { return Promise.resolve(storage.remove(keys)); } +/** + * Run `fn` while holding the browser's lock called `name` + * (`navigator.locks`), and settle as `fn` settles. Every page of the extension + * and its background share these locks, so while one of them holds `name` + * another asking for it waits, in the order they asked. The lock is let go + * when `fn` settles, whether it resolved or rejected. + * + * Every write of the stored record holds the lock named after its key, + * "autistmask", from its read to its write: saveState() in + * src/shared/state.js, updateState() in src/background/state.js, and the + * reset in src/popup/views/stateRecovery.js. + * + * @param {string} name + * @param {function(): Promise<*>} fn + * @returns {Promise<*>} what `fn` resolved with. + * @throws rejects, without running `fn`, where `navigator.locks` is absent. + */ +function withLock(name, fn) { + if (typeof navigator === "undefined" || !navigator.locks) { + return Promise.reject( + new Error("navigator.locks is not available in this context"), + ); + } + return navigator.locks.request(name, () => fn()); +} + // `storage.session`, the browser's session storage: what is kept there lasts // until the browser closes. It survives the background worker being stopped // and started again, which module-level variables do not (README, Background @@ -373,4 +399,5 @@ module.exports = { windowsGetLastFocused, windowsRemove, windowsUpdate, + withLock, }; diff --git a/src/shared/state.js b/src/shared/state.js index ecc8fc2..d0c200c 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -35,7 +35,7 @@ const { migrationNeeded, } = require("./stateSchema"); -const { storageGet, storageSet } = require("./browserApi"); +const { storageGet, storageSet, withLock } = require("./browserApi"); const { log } = require("./log"); // The live record the proxy below guards. Everything inside this module reads @@ -458,8 +458,8 @@ function mergeNetworkEndpoints(base, ours, theirs) { // This page's own live `state` is deliberately NOT rehydrated from a field // another page changed — only the record written to storage is merged. // showView() fires saveState() on every navigation without awaiting it, -// which is what makes the queue above necessary in the first place, and a -// save that is slow to come back has no way to tell whether the field it +// which is why saves from one page wait for each other (see saveState()), and +// a save that is slow to come back has no way to tell whether the field it // is about to hand back is still the current answer or has since been // overtaken by something this very page did in the meantime; writing it // into `state` regardless reintroduced exactly the clobber this function @@ -535,24 +535,11 @@ async function saveStateOnce() { baseline = current; } -// showView() calls saveState() on every navigation without awaiting it, so -// two saves from the SAME page can be in flight at once — e.g. a screen -// shown, then immediately replaced before the first save's storageGet() -// round trip has come back. Left concurrent, the first save's turn would -// finish after the second's live-state mutation and then re-hydrate `state` -// from what IT read, stomping the second, later change back to a stale -// value — the same clobber this function exists to prevent, just between -// two saves on one page instead of two pages. Queuing makes every save's -// snapshot-diff-write-rehydrate run start to finish before the next one -// begins, so each one only ever sees the true live state at its turn. -let saveQueue = Promise.resolve(); - // Where a failed save is REPORTED, set once by the context that has a screen // to say it on (src/popup/index.js). // // A save that fails must not fail silently. showView() fires saveState() on -// every navigation without awaiting it, and the queue below has to attach a -// rejection handler to keep advancing — so a failing save was swallowed +// every navigation without awaiting it, so a failing save was swallowed // entirely: no throw, no message, nothing on screen. The wallet kept running // against storage that was rejecting every write, which is the data-loss half // of https://git.eeqj.de/sneak/AutistMask/issues/362. The awaited callers were @@ -583,11 +570,18 @@ function reportSaveFailure(err) { } } +// Each save runs start to finish holding the lock every write of the stored +// record takes (withLock() in src/shared/browserApi.js), so no write by +// another page or the background lands between its read and its write and is +// lost (https://git.eeqj.de/sneak/AutistMask/issues/641). +// +// The same lock orders this page's own saves. showView() calls saveState() on +// every navigation without awaiting it, so two saves from one page can be +// asked for at once; they take the lock in the order they were asked, and each +// compares the page's state as it stands when its turn begins against the +// baseline the save before it left. function saveState() { - const turn = saveQueue.then(saveStateOnce); - // The queue must advance even when a save rejects, or every save after - // it queues behind a promise that never settles. - saveQueue = turn.catch(() => {}); + const turn = withLock("autistmask", saveStateOnce); // Every failed save is reported, whether or not the caller awaited this // one. The returned promise still rejects, so a caller that DOES await // keeps its own error handling. diff --git a/tests/confirmTxAmount.test.js b/tests/confirmTxAmount.test.js index 80b0822..0760cf6 100644 --- a/tests/confirmTxAmount.test.js +++ b/tests/confirmTxAmount.test.js @@ -158,7 +158,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { Interface, diff --git a/tests/confirmTxNetworkSwitch.test.js b/tests/confirmTxNetworkSwitch.test.js index 337849f..1a9a10b 100644 --- a/tests/confirmTxNetworkSwitch.test.js +++ b/tests/confirmTxNetworkSwitch.test.js @@ -189,7 +189,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { BaseWallet, diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 3ec090b..be29953 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -367,11 +367,14 @@ async function leaveTransactionDetail(page) { await openAddressDetail(page); } -// Back out to Home from wherever the previous test finished. +// Back out to Home from wherever the previous test finished, and wait for the +// save that navigation makes, so a test reading the stored stack next reads +// the one Home left. async function goHome(page) { await leaveTransactionDetail(page); await page.click("#btn-address-back"); await visible(page, "#view-main"); + await waitForPersisted(page, "currentView", "main", "back on Home"); } // One field of the popup's state as it was actually persisted, read out of diff --git a/tests/nativeCurrency.test.js b/tests/nativeCurrency.test.js index ae31109..40d036a 100644 --- a/tests/nativeCurrency.test.js +++ b/tests/nativeCurrency.test.js @@ -151,7 +151,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { state } = require("../src/shared/state"); const { NETWORKS } = require("../src/shared/networks"); diff --git a/tests/sendAmountRule.test.js b/tests/sendAmountRule.test.js index 224ff4a..98f43fb 100644 --- a/tests/sendAmountRule.test.js +++ b/tests/sendAmountRule.test.js @@ -97,7 +97,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { state } = require("../src/shared/state"); const send = require("../src/popup/views/send"); diff --git a/tests/sendDisplayFloor.test.js b/tests/sendDisplayFloor.test.js index d7a37d6..e7694c3 100644 --- a/tests/sendDisplayFloor.test.js +++ b/tests/sendDisplayFloor.test.js @@ -151,7 +151,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { refreshBalances } = require("../src/shared/balances"); const { state } = require("../src/shared/state"); diff --git a/tests/sendMax.test.js b/tests/sendMax.test.js index af8943d..ff194ed 100644 --- a/tests/sendMax.test.js +++ b/tests/sendMax.test.js @@ -146,7 +146,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { Transaction, Wallet, formatEther } = require("ethers"); const { refreshBalances } = require("../src/shared/balances"); diff --git a/tests/stateWriteLock.test.js b/tests/stateWriteLock.test.js new file mode 100644 index 0000000..753960b --- /dev/null +++ b/tests/stateWriteLock.test.js @@ -0,0 +1,78 @@ +// A popup save reads the stored record, merges this page's changes into it +// and writes it back. The background's write of a network the user approved +// used to be able to land between that read and that write, and the popup's +// write then put the old network back +// (https://git.eeqj.de/sneak/AutistMask/issues/641). Every write of the record +// now holds one lock from its read to its write. +// +// The popup and the background run here as the real modules over one shared +// storage stub, and share the lock the way the extension's pages and its +// background share the browser's (tests/support/navigatorLocks.js). + +const { makeStorageStub } = require("./support/storageStub"); + +function loadContexts(storage) { + jest.resetModules(); + globalThis.chrome = { storage: { local: storage.local } }; + return { + popup: require("../src/shared/state"), + background: require("../src/background/state"), + applyChainSwitchFields: require("../src/shared/chainSwitchFields") + .applyChainSwitchFields, + }; +} + +// Lets every pending promise callback run, and then some. +function nextMacrotask() { + return new Promise((resolve) => setImmediate(resolve)); +} + +afterEach(() => { + delete globalThis.chrome; + jest.restoreAllMocks(); +}); + +test("a network the background switches to during a popup save stays stored", async () => { + const storage = makeStorageStub({ autistmask: { networkId: "sepolia" } }); + const { popup, background, applyChainSwitchFields } = loadContexts(storage); + await popup.loadState(); + + // The popup navigates, so its save has a change of its own to write. + popup.state.currentView = "settings"; + + // The popup's save has read the record and is about to write it back when + // the user's approval of a site's switch to mainnet reaches the + // background. Without the lock the background's whole update runs before + // the next macrotask; with it, the update waits for the popup's write. + let backgroundSwitch; + const realSet = storage.local.set.getMockImplementation(); + storage.local.set.mockImplementationOnce(async (items) => { + backgroundSwitch = background.updateState((fresh) => + applyChainSwitchFields(fresh, "mainnet"), + ); + await nextMacrotask(); + return realSet(items); + }); + + await popup.saveState(); + await backgroundSwitch; + + const stored = storage.read("autistmask"); + expect(stored.networkId).toBe("mainnet"); + expect(stored.currentView).toBe("settings"); +}); + +test("a save that fails holding the lock lets it go", async () => { + jest.spyOn(console, "error").mockImplementation(() => {}); + const storage = makeStorageStub({ autistmask: { networkId: "sepolia" } }); + const { popup, background, applyChainSwitchFields } = loadContexts(storage); + await popup.loadState(); + + storage.local.set.mockRejectedValueOnce(new Error("quota exceeded")); + await expect(popup.saveState()).rejects.toThrow("quota exceeded"); + + await background.updateState((fresh) => + applyChainSwitchFields(fresh, "mainnet"), + ); + expect(storage.read("autistmask").networkId).toBe("mainnet"); +}); diff --git a/tests/support/navigatorLocks.js b/tests/support/navigatorLocks.js new file mode 100644 index 0000000..f3e8d3a --- /dev/null +++ b/tests/support/navigatorLocks.js @@ -0,0 +1,26 @@ +// `navigator.locks` for the unit tests, which run on node, where there is none. +// +// One lock per name: granted in the order it was asked for, and let go when +// the callback settles, whether it resolved or rejected. That is what the +// browser's own does for the one call the extension makes, +// `navigator.locks.request(name, callback)`. Every module a test file loads +// shares it, as every page of the extension and its background share the +// browser's. +// +// Run before each test file by `setupFiles` in package.json. + +// For each name, a promise that settles once the last request for it has +// let the lock go. +const released = new Map(); + +function request(name, callback) { + const previous = released.get(name) || Promise.resolve(); + const result = previous.then(() => callback({ name, mode: "exclusive" })); + released.set( + name, + result.catch(() => {}), + ); + return result; +} + +globalThis.navigator = { locks: { request } }; diff --git a/tests/unknownScaleSend.test.js b/tests/unknownScaleSend.test.js index c65a9b3..e1509b5 100644 --- a/tests/unknownScaleSend.test.js +++ b/tests/unknownScaleSend.test.js @@ -120,7 +120,7 @@ global.document = { body: { prepend() {}, appendChild() {} }, addEventListener() {}, }; -global.navigator = { clipboard: { writeText() {} } }; +global.navigator.clipboard = { writeText() {} }; const { AbiCoder, parseUnits } = require("ethers"); const { fetchTokenBalances } = require("../src/shared/balances");