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, each 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) from the read to
the write, through withLock() in src/shared/browserApi.js, and the recovery
screen's reset takes it too. A page in a Firefox private window gets locks of
its own and is not covered. 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

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
-// 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
@@ -529,35 +529,21 @@ async function saveStateOnce() {
     rawState.hasWallet = rawState.wallets.length > 0;
 
     // What this save compared and wrote, not the page's state now: a change
-    // made during the save must still differ from the baseline, or the save
-    // queued after it finds nothing to store
+    // made during the save must still differ from the baseline, or the next
+    // save finds nothing to store
     // (https://git.eeqj.de/sneak/AutistMask/issues/448).
     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
-// 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
-// no better off: `await saveState()` inside an unguarded event handler surfaces
-// in the console and nowhere the user looks.
+// every navigation without awaiting it, and `await saveState()` inside an
+// unguarded event handler is no better: either way, without this, a failure
+// would reach only the console, where the user never looks, and the wallet
+// would keep running against storage that rejects every write. That is the
+// data-loss half of https://git.eeqj.de/sneak/AutistMask/issues/362.
 //
 // 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);
-    // 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.