README.md

@@ -2092,8 +2092,11 @@ view would leave a wallet one click from deletion.
       none, and the tokens tracked on that network, and sends `chainChanged`
       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. Choosing the network already active changes nothing and
-      sends nothing
+      network only. Switching also clears every address's balances. A balance
+      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/background/index.js

@@ -1379,6 +1379,13 @@ const RECENT_BALANCE_REFRESH_MS = Math.floor(BALANCE_REFRESH_PERIOD_MS / 2);
 // balances that came back — by address, onto whatever storage holds NOW.
 // Applying by address rather than writing the array back is what keeps a
 // wallet or address added, renamed or deleted during the round trip.
+//
+// Nothing is applied when the network stored NOW is not the one the refresh
+// ran on. A network switch during the round trip cleared every address's
+// balances (applyChainSwitchFields()), and the previous network's must not come
+// back over them (https://git.eeqj.de/sneak/AutistMask/issues/577).
+// lastBalanceRefresh is left as the switch set it, so the next tick refreshes
+// the new network.
 async function backgroundRefresh() {
     const s = await getState();
     const now = Date.now();
@@ -1402,6 +1409,7 @@ async function backgroundRefresh() {
     }
 
     await updateState((fresh) => {
+        if (fresh.networkId !== s.networkId) return;
         for (const wallet of fresh.wallets) {
             for (const addr of wallet.addresses || []) {
                 const got = refreshed.get(String(addr.address).toLowerCase());

src/popup/index.js

@@ -68,23 +68,54 @@ 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. 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;
     try {
+        const networkId = state.networkId;
+        const wallets = structuredClone(state.wallets);
         await Promise.all([
             refreshPrices(),
             refreshBalances(
-                state.wallets,
+                wallets,
                 state.rpcUrl,
                 state.blockscoutUrl,
                 state.trackedTokens,
-                state.networkId,
+                networkId,
                 pageClosed.signal,
             ),
         ]);
-        state.lastBalanceRefresh = Date.now();
-        await saveState();
+        const refreshed = new Map();
+        for (const wallet of wallets) {
+            for (const addr of wallet.addresses) {
+                refreshed.set(addr.address, addr);
+            }
+        }
+        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();
+            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) === false) 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) {
@@ -579,10 +581,15 @@ function reportSaveFailure(err) {
 // 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 = withLock("autistmask", saveStateOnce);
+// compares the page's state, as it stands once the stored record has been read
+// and `beforeWrite` has run, against the baseline the save before it left.
+//
+// `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. When it returns false, nothing is
+// written; any other return lets the save go ahead.
+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/backgroundStateIsolation.test.js

@@ -19,7 +19,10 @@
 //     that followed persisted the pre-refresh values — while still stamping
 //     lastBalanceRefresh, suppressing the redo.
 //
-// Both use the real persistence path over a cloning storage stub. Nothing here
+// A network switch landing inside a balance refresh's round trip is covered at
+// the end (https://git.eeqj.de/sneak/AutistMask/issues/577).
+//
+// All use the real persistence path over a cloning storage stub. Nothing here
 // asserts the absence of a loadState() call; each asserts the OUTCOME, so it
 // holds against any implementation that gets the outcome right.
 
@@ -395,3 +398,48 @@ describe("a balance refresh under another handler's state read", () => {
         expect(after.wallets[0].addresses[0].balance).toBe(REFRESHED_BALANCE);
     });
 });
+
+describe("a balance refresh under a network switch", () => {
+    // https://git.eeqj.de/sneak/AutistMask/issues/577. The switch clears every
+    // address's balances, and a refresh that started on the previous network
+    // must not write that network's back over them when it returns. The same
+    // refresh in the popup is in tests/balanceRefreshNetworkSwitch.test.js.
+    test("a refresh that returns after a switch writes nothing, and the next one runs on the new network", async () => {
+        const roundTrip = deferred();
+        const reachedNetwork = deferred();
+        const ranOn = [];
+
+        const bg = loadWorker("sepolia", {
+            refreshBalances: async (wallets, rpcUrl, explorer, tokens, id) => {
+                ranOn.push(id);
+                reachedNetwork.resolve();
+                await roundTrip.promise;
+                wallets[0].addresses[0].balance = REFRESHED_BALANCE;
+            },
+        });
+
+        const refresh = bg.fireBalanceAlarm();
+        await reachedNetwork.promise;
+
+        // The user switches to Ethereum Mainnet in Settings while the round
+        // trip is out.
+        const during = bg.persisted();
+        applyChainSwitchFields(during, MAINNET.id);
+        global.chrome.storage.write("autistmask", during);
+
+        roundTrip.resolve();
+        await refresh;
+
+        const after = bg.persisted();
+        expect(after.networkId).toBe("mainnet");
+        expect(after.wallets[0].addresses[0].balance).toBe("0");
+        expect(after.lastBalanceRefresh).toBe(0);
+
+        await bg.fireBalanceAlarm();
+
+        expect(ranOn).toEqual(["sepolia", "mainnet"]);
+        expect(bg.persisted().wallets[0].addresses[0].balance).toBe(
+            REFRESHED_BALANCE,
+        );
+    });
+});

tests/balanceRefreshNetworkSwitch.test.js

@@ -0,0 +1,160 @@
+// A balance refresh the popup started on one network, and that returns after
+// 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,
+    cleanupPopup,
+    unversionedValidProfile,
+    TOKEN_ADDRESS,
+} = require("./support/popupBoot");
+const { applyChainSwitchFields } = require("../src/shared/chainSwitchFields");
+
+afterEach(() => {
+    cleanupPopup();
+});
+
+// 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");
+    let answer;
+    refreshBalances.mockImplementationOnce(async (wallets) => {
+        await new Promise((resolve) => {
+            answer = resolve;
+        });
+        wallets[0].addresses[0].balance = "7.0";
+        wallets[0].addresses[0].tokenBalances = [
+            {
+                address: TOKEN_ADDRESS,
+                name: "Test Token",
+                symbol: "TST",
+                decimals: 18,
+                balance: "5.0",
+                holders: null,
+            },
+        ];
+    });
+    const ticking = page.tick();
+    await page.settle();
+    return {
+        finish: async () => {
+            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();
+}
+
+// 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"]);
+    expect(page.text("total-value")).toBe("0.0000 SepoliaETH");
+
+    await page.tick();
+
+    expect(refreshBalances.mock.lastCall[4]).toBe("sepolia");
+    expect(page.storage.read("autistmask").lastBalanceRefresh).toBeGreaterThan(
+        0,
+    );
+});
+
+// The stored record still holds Ethereum Mainnet while the refresh's save reads
+// it, so only this page's own network shows the switch.
+test("a refresh whose save is reading the stored record when the user switches in Settings writes nothing", async () => {
+    const page = await bootPopup(unversionedValidProfile());
+    expect(page.pageErrors).toEqual([]);
+    const { state } = require("../src/shared/state");
+
+    const refresh = await holdRefresh(page);
+    await page.click("btn-settings");
+
+    // The next read of the stored record, the refresh's save's, is held.
+    const get = page.storage.local.get;
+    const read = get.getMockImplementation();
+    let releaseRead;
+    get.mockImplementationOnce(async (key) => {
+        await new Promise((resolve) => {
+            releaseRead = resolve;
+        });
+        return read(key);
+    });
+    const finishing = refresh.finish();
+    await page.settle();
+
+    // The user picks Sepolia meanwhile; the switch's own save waits for the
+    // refresh's save to let the lock go.
+    const select = page.node("settings-network");
+    select.value = "sepolia";
+    const switching = Promise.all(select.listeners.change.map((fn) => fn()));
+    await page.settle();
+
+    releaseRead();
+    await finishing;
+    await switching;
+    await page.settle();
+
+    expect(state.networkId).toBe("sepolia");
+    expect(state.wallets[0].addresses[0].balance).toBe("0");
+    expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]);
+    expectNothingStored(page);
+});
+
+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);
+});