diff --git a/TODO.md b/TODO.md index 3f11693..4311a3c 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,21 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-06: Reloading or closing the popup mid-refresh no longer logs the + requests that cancels as failures + ([#218](https://git.eeqj.de/sneak/AutistMask/issues/218)). Chrome cancels a + closing page's open requests just after `pagehide`, and in the page a + cancelled `fetch()` fails with the same "Failed to fetch" as a server that + cannot be reached, so the home screen's transaction list and the balance + refresh logged an error for each, and the e2e suite failed on them. The popup + now aborts an `AbortController` on `pagehide`, and those two check its signal + before reporting a failure. New e2e tests reload the popup with Blockscout + held and require nothing logged, and fail the transaction list for real and + require the failure reported; `tests/balanceRefreshCancelled.test.js` covers + the balance refresh. The address and token screens and the address scan after + a new wallet still log a cancelled request + ([#475](https://git.eeqj.de/sneak/AutistMask/issues/475)). + - 2026-10-05: `make build` no longer prints the Node `DEP0205` `module.register()` deprecation warning ([#355](https://git.eeqj.de/sneak/AutistMask/issues/355)). The call came from diff --git a/src/popup/index.js b/src/popup/index.js index bcceac5..21da8e4 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -48,6 +48,14 @@ function renderWalletList() { home.render(ctx); } +// Aborted when the popup page goes away, closed or reloaded. Chrome then +// cancels the requests the page still has open, and a cancelled fetch() fails +// with the same "Failed to fetch" as a server that cannot be reached. pagehide +// fires first, so code that reports a failed request checks this and stays +// silent about one the page's own closing cancelled. +const pageClosed = new AbortController(); +window.addEventListener("pagehide", () => pageClosed.abort()); + let refreshInFlight = false; // The ten-second refresh init() starts, stopped when the popup moves to the @@ -66,6 +74,7 @@ async function doRefreshAndRender() { state.blockscoutUrl, state.trackedTokens, state.networkId, + pageClosed.signal, ), ]); state.lastBalanceRefresh = Date.now(); @@ -87,6 +96,7 @@ async function doRefreshAndRender() { const ctx = { renderWalletList, doRefreshAndRender, + pageClosed: pageClosed.signal, showAddWalletView: () => { pushCurrentView(); addWallet.show(); diff --git a/src/popup/views/home.js b/src/popup/views/home.js index b30c0fd..b77d106 100644 --- a/src/popup/views/home.js +++ b/src/popup/views/home.js @@ -225,6 +225,9 @@ async function loadHomeTxs(ctx) { homeTxs = merged.slice(0, 25); renderHomeTxList(ctx); } catch (e) { + // Cancelled by the popup closing, not failed: see pageClosed in + // src/popup/index.js. + if (ctx.pageClosed.aborted) return; log.errorf("loadHomeTxs failed:", e.message); const list = $("home-tx-list"); if (list) { diff --git a/src/shared/balances.js b/src/shared/balances.js index 81a8d09..eabbed2 100644 --- a/src/shared/balances.js +++ b/src/shared/balances.js @@ -87,7 +87,15 @@ function rawUnits(value) { // way `holders` already is. Absence is never filled in here: this is the // upstream of every screen that displays a token amount, so a value invented // at this point is indistinguishable from a real one everywhere below it. -async function fetchTokenBalances(address, blockscoutUrl, trackedTokens) { +// +// `signal`, passed by the popup, is aborted when the popup closes; a request +// that fails after that was cancelled by the closing and is not logged. +async function fetchTokenBalances( + address, + blockscoutUrl, + trackedTokens, + signal, +) { try { const resp = await debugFetch( blockscoutUrl + "/addresses/" + address + "/token-balances", @@ -190,18 +198,22 @@ async function fetchTokenBalances(address, blockscoutUrl, trackedTokens) { } return balances; } catch (e) { - log.errorf("fetchTokenBalances failed:", e.message); + if (!signal?.aborted) { + log.errorf("fetchTokenBalances failed:", e.message); + } return null; } } // Fetch ETH balances, ENS names, and ERC-20 token balances for all addresses. +// `signal` is as for fetchTokenBalances(). async function refreshBalances( wallets, rpcUrl, blockscoutUrl, trackedTokens, networkId, + signal, ) { log.debugf("refreshBalances start, rpc:", urlOrigin(rpcUrl)); const provider = getProvider(rpcUrl, networkId); @@ -220,6 +232,7 @@ async function refreshBalances( log.debugf("ETH balance", addr.address, addr.balance); }) .catch((e) => { + if (signal?.aborted) return; log.errorf( "ETH balance failed", addr.address, @@ -243,6 +256,7 @@ async function refreshBalances( ); }) .catch((e) => { + if (signal?.aborted) return; log.errorf( "ENS reverse failed", addr.address, @@ -258,6 +272,7 @@ async function refreshBalances( addr.address, blockscoutUrl, trackedTokens, + signal, ).then((balances) => { if (balances !== null) { addr.tokenBalances = balances; diff --git a/tests/balanceRefreshCancelled.test.js b/tests/balanceRefreshCancelled.test.js new file mode 100644 index 0000000..c9e56d4 --- /dev/null +++ b/tests/balanceRefreshCancelled.test.js @@ -0,0 +1,67 @@ +// The balance refresh does not report a request the popup's own closing +// cancelled, and still reports one that failed while the popup was open +// (https://git.eeqj.de/sneak/AutistMask/issues/218). +// +// In the popup a cancelled fetch() fails with the same "Failed to fetch" as a +// server that cannot be reached, so every request here fails that way, and +// only the signal the popup aborts on pagehide tells the two cases apart. + +const { FetchRequest } = require("ethers"); +const { refreshBalances } = require("../src/shared/balances"); + +const RPC_URL = "https://rpc.example.invalid"; +const EXPLORER_URL = "https://explorer.example.invalid/api/v2"; +const ADDRESS = "0x1111111111111111111111111111111111111111"; + +const realFetch = globalThis.fetch; +let logged; + +beforeEach(() => { + logged = []; + jest.spyOn(console, "error").mockImplementation((...args) => { + logged.push(args.map(String).join(" ")); + }); + const failedToFetch = async () => { + throw new TypeError("Failed to fetch"); + }; + // The RPC calls (ETH balance, ENS name) and the explorer request (token + // balances) all fail the same way. + FetchRequest.registerGetUrl(failedToFetch); + globalThis.fetch = jest.fn(failedToFetch); +}); + +afterEach(() => { + FetchRequest.registerGetUrl(FetchRequest.createGetUrlFunc()); + globalThis.fetch = realFetch; + jest.restoreAllMocks(); +}); + +function refresh(signal) { + const wallets = [{ addresses: [{ address: ADDRESS }] }]; + return refreshBalances( + wallets, + RPC_URL, + EXPLORER_URL, + [], + "mainnet", + signal, + ); +} + +test("a failure while the popup is open is reported", async () => { + await refresh(new AbortController().signal); + for (const label of [ + "ETH balance failed", + "ENS reverse failed", + "fetchTokenBalances failed: Failed to fetch", + ]) { + expect(logged.some((line) => line.includes(label))).toBe(true); + } +}); + +test("a failure once the popup has closed is not", async () => { + const pageClosed = new AbortController(); + pageClosed.abort(); + await refresh(pageClosed.signal); + expect(logged).toEqual([]); +}); diff --git a/tests/e2e/network.js b/tests/e2e/network.js index 1b91cf4..86a5993 100644 --- a/tests/e2e/network.js +++ b/tests/e2e/network.js @@ -418,27 +418,23 @@ function sleep(ms) { const HOLD_POLL_MS = 25; const HOLD_MAX_MS = 30000; -// Hold a gas estimate open for as long as the test asks. +// Hold a reply open for as long as the test asks: until opts[name] is false. // -// opts.holdGasEstimate is read here rather than captured, so a test flips it -// on the same options object the route was registered with — the same -// pattern as seedTokenTransfer. This is the only way to observe the -// confirmation screen while its estimate is genuinely in flight; sampling -// the screen and hoping to win a race against the network would assert -// nothing on a slow machine. +// The switch (holdGasEstimate or holdBlockscout) is read here rather than +// captured, so a test flips it on the same options object the route was +// registered with — the same pattern as seedTokenTransfer. This is the only way +// to observe a screen while its request is genuinely in flight; sampling the +// screen and hoping to win a race against the network would assert nothing on +// a slow machine. // // It never gives up quietly. A hold that outlives the bound is reported like // any other harness fault, because a "pending" state that stopped being // pending on its own is a green assertion about the wrong screen. -async function awaitRelease(opts, report) { +async function awaitRelease(opts, name, report) { const started = Date.now(); - while (opts.holdGasEstimate) { + while (opts[name]) { if (Date.now() - started > HOLD_MAX_MS) { - report( - "held gas estimate was never released after " + - HOLD_MAX_MS + - "ms", - ); + report(name + " was never released after " + HOLD_MAX_MS + "ms"); return; } await sleep(HOLD_POLL_MS); @@ -562,7 +558,7 @@ async function handleRpc(route, postData, opts, report) { return route.abort(); } if (batch.some((req) => req.method === "eth_estimateGas")) { - await awaitRelease(opts, report); + await awaitRelease(opts, "holdGasEstimate", report); } const replies = batch.map((req) => rpcReply(req, opts, report)); @@ -618,6 +614,10 @@ function traceEnabled(raw) { * node-side refusal. * @param {boolean} [opts.holdGasEstimate] hold every batch containing an * eth_estimateGas until this is cleared again. + * @param {boolean} [opts.holdBlockscout] hold every Blockscout request until + * this is cleared again. + * @param {boolean} [opts.failTransactionList] fail every request for an + * address's transaction list as a network error; read at request time. * @param {string[]} [opts.broadcastTransactions] every raw signed * transaction handed to eth_sendRawTransaction, appended in order. * @param {number|string|null} [opts.tokenDecimalsOverride] the scale @@ -693,7 +693,12 @@ async function installNetworkStubs(ctx, opts) { // Blockscout v2 if (p.includes("/api/v2/")) { + await awaitRelease(opts, "holdBlockscout", report); if (/\/addresses\/0x[0-9a-fA-F]{40}\/transactions$/.test(p)) { + // Aborted rather than answered with an error status: to the + // page this is a server that cannot be reached, and fetch() + // rejects with "Failed to fetch". + if (opts.failTransactionList) return route.abort(); const addr = blockscoutAddress(p); return jsonResponse(route, { items: diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 4db35f4..797c52a 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -696,6 +696,62 @@ test("the token contract row links to the explorer's token page (#151)", async ( ); }); +// ------------------------------------------- reload mid-refresh (#218) +// +// Reloading or closing the popup makes Chrome cancel the requests it still has +// open, and in the page a cancelled fetch() fails with the same "Failed to +// fetch" as a server that cannot be reached. Here rather than straight after +// wallet creation because a reload also cancels the address scan that follows +// it, which still logs (#475). + +// Blockscout is held, so the home screen's transaction list and token balances +// cannot have been answered when the popup reloads. The assertion is the +// harness's own: a console.error from the page being reloaded fails this test. +test("reloading the popup mid-refresh reports no failure (#218)", async (env) => { + await goHome(env.page); + env.routeOpts.holdBlockscout = true; + try { + const inFlight = Promise.all([ + env.page.waitForRequest((r) => r.url().endsWith("/transactions")), + env.page.waitForRequest((r) => r.url().endsWith("/token-balances")), + ]); + await env.page.reload(); + await inFlight; + await env.page.reload(); + } finally { + env.routeOpts.holdBlockscout = false; + } + await visible(env.page, "#view-main"); +}); + +// The same "Failed to fetch" from a server that really cannot be reached is +// still reported, so it still fails the run unless a test declares it. Chrome +// reports the failed request itself as well, which a cancelled one does not. +test("a transaction list that cannot be fetched is still reported (#218)", async (env) => { + env.errors.expect( + "loadHomeTxs failed", + /loadHomeTxs failed: Failed to fetch/, + ); + env.errors.expect( + "Chrome's report of the failed request", + /Failed to load resource: net::ERR_FAILED/, + ); + env.routeOpts.failTransactionList = true; + try { + // Drawn again by the next ten-second refresh. + await env.page.waitForFunction( + () => + document + .getElementById("home-tx-list") + .textContent.includes("Failed to load transactions."), + null, + { timeout: 15000 }, + ); + } finally { + env.routeOpts.failTransactionList = false; + } +}); + // -------------------------------------------- recovery phrase (#161) // The gear toggles, so pressing it while Settings is already up leaves it. @@ -2107,10 +2163,9 @@ function quantity(wei) { // Wait on the main view until a changed balance fixture has been picked up. // -// Deliberately not a reload: the popup re-refreshes on a 10-second timer by -// itself, and reloading aborts whatever fetch the home screen has open at -// that instant, which the extension reports through log.errorf and the -// harness — correctly — fails the run on. +// Not a reload: the popup re-refreshes on a 10-second timer by itself, and a +// reload on the address screen still logs the transaction list it cancels +// (#475). // // It also deliberately settles on MAIN rather than on the address screen. // The address screen builds the send screen's token dropdown once, from the @@ -4470,6 +4525,10 @@ async function main() { ethBalanceWei: null, failGasEstimate: false, holdGasEstimate: false, + // Whether Blockscout requests are held unanswered, and whether an + // address's transaction list fails as a network error (#218). + holdBlockscout: false, + failTransactionList: false, // What decimals() answers for the stub token, when it is to answer // something other than the value the same fixture reports through // Blockscout. The token that lies about its scale (#305).