fix: a balance refresh that returns after a network switch writes nothing (closes #577)

The popup's doRefreshAndRender() and the background's backgroundRefresh()
each note the network a refresh starts on and write its balances only if
that network is still the active one when the requests return. Otherwise
nothing is written and lastBalanceRefresh stays as the switch left it. The
background checks inside updateState(). The popup refreshes a copy of its
wallets and copies the results onto `state` from inside its save, under the
lock, only if both its own network and the stored record's still match, so
a switch the background makes is caught too. saveState() takes an optional
function, run there on the stored record before the page's state is
compared, that may change `state` or cancel the write.

Model: opus-5-5

README.md

@@ -2081,10 +2081,10 @@ view would leave a wallet one click from deletion.
       with the new chain id to every open tab. A token is tracked only on the
       network it was added on, because a contract address names a token on one
       network only. Switching also clears every address's balances. A balance
-      refresh, by the popup or the background, that started before the switch
-      stores nothing when it returns, so the previous network's balances never
-      reach the new one; the next refresh fetches the new network's. Choosing
-      the network already active changes nothing and sends nothing
+      refresh, by the popup or the background, stores what it fetched only if
+      the stored network is still the one it fetched on when it returns, so the
+      previous network's balances never reach the new one. Choosing the network
+      already active changes nothing and sends nothing
     - Ethereum RPC: endpoint URL input + "Save" button (validated against
       `eth_chainId` before being saved)
     - Blockscout API: endpoint URL input + "Save" button (validated against

src/popup/index.js

@@ -68,15 +68,16 @@ let refreshInFlight = false;
 // the recovery screen: there is no profile left to refresh.
 let refreshTimer = null;
 
-// The refresh writes into a copy of the wallets. Its results are copied onto
-// the address records in `state` only if the network it ran on is still the
-// active one: a network switch while it ran cleared every address's balances
-// (applyChainSwitchFields() in src/shared/chainSwitchFields.js), and the
-// previous network's must not come back over them
-// (https://git.eeqj.de/sneak/AutistMask/issues/577). A refresh that returns
-// after a switch is dropped and records nothing, so the next tick refreshes the
-// new network. Results are copied by address, so an address added or removed
-// meanwhile is left as it is.
+// The refresh writes into a copy of the wallets. Inside the save, under the
+// lock, its results are copied onto the address records in `state` only if the
+// network it ran on is still the active one, on this page and in the stored
+// record; the background's switch, after the user approves a site's request,
+// changes only the stored record. A switch while the refresh ran cleared every
+// address's balances (applyChainSwitchFields() in
+// src/shared/chainSwitchFields.js), and the previous network's must not come
+// back over them (https://git.eeqj.de/sneak/AutistMask/issues/577). A dropped
+// refresh is not recorded as done. Results are copied by address, so an address
+// added or removed meanwhile is left as it is.
 async function doRefreshAndRender() {
     if (refreshInFlight) return;
     refreshInFlight = true;
@@ -94,24 +95,27 @@ async function doRefreshAndRender() {
                 pageClosed.signal,
             ),
         ]);
-        if (state.networkId !== networkId) return;
         const refreshed = new Map();
         for (const wallet of wallets) {
             for (const addr of wallet.addresses) {
                 refreshed.set(addr.address, addr);
             }
         }
-        for (const wallet of state.wallets) {
-            for (const addr of wallet.addresses) {
-                const got = refreshed.get(addr.address);
-                if (!got) continue;
-                for (const key of ["balance", "ensName", "tokenBalances"]) {
-                    if (got[key] !== undefined) addr[key] = got[key];
+        await saveState((stored) => {
+            if (state.networkId !== networkId) return false;
+            if (stored.networkId !== networkId) return false;
+            for (const wallet of state.wallets) {
+                for (const addr of wallet.addresses) {
+                    const got = refreshed.get(addr.address);
+                    if (!got) continue;
+                    for (const key of ["balance", "ensName", "tokenBalances"]) {
+                        if (got[key] !== undefined) addr[key] = got[key];
+                    }
                 }
             }
-        }
-        state.lastBalanceRefresh = Date.now();
-        await saveState();
+            state.lastBalanceRefresh = Date.now();
+            return true;
+        });
         renderWalletList();
     } catch (e) {
         // Every call site fires this and walks away — the boot below, the ten

src/shared/state.js

@@ -467,11 +467,7 @@ function mergeNetworkEndpoints(base, ours, theirs) {
 // (caught by tests/txStatus.test.js). A page's live picture of a field it
 // does not own goes on being whatever its last loadState() saw, same as
 // before this fix; only the persisted record is guaranteed current.
-async function saveStateOnce() {
-    // A copy, so what this save compares and writes is the page's state as it
-    // stood when the save began. A change made while it waits on storage is
-    // left for the next save, which compares against this copy.
-    const current = structuredClone(snapshotPersisted());
+async function saveStateOnce(beforeWrite) {
     const result = await storageGet("autistmask");
     // The record in storage right now is about to be merged into and written
     // back, so it is validated exactly like a load validates it. Without this,
@@ -485,6 +481,12 @@ async function saveStateOnce() {
     // come from storage in its loaded (self-healed) shape. See
     // normalizePersisted() in persistedState.js.
     const fresh = normalizePersisted(result.autistmask);
+    if (beforeWrite && !beforeWrite(fresh)) return;
+
+    // A copy, so what this save compares and writes is the page's state as it
+    // stood once the stored record was read. A change made while it waits on
+    // the write is left for the next save, which compares against this copy.
+    const current = structuredClone(snapshotPersisted());
 
     const merged = { ...fresh };
     for (const key of PERSISTED_FIELDS) {
@@ -581,8 +583,13 @@ function reportSaveFailure(err) {
 // 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 = withLock("autistmask", saveStateOnce);
+//
+// `beforeWrite`, when given, is called inside the lock with the stored record
+// as just read, before this page's state is compared against it. It may change
+// `state`, and this save writes that change. It returns whether the save goes
+// ahead: when it returns false, nothing is written.
+function saveState(beforeWrite) {
+    const turn = withLock("autistmask", () => saveStateOnce(beforeWrite));
     // 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/balanceRefreshNetworkSwitch.test.js

@@ -1,9 +1,11 @@
 // A balance refresh the popup started on one network, and that returns after
-// the user switched to another in Settings, writes nothing: the switch cleared
-// every address's balances, and the previous network's must not come back over
-// them (https://git.eeqj.de/sneak/AutistMask/issues/577). Driven through the
-// real popup over a stored profile on Ethereum Mainnet. The same refresh in the
-// background is in tests/backgroundStateIsolation.test.js.
+// the network was switched to another, writes nothing: the switch cleared every
+// address's balances, and the previous network's must not come back over them
+// (https://git.eeqj.de/sneak/AutistMask/issues/577). The switch is made either
+// by the user in this popup's Settings, or by the background after the user
+// approved a site's request, which changes only the stored record. Driven
+// through the real popup over a stored profile on Ethereum Mainnet. The same
+// refresh in the background is in tests/backgroundStateIsolation.test.js.
 
 const {
     bootPopup,
@@ -11,28 +13,18 @@ const {
     unversionedValidProfile,
     TOKEN_ADDRESS,
 } = require("./support/popupBoot");
+const { applyChainSwitchFields } = require("../src/shared/chainSwitchFields");
 
 afterEach(() => {
     cleanupPopup();
 });
 
-// Picks `networkId` in the Settings network selector, as the user does.
-async function chooseNetwork(page, networkId) {
-    const select = page.node("settings-network");
-    select.value = networkId;
-    for (const fn of select.listeners.change) await fn();
-    await page.settle();
-}
-
-test("a refresh that returns after a switch writes nothing, and the next one runs on the new network", async () => {
-    const page = await bootPopup(unversionedValidProfile());
-    expect(page.pageErrors).toEqual([]);
+// Starts the ten-second refresh and holds it open. The balance stand-in writes
+// Ethereum Mainnet's balances in place, on the records handed in, as the real
+// refreshBalances() does, once the returned function is called. Resolves with
+// that function and the refresh still running.
+async function holdRefresh(page) {
     const { refreshBalances } = require("../src/shared/balances");
-    const { state } = require("../src/shared/state");
-
-    // The ten-second refresh, held open until answer() is called. It writes
-    // Ethereum Mainnet's balances in place, on the records handed in, as the
-    // real refreshBalances() does.
     let answer;
     refreshBalances.mockImplementationOnce(async (wallets) => {
         await new Promise((resolve) => {
@@ -52,19 +44,46 @@ test("a refresh that returns after a switch writes nothing, and the next one run
     });
     const ticking = page.tick();
     await page.settle();
+    return {
+        finish: async () => {
+            answer();
+            await ticking;
+        },
+    };
+}
 
-    await page.click("btn-settings");
-    await chooseNetwork(page, "sepolia");
-    answer();
-    await ticking;
+// Picks `networkId` in the Settings network selector, as the user does.
+async function chooseNetwork(page, networkId) {
+    const select = page.node("settings-network");
+    select.value = networkId;
+    for (const fn of select.listeners.change) await fn();
+    await page.settle();
+}
 
-    expect(state.wallets[0].addresses[0].balance).toBe("0");
-    expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]);
+// The stored record holds none of Ethereum Mainnet's balances, and no refresh
+// recorded as done.
+function expectNothingStored(page) {
     const stored = page.storage.read("autistmask");
     expect(stored.networkId).toBe("sepolia");
     expect(stored.wallets[0].addresses[0].balance).toBe("0");
     expect(stored.wallets[0].addresses[0].tokenBalances).toEqual([]);
     expect(stored.lastBalanceRefresh).toBe(0);
+}
+
+test("a refresh that returns after a switch in Settings writes nothing, and the next one runs on the new network", async () => {
+    const page = await bootPopup(unversionedValidProfile());
+    expect(page.pageErrors).toEqual([]);
+    const { refreshBalances } = require("../src/shared/balances");
+    const { state } = require("../src/shared/state");
+
+    const refresh = await holdRefresh(page);
+    await page.click("btn-settings");
+    await chooseNetwork(page, "sepolia");
+    await refresh.finish();
+
+    expect(state.wallets[0].addresses[0].balance).toBe("0");
+    expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]);
+    expectNothingStored(page);
 
     await page.click("btn-settings-back");
     expect(page.visibleViews()).toEqual(["main"]);
@@ -77,3 +96,24 @@ test("a refresh that returns after a switch writes nothing, and the next one run
         0,
     );
 });
+
+test("a refresh that returns after the background switched the network writes nothing", async () => {
+    const page = await bootPopup(unversionedValidProfile());
+    expect(page.pageErrors).toEqual([]);
+    const { saveState } = require("../src/shared/state");
+
+    const refresh = await holdRefresh(page);
+    // The background writes the switch to Sepolia, as its updateState() does
+    // once the user approves a site's request. This page is not told.
+    const record = page.storage.read("autistmask");
+    applyChainSwitchFields(record, "sepolia");
+    page.storage.write("autistmask", record);
+    await refresh.finish();
+
+    expectNothingStored(page);
+
+    // Nor does a later save by this page, such as every navigation makes.
+    await saveState();
+
+    expectNothingStored(page);
+});