fix: a popup save no longer undoes a network switch the background saved during it (closes #641)

The popup's saveState() and the background's updateState() each read the
stored record, change it and write it back, and each was ordered only within
its own context, so a write by one could land between the other's read and
write and be lost. Both now hold the browser's lock (navigator.locks, shared
by every page and the background except a page in a Firefox private window)
from the read to the write, through withLock() in src/shared/browserApi.js;
the recovery screen's reset takes it too. The lock also orders a page's own
saves, so both per-context queues are gone. The unit tests get a small lock
from a Jest setup file.

Model: opus-5-5

README.md

@@ -1389,6 +1389,19 @@ 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`), taken through
+`withLock()` in `src/shared/browserApi.js`. Every page of the extension and its
+background share it, except a page in a Firefox private window, which Firefox
+gives locks of its own (see https://git.eeqj.de/sneak/AutistMask/issues/646).
+The popup's `saveState()`, the background's `updateState()` and the
+StateRecovery reset all take it, so among the pages and the background that
+share it, 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

package.json

@@ -13,6 +13,11 @@
         "fmt": "prettier --write .",
         "fmt-check": "prettier --check ."
     },
+    "jest": {
+        "setupFiles": [
+            "<rootDir>/tests/support/navigatorLocks.js"
+        ]
+    },
     "devDependencies": {
         "@eslint/js": "10.0.1",
         "@tailwindcss/cli": "4.3.1",

src/background/state.js

@@ -16,17 +16,20 @@
 //                     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
+//                     back — all holding the lock every write of the stored
-//                     never interleave, and the read is one storage round trip
+//                     record takes, so no other write lands in between, by
-//                     ahead of the write rather than a page lifetime ahead of
+//                     the background or by any page except a page in a
-//                     it (which is what made the popup's saveState() need a
+//                     Firefox private window (README, "Stored state and its
+//                     version"). 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 +54,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 +68,22 @@ 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
+// place; it may be async, but it must not do anything slow. The write is the
-// between the read and the write is the window in which another context's
+// WHOLE record, which is safe only because the lock (withLock() in
-// write is lost, and keeping it to one storage round trip is what makes a
+// src/shared/browserApi.js) is held from the read to the write: two
-// whole-record write safe here. Concretely: the write is the WHOLE record, so
+// background turns, or a turn and a page's save, never interleave
-// a popup write that lands inside that window is reverted, in every field, by
+// (https://git.eeqj.de/sneak/AutistMask/issues/641). Every page's save waits
-// the record this turn read before it. That is accepted because the window is
+// while it is held, except a page in a Firefox private window, which Firefox
-// one round trip long and the popup is not writing while the worker is;
+// gives locks of its own (README, "Stored state and its version").
-// widening it is what would make it a real hazard.
 //
 // `mutate` must also not call updateState() itself, directly or through
-// anything it awaits: the queue is strictly serial, so the inner turn waits on
+// anything it awaits: the inner turn waits for the lock the outer one holds,
-// the outer one, which is waiting on the inner one. That deadlocks the whole
+// and the outer one is waiting on the inner one. That deadlocks every write of
-// background, not just the caller. Mutate the record you were handed.
+// 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));
+    return withLock("autistmask", () => 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;
 }
 
 module.exports = { getState, updateState };

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.");

src/shared/browserApi.js

@@ -31,6 +31,11 @@
 // Everything is resolved on use rather than captured at module load. The MV3
 // service worker is torn down and re-evaluated repeatedly, and the unit
 // suite installs its stubs on `global.chrome` around a require().
+//
+// The module also holds withLock(), a wrapper around the browser's
+// `navigator.locks`, which is in neither namespace. It sits beside
+// storageGet() and storageSet() because every write of the stored record
+// takes it.
 
 // The extension API namespace, preferring `browser.*` where it exists.
 //
@@ -219,6 +224,33 @@ 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, except a page in a Firefox private
+ * window (README, "Stored state and its version"), 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 +405,5 @@ module.exports = {
     windowsGetLastFocused,
     windowsRemove,
     windowsUpdate,
+    withLock,
 };

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
+// which is why saves from one page wait for each other (see saveState()), and
-// save that is slow to come back has no way to tell whether the field it
+// 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,29 +535,15 @@ 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
+// every navigation without awaiting it, and `await saveState()` inside an
-// rejection handler to keep advancing — so a failing save was swallowed
+// unguarded event handler is no better: either way, without this, a failure
-// entirely: no throw, no message, nothing on screen. The wallet kept running
+// would reach only the console, where the user never looks, and the wallet
-// against storage that was rejecting every write, which is the data-loss half
+// would keep running against storage that rejects every write. That is the
-// of https://git.eeqj.de/sneak/AutistMask/issues/362. The awaited callers were
+// data-loss half of https://git.eeqj.de/sneak/AutistMask/issues/362.
-// no better off: `await saveState()` inside an unguarded event handler surfaces
-// in the console and nowhere the user looks.
 //
 // This is the "tell the user" half; the other half is the floor in
 // normalizePersisted(), which stops the malformed-record cause from arising in
@@ -583,11 +569,20 @@ 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). A page in a Firefox
+// private window is the exception: Firefox gives it locks of its own (README,
+// "Stored state and its version").
+//
+// 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);
+    const turn = withLock("autistmask", 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(() => {});
     // 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.

tests/confirmTxAmount.test.js

@@ -158,7 +158,7 @@ global.document = {
     body: { prepend() {}, appendChild() {} },
     addEventListener() {},
 };
-global.navigator = { clipboard: { writeText() {} } };
+global.navigator.clipboard = { writeText() {} };
 
 const {
     Interface,

tests/confirmTxNetworkSwitch.test.js

@@ -189,7 +189,7 @@ global.document = {
     body: { prepend() {}, appendChild() {} },
     addEventListener() {},
 };
-global.navigator = { clipboard: { writeText() {} } };
+global.navigator.clipboard = { writeText() {} };
 
 const {
     BaseWallet,

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

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");

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");

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");

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");

tests/stateWriteLock.test.js

@@ -0,0 +1,95 @@
+// 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("after a save that failed, the next save still keeps a network the background switches to during it", async () => {
+    jest.spyOn(console, "error").mockImplementation(() => {});
+    const storage = makeStorageStub({ autistmask: { networkId: "sepolia" } });
+    const { popup, background, applyChainSwitchFields } = loadContexts(storage);
+    await popup.loadState();
+    popup.state.currentView = "settings";
+
+    // The first save's write is refused, so it gives the lock back having
+    // failed.
+    storage.local.set.mockRejectedValueOnce(new Error("quota exceeded"));
+    await expect(popup.saveState()).rejects.toThrow("quota exceeded");
+
+    // The next save is between its read and its write when the background
+    // switches to mainnet, as in the test above.
+    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");
+});

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 } };

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");