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
+      refresh, by the popup or the background, stores what it fetched only if
-      stores nothing when it returns, so the previous network's balances never
+      the stored network is still the one it fetched on when it returns, so the
-      reach the new one; the next refresh fetches the new network's. Choosing
+      previous network's balances never reach the new one. Choosing the network
-      the network already active changes nothing and sends nothing
+      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 refresh writes into a copy of the wallets. Inside the save, under the
-// the address records in `state` only if the network it ran on is still the
+// lock, its results are copied onto the address records in `state` only if the
-// active one: a network switch while it ran cleared every address's balances
+// network it ran on is still the active one, on this page and in the stored
-// (applyChainSwitchFields() in src/shared/chainSwitchFields.js), and the
+// record; the background's switch, after the user approves a site's request,
-// previous network's must not come back over them
+// changes only the stored record. A switch while the refresh ran cleared every
-// (https://git.eeqj.de/sneak/AutistMask/issues/577). A refresh that returns
+// address's balances (applyChainSwitchFields() in
-// after a switch is dropped and records nothing, so the next tick refreshes the
+// src/shared/chainSwitchFields.js), and the previous network's must not come
-// new network. Results are copied by address, so an address added or removed
+// back over them (https://git.eeqj.de/sneak/AutistMask/issues/577). A dropped
-// meanwhile is left as it is.
+// 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) {
+        await saveState((stored) => {
-            for (const addr of wallet.addresses) {
+            if (state.networkId !== networkId) return false;
-                const got = refreshed.get(addr.address);
+            if (stored.networkId !== networkId) return false;
-                if (!got) continue;
+            for (const wallet of state.wallets) {
-                for (const key of ["balance", "ensName", "tokenBalances"]) {
+                for (const addr of wallet.addresses) {
-                    if (got[key] !== undefined) addr[key] = got[key];
+                    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();
-        state.lastBalanceRefresh = Date.now();
+            return true;
-        await saveState();
+        });
         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() {
+async function saveStateOnce(beforeWrite) {
-    // 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());
     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
+// the network was switched to another, writes nothing: the switch cleared every
-// every address's balances, and the previous network's must not come back over
+// address's balances, and the previous network's must not come back over them
-// them (https://git.eeqj.de/sneak/AutistMask/issues/577). Driven through the
+// (https://git.eeqj.de/sneak/AutistMask/issues/577). The switch is made either
-// real popup over a stored profile on Ethereum Mainnet. The same refresh in the
+// by the user in this popup's Settings, or by the background after the user
-// background is in tests/backgroundStateIsolation.test.js.
+// 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.
+// Starts the ten-second refresh and holds it open. The balance stand-in writes
-async function chooseNetwork(page, networkId) {
+// Ethereum Mainnet's balances in place, on the records handed in, as the real
-    const select = page.node("settings-network");
+// refreshBalances() does, once the returned function is called. Resolves with
-    select.value = networkId;
+// that function and the refresh still running.
-    for (const fn of select.listeners.change) await fn();
+async function holdRefresh(page) {
-    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([]);
     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");
+// Picks `networkId` in the Settings network selector, as the user does.
-    await chooseNetwork(page, "sepolia");
+async function chooseNetwork(page, networkId) {
-    answer();
+    const select = page.node("settings-network");
-    await ticking;
+    select.value = networkId;
+    for (const fn of select.listeners.change) await fn();
+    await page.settle();
+}
 
-    expect(state.wallets[0].addresses[0].balance).toBe("0");
+// The stored record holds none of Ethereum Mainnet's balances, and no refresh
-    expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]);
+// 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);
+});