Read this first: the issue's premise does not survive its own verification
The issue says Firefox is non-functional because browser.* is promise-only
and the code calls it with Chrome-style callbacks, so the callback is never
invoked. That is false on Firefox 153.0.3, measured directly. Firefox's browser.* honours a trailing Chrome-style callback — and, when one is given,
returns no promise at all — and it does populate browser.runtime.lastError.
Probe run inside the pinned e2e container, from the extension's own popup page:
and, against item 8 specifically (browser.tabs.sendMessage to a dead tab id):
{ "hasLastErrorProp": true, "callbackFired": true, "lastErrorSet": true,
"lastErrorMessage": "Could not establish connection. Receiving end does not exist." }
Stronger still, end to end: I stashed every src/ change, rebuilt, and ran the
new Firefox suite against the unconverted code. 8/8, exit 0. All four
DoD flows pass without this PR's fix. Confirmed the stash took effect — the
build differed (the one tolerated console error was reported at src/popup/index.js:6 unfixed against :1 fixed).
So this is not a repair of a broken target. What it is:
the structural change the issue asks for in its own first DoD checkbox — one
compat module, no namespace ternaries anywhere else, one call shape;
the Firefox dApp coverage that turns the six manual checks into assertions,
which is what produced the measurement above;
no defect fixed. An earlier revision of this PR claimed the windows.create() write-back guard as its own; it is not this change's, and
the claim has been withdrawn from the commit message and from the section
below.
Reviewer's call whether the refactor is worth its diff on that basis. Nothing
in it is load-bearing for correctness on either browser today. Every comment in
the tree that stated the refuted premise now states the measurement instead — browserApi.js's header and its lastError() and storage notes, and the two
in tests/e2e/firefox/run.js.
Strategy: (a), the promise shim
src/shared/browserApi.js is now the only file in the tree that names browser or chrome. It exports lazily-resolved namespace handles for events
and synchronous methods (runtimeApi(), windowsApi(), tabsApi(), actionApi(), storageLocal(), alarmsApi()) and promise-returning wrappers
for everything callback-shaped on Chrome. Callers await.
Why (a) over (b): state.js, alarms.js and phishingDomains.js already used
the promise form. (b) would have made the callback half match the other by
regressing the promise half to callbacks, or left the tree split down the
middle. (a) makes it one shape and composes with the async handlers in the
background.
Three decisions worth naming rather than leaving to be found:
Storage is called in its promise form on both namespaces, not wrapped. chrome.storage.local.get() returns a promise on MV3 and three modules
already depended on that. Wrapping it would be a change, not a fix. storageGet() and storageSet()reject where storage.local is absent
rather than defaulting to {} and a no-op write: they carry the wallet, and
defaulting would make an existing wallet read back as no wallet and discard
every save silently. src/shared/phishingDomains.js is the one caller that
genuinely degrades — it falls back to its vendored blocklist — and it takes storageLocal() directly with its own null check.
notify() is a separate entry point for a send whose answer nobody
reads. It sends with no callback and swallows the no-receiver rejection.
Appending a callback would manufacture a lastError for a receiver that was
never expected to reply — and on MV3 the one-argument send returns a promise
that rejects with no listener, which was previously an unhandled rejection at
four call sites.
runtime.lastError is gone entirely. On the Chrome path the wrapper
reads it inside the callback and rejects; the three sites that checked it are
now .catch(() => {}) on the send itself.
Per-call-site conversion
All eight still existed, at moved line numbers.
#
Site
Now
1
content/index.jsstorage.get("eip6963Uuid", cb)
await storageGet(...) / await storageSet(...) in an async IIFE. A storage failure now still announces, under a fresh uuid, rather than not announcing at all.
2
content/index.jsruntime.sendMessage(msg, cb)
sendMessage(msg).then(...).catch(...). The catch preserves today's behaviour (page promise stays pending when the background is gone) rather than quietly changing what dApps see.
3
background/index.jswindowsApi.getLastFocused(cb)
await windowsGetLastFocused(), failure = open uncentred.
4
background/index.jswindowsApi.create(opts, cb)
await windowsCreate(opts), failure logged and the approval settled via the !win path that came in with #271.
5
background/index.jstabsApi.query/sendMessage in broadcastChainChanged
await tabsQuery({}), then tabsSendMessage(...).catch(() => {}) per tab.
6
background/index.jswindowsApi.remove(id, cb) and the same pattern in broadcastAccountsChanged
await sendMessage(...); show() is async and absorbs its own failure. The three reject buttons and the site-approval buttons use notify().
8
background/index.js three runtime.lastError checks
Deleted; see above.
Beyond 1-8, so the DoD's "no ternaries outside that module" holds: state.js, alarms.js, phishingDomains.js, walletDelete.js, popup/views/home.js, popup/views/settings.js. Namespace re-source only, no call-shape change. src/content/inpage.js untouched.
grep -rn 'typeof browser\|\bchrome\.\|\bbrowser\.' src/ outside the module
returns two comments and no code.
No manifest change was needed, and none was made. manifest/firefox.json
stays MV2-correct.
The windows.create() guard is preserved here, not introduced
pendingApprovals[id].windowId = win.id is guarded, and needs to be: windows.create() is asynchronous on both browsers, and an approval settled
during the open — an address switch goes through broadcastAccountsChanged()
and deletes it — would otherwise be a dereference of a deleted entry.
That guard is not this change's. It, and the !win settle path beside it,
arrived with #271 in c06765e,
which is already on next. Converting the call to windowsCreate() carries
both across unchanged. An earlier revision of this PR and of its commit message
claimed the fix; that claim was wrong and is withdrawn.
show() is async and unawaited, on purpose
src/popup/index.js calls approval.show() without awaiting it and without a .catch(), so a throw past its first await is an unhandled rejection rather
than an uncaught error. That is not a loss of visibility, measured rather than
assumed: a throw at the END of show()'s site-approval branch — past the
first await, after every DOM write, so the view still renders and the
rejection is the only difference — fails both suites.
Firefox: not ok 5, uncaught extension errors during this step, Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2. Steps 6-8 stayed green.
Chrome: not ok 32 and not ok 33, both pageerror: PROBE past the first await in show(), 38/40, exit 2.
README.md records the demonstration; it previously documented neither harness
as covering this.
Not regressed
settleApproval() is still the single chokepoint: exactly one delete pendingApprovals[...] and one approval.resolve(...), both inside it; claimApproval()/releaseApproval() and attemptInFlight untouched; the #216
background-side population and approvedFrom pinning untouched; the
defective-wallet gates in approval.js untouched. Only the browser-API calls
around them changed.
What was tested, and on what
Chrome — make test-e2e, 44/44. Chromium from mcr.microsoft.com/playwright:v1.56.0-noble (pinned by digest), Playwright
1.56.0, MV3 build from dist/chrome/. Covers all four flows plus eth_signTypedData_v4, every signature recovered in the runner, the
transaction asserted against the raw bytes the stubbed RPC received, and the
password absent from both boundaries.
Firefox — make test-e2e-firefox, 8/8. Firefox 153.0.3 and geckodriver 0.36.0, both pinned by digest, MV2 build from dist/firefox/ installed as a
temporary add-on. Steps 1-3 are the pre-existing popup steps; 4-8 are new:
4 — the loopback page gets the real inpage provider: EIP-6963
announcement naming berlin.sneak.autistmask, identity-checked against window.ethereum, carrying the 36-char uuid read from storage (site 1), then
an eth_chainId round trip through the relay (site 2).
5 — eth_requestAccounts: prompt names the origin and the address,
approve, page receives exactly the active address.
6 — personal_sign: prompt shows origin, type, decoded message and
signing address; signature recovered in the runner with verifyMessage(getBytes(...)) and compared to the address read out of
extension storage.
7 — eth_sendTransaction: prompt shows origin, sender, recipient, value
and raw calldata; the broadcast artifact is parsed from what reached the node
and checked field by field (signer, to, value, data, chainId 1); the hash the
page received and the hash on the wait screen must both be that artifact's.
8 — closing the approval window rejects with 4001 on the wire and on
the page's ProviderRpcError (now that #274 has landed), and nothing was
broadcast. This is the windows.onRemoved path, which can only fire if windows.create() produced a window id — site 4.
How the http:// origin problem was solved
The Firefox harness documented that with --network none there is no http://
page to inject a content script into. Loopback survives --network none. tests/e2e/firefox/dapp.js serves the dApp page and a JSON-RPC node from 127.0.0.1 inside the same container, and the extension's rpcUrl is pointed
at it. The run still reaches nothing but itself. The page fixture is not
written twice — DAPP_HTML is exported from the Chrome suite and served
verbatim — so an assertion about the __dapp API means the same thing on both.
A JSON-RPC method the fixture does not model fails the run rather than
answering null.
What I could not drive, and one tolerated error
The toolbar-anchored popup presentation. A panel is not a top-level
browsing context, so WebDriver cannot see or click it — the same blind spot
the Chrome harness documents. extensions.openPopupWithoutUserGesture.enabled
is pinned to false in the profile so the site prompt deterministically takes
its shipped windows.create() fallback, rather than leaving which path runs
to whether headless Firefox counts as having had a user gesture. Same approval
id, same code; the panel presentation itself is uncovered on both browsers.
eth_signTypedData_v4 is not in the Firefox suite. Chrome covers it; the
DoD named four flows and those are the four.
Content-script error capture is still unproven. Content scripts are now
executed, which they were not before, but no probe has forced a throw inside
one and watched it fail the run. The README claim was downgraded to say
exactly that rather than upgraded.
The poll-based capture limits are unchanged: ~1.5s tail, 250-message ring
buffer.
One tolerated error, in a new ALLOWED_ERRORS list mirroring the Chrome
harness's, naming the issue that deletes it and printed on every occurrence:
Firefox reports Promise rejected after context unloaded for the
site-approval popup's unawaited sendMessage when window.close() unloads
the context. Pre-existing — the send was already unawaited — and
unsuppressable from calling code, because BaseContext.wrapPromise reports it
whether or not a handler is attached. It is the same teardown ordering as #275, and awaiting the send before
closing is precisely what that issue forbids as a fix, so it is tolerated
here rather than worked around.
Incidentally measured for that issue, and posted there: in a real window on
Firefox, the approve message wins the race every time across these runs — the
opposite of the Chrome-in-a-tab result it records.
Gates
make check — green. 30 suites, 743 tests; script/test-verify-build
18/18 cases; prettier --check clean. Re-run containerized through script/cibuild (docker build), same counts, exit 0.
make test-e2e — green, 44/44, exit 0.
make test-e2e-firefox — green, 8/8, exit 0.
Rebased onto next at ab1c184. That picked up #291, which bakes the repo and the
extension build into the Firefox e2e image instead of bind-mounting it — so
both e2e suites were re-run rather than carried over, because that change
alters how this PR's own Firefox suite is built and executed. The only conflict
was in TODO.md, against #296; both Completed Steps entries
kept.
Closes https://git.eeqj.de/sneak/AutistMask/issues/153.
## Read this first: the issue's premise does not survive its own verification
The issue says Firefox is non-functional because `browser.*` is promise-only
and the code calls it with Chrome-style callbacks, so the callback is never
invoked. **That is false on Firefox 153.0.3, measured directly.** Firefox's
`browser.*` honours a trailing Chrome-style callback — and, when one is given,
returns no promise at all — and it does populate `browser.runtime.lastError`.
Probe run inside the pinned e2e container, from the extension's own popup page:
```
{ "typeofBrowser": "object", "typeofChrome": "object", "browserIsChrome": false,
"storageCallbackFired": true, "storageReturnedThenable": false,
"sendMessageCallbackFired": true, "sendMessageReturnedThenable": false,
"getLastFocusedCallbackFired": true, "getLastFocusedReturnedThenable": false,
"getLastFocusedId": 1 }
```
and, against item 8 specifically (`browser.tabs.sendMessage` to a dead tab id):
```
{ "hasLastErrorProp": true, "callbackFired": true, "lastErrorSet": true,
"lastErrorMessage": "Could not establish connection. Receiving end does not exist." }
```
Stronger still, end to end: I stashed every `src/` change, rebuilt, and ran the
new Firefox suite against the **unconverted** code. **8/8, exit 0.** All four
DoD flows pass without this PR's fix. Confirmed the stash took effect — the
build differed (the one tolerated console error was reported at
`src/popup/index.js:6` unfixed against `:1` fixed).
So this is **not a repair of a broken target**. What it is:
- the structural change the issue asks for in its own first DoD checkbox — one
compat module, no namespace ternaries anywhere else, one call shape;
- the Firefox dApp coverage that turns the six manual checks into assertions,
which is what produced the measurement above;
- **no defect fixed.** An earlier revision of this PR claimed the
`windows.create()` write-back guard as its own; it is not this change's, and
the claim has been withdrawn from the commit message and from the section
below.
Reviewer's call whether the refactor is worth its diff on that basis. Nothing
in it is load-bearing for correctness on either browser today. Every comment in
the tree that stated the refuted premise now states the measurement instead —
`browserApi.js`'s header and its `lastError()` and storage notes, and the two
in `tests/e2e/firefox/run.js`.
## Strategy: (a), the promise shim
`src/shared/browserApi.js` is now the only file in the tree that names
`browser` or `chrome`. It exports lazily-resolved namespace handles for events
and synchronous methods (`runtimeApi()`, `windowsApi()`, `tabsApi()`,
`actionApi()`, `storageLocal()`, `alarmsApi()`) and promise-returning wrappers
for everything callback-shaped on Chrome. Callers `await`.
Why (a) over (b): `state.js`, `alarms.js` and `phishingDomains.js` already used
the **promise** form. (b) would have made the callback half match the other by
regressing the promise half to callbacks, or left the tree split down the
middle. (a) makes it one shape and composes with the `async` handlers in the
background.
Three decisions worth naming rather than leaving to be found:
- **Storage is called in its promise form on both namespaces**, not wrapped.
`chrome.storage.local.get()` returns a promise on MV3 and three modules
already depended on that. Wrapping it would be a change, not a fix.
`storageGet()` and `storageSet()` **reject** where `storage.local` is absent
rather than defaulting to `{}` and a no-op write: they carry the wallet, and
defaulting would make an existing wallet read back as no wallet and discard
every save silently. `src/shared/phishingDomains.js` is the one caller that
genuinely degrades — it falls back to its vendored blocklist — and it takes
`storageLocal()` directly with its own null check.
- **`notify()`** is a separate entry point for a send whose answer nobody
reads. It sends with no callback and swallows the no-receiver rejection.
Appending a callback would manufacture a `lastError` for a receiver that was
never expected to reply — and on MV3 the one-argument send returns a promise
that rejects with no listener, which was previously an unhandled rejection at
four call sites.
- **`runtime.lastError` is gone entirely.** On the Chrome path the wrapper
reads it inside the callback and rejects; the three sites that checked it are
now `.catch(() => {})` on the send itself.
## Per-call-site conversion
All eight still existed, at moved line numbers.
| # | Site | Now |
|---|------|-----|
| 1 | `content/index.js` `storage.get("eip6963Uuid", cb)` | `await storageGet(...)` / `await storageSet(...)` in an async IIFE. A storage failure now still announces, under a fresh uuid, rather than not announcing at all. |
| 2 | `content/index.js` `runtime.sendMessage(msg, cb)` | `sendMessage(msg).then(...).catch(...)`. The catch preserves today's behaviour (page promise stays pending when the background is gone) rather than quietly changing what dApps see. |
| 3 | `background/index.js` `windowsApi.getLastFocused(cb)` | `await windowsGetLastFocused()`, failure = open uncentred. |
| 4 | `background/index.js` `windowsApi.create(opts, cb)` | `await windowsCreate(opts)`, failure logged and the approval settled via the `!win` path that came in with `#271`. |
| 5 | `background/index.js` `tabsApi.query`/`sendMessage` in `broadcastChainChanged` | `await tabsQuery({})`, then `tabsSendMessage(...).catch(() => {})` per tab. |
| 6 | `background/index.js` `windowsApi.remove(id, cb)` and the same pattern in `broadcastAccountsChanged` | `windowsRemove(id).catch(() => {})`, `await tabsQuery({})`. |
| 7 | `popup/views/approval.js` `AUTISTMASK_GET_APPROVAL`, `AUTISTMASK_TX_RESPONSE`, `AUTISTMASK_SIGN_RESPONSE` | `await sendMessage(...)`; `show()` is async and absorbs its own failure. The three reject buttons and the site-approval buttons use `notify()`. |
| 8 | `background/index.js` three `runtime.lastError` checks | Deleted; see above. |
Beyond 1-8, so the DoD's "no ternaries outside that module" holds: `state.js`,
`alarms.js`, `phishingDomains.js`, `walletDelete.js`, `popup/views/home.js`,
`popup/views/settings.js`. Namespace re-source only, no call-shape change.
`src/content/inpage.js` untouched.
`grep -rn 'typeof browser\|\bchrome\.\|\bbrowser\.' src/` outside the module
returns two comments and no code.
**No manifest change was needed**, and none was made. `manifest/firefox.json`
stays MV2-correct.
### The `windows.create()` guard is preserved here, not introduced
`pendingApprovals[id].windowId = win.id` is guarded, and needs to be:
`windows.create()` is asynchronous on both browsers, and an approval settled
during the open — an address switch goes through `broadcastAccountsChanged()`
and deletes it — would otherwise be a dereference of a deleted entry.
**That guard is not this change's.** It, and the `!win` settle path beside it,
arrived with https://git.eeqj.de/sneak/AutistMask/issues/271 in `c06765e`,
which is already on `next`. Converting the call to `windowsCreate()` carries
both across unchanged. An earlier revision of this PR and of its commit message
claimed the fix; that claim was wrong and is withdrawn.
### `show()` is async and unawaited, on purpose
`src/popup/index.js` calls `approval.show()` without awaiting it and without a
`.catch()`, so a throw past its first `await` is an unhandled rejection rather
than an uncaught error. That is not a loss of visibility, measured rather than
assumed: a `throw` at the END of `show()`'s site-approval branch — past the
first `await`, after every DOM write, so the view still renders and the
rejection is the only difference — fails both suites.
- Firefox: `not ok 5`, `uncaught extension errors during this step`,
`Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript)`, 7/8, exit 2. Steps 6-8 stayed green.
- Chrome: `not ok 32` and `not ok 33`, both `pageerror: PROBE past the first await in show()`, 38/40, exit 2.
`README.md` records the demonstration; it previously documented neither harness
as covering this.
### Not regressed
`settleApproval()` is still the single chokepoint: exactly one
`delete pendingApprovals[...]` and one `approval.resolve(...)`, both inside it;
`claimApproval()`/`releaseApproval()` and `attemptInFlight` untouched; the #216
background-side population and `approvedFrom` pinning untouched; the
defective-wallet gates in `approval.js` untouched. Only the browser-API calls
around them changed.
## What was tested, and on what
**Chrome — `make test-e2e`, 44/44.** Chromium from
`mcr.microsoft.com/playwright:v1.56.0-noble` (pinned by digest), Playwright
1.56.0, MV3 build from `dist/chrome/`. Covers all four flows plus
`eth_signTypedData_v4`, every signature recovered in the runner, the
transaction asserted against the raw bytes the stubbed RPC received, and the
password absent from both boundaries.
**Firefox — `make test-e2e-firefox`, 8/8.** Firefox **153.0.3** and geckodriver
**0.36.0**, both pinned by digest, MV2 build from `dist/firefox/` installed as a
temporary add-on. Steps 1-3 are the pre-existing popup steps; 4-8 are new:
- **4** — the loopback page gets the real inpage provider: EIP-6963
announcement naming `berlin.sneak.autistmask`, identity-checked against
`window.ethereum`, carrying the 36-char uuid read from storage (site 1), then
an `eth_chainId` round trip through the relay (site 2).
- **5** — `eth_requestAccounts`: prompt names the origin and the address,
approve, page receives exactly the active address.
- **6** — `personal_sign`: prompt shows origin, type, decoded message and
signing address; signature recovered **in the runner** with
`verifyMessage(getBytes(...))` and compared to the address read out of
extension storage.
- **7** — `eth_sendTransaction`: prompt shows origin, sender, recipient, value
and raw calldata; the broadcast artifact is parsed from what reached the node
and checked field by field (signer, to, value, data, chainId 1); the hash the
page received and the hash on the wait screen must both be that artifact's.
- **8** — closing the approval window rejects with 4001 on the wire **and** on
the page's `ProviderRpcError` (now that #274 has landed), and nothing was
broadcast. This is the `windows.onRemoved` path, which can only fire if
`windows.create()` produced a window id — site 4.
### How the http:// origin problem was solved
The Firefox harness documented that with `--network none` there is no `http://`
page to inject a content script into. Loopback survives `--network none`.
`tests/e2e/firefox/dapp.js` serves the dApp page and a JSON-RPC node from
`127.0.0.1` inside the same container, and the extension's `rpcUrl` is pointed
at it. The run still reaches nothing but itself. The page fixture is not
written twice — `DAPP_HTML` is exported from the Chrome suite and served
verbatim — so an assertion about the `__dapp` API means the same thing on both.
A JSON-RPC method the fixture does not model **fails the run** rather than
answering `null`.
### What I could not drive, and one tolerated error
- **The toolbar-anchored popup presentation.** A panel is not a top-level
browsing context, so WebDriver cannot see or click it — the same blind spot
the Chrome harness documents. `extensions.openPopupWithoutUserGesture.enabled`
is pinned to `false` in the profile so the site prompt deterministically takes
its shipped `windows.create()` fallback, rather than leaving which path runs
to whether headless Firefox counts as having had a user gesture. Same approval
id, same code; the panel presentation itself is uncovered on both browsers.
- **`eth_signTypedData_v4` is not in the Firefox suite.** Chrome covers it; the
DoD named four flows and those are the four.
- **Content-script error *capture* is still unproven.** Content scripts are now
executed, which they were not before, but no probe has forced a throw inside
one and watched it fail the run. The README claim was downgraded to say
exactly that rather than upgraded.
- **The poll-based capture limits are unchanged**: ~1.5s tail, 250-message ring
buffer.
- **One tolerated error**, in a new `ALLOWED_ERRORS` list mirroring the Chrome
harness's, naming the issue that deletes it and printed on every occurrence:
Firefox reports `Promise rejected after context unloaded` for the
site-approval popup's unawaited `sendMessage` when `window.close()` unloads
the context. Pre-existing — the send was already unawaited — and
unsuppressable from calling code, because `BaseContext.wrapPromise` reports it
whether or not a handler is attached. It is the same teardown ordering as
https://git.eeqj.de/sneak/AutistMask/issues/275, and awaiting the send before
closing is precisely what that issue forbids as a fix, so it is tolerated
here rather than worked around.
Incidentally measured for that issue, and posted there: in a real window on
Firefox, the approve message wins the race every time across these runs — the
opposite of the Chrome-in-a-tab result it records.
## Gates
- `make check` — **green.** 30 suites, 743 tests; `script/test-verify-build`
18/18 cases; `prettier --check` clean. Re-run containerized through
`script/cibuild` (`docker build`), same counts, exit 0.
- `make test-e2e` — **green, 44/44**, exit 0.
- `make test-e2e-firefox` — **green, 8/8**, exit 0.
Rebased onto `next` at `ab1c184`. That picked up
https://git.eeqj.de/sneak/AutistMask/pulls/291, which bakes the repo and the
extension build into the Firefox e2e image instead of bind-mounting it — so
both e2e suites were re-run rather than carried over, because that change
alters how this PR's own Firefox suite is built and executed. The only conflict
was in `TODO.md`, against
https://git.eeqj.de/sneak/AutistMask/pulls/296; both Completed Steps entries
kept.
Every call site that touched `browser.*` or `chrome.*` now goes through
`src/shared/browserApi.js`, the only file in the tree that names either.
It exposes lazily-resolved namespace handles for events and synchronous
methods, and promise-returning wrappers for everything that is
callback-shaped on Chrome. Callers await; `runtime.lastError` is gone,
folded into the rejection the wrapper produces on the Chrome path.
The Firefox suite gains the four dApp round trips the issue's definition
of done asks for — `eth_requestAccounts`, `personal_sign`,
`eth_sendTransaction`, and a closed approval window rejecting with
EIP-1193 4001 — driven through the real content script, background page
and approval windows. `--network none` was thought to rule that out
because it leaves no `http://` origin to inject into; loopback survives
it, so the page and a JSON-RPC node are served from 127.0.0.1 inside the
container and the run still reaches nothing but itself.
That harness refutes the premise it was built to verify. On Firefox
153.0.3, `browser.*` honours a trailing Chrome-style callback and does
populate `runtime.lastError`, both measured directly, and all four flows
pass against the unconverted code. So this is a uniformity and coverage
change, not a repair of a broken target; the PR records the measurement
in full.
One real defect is fixed on the way past: the window id written back
into a pending approval after `windows.create()` was unguarded, so an
approval settled during the open — an address switch will do it —
dereferenced a deleted entry.
1. src/shared/browserApi.js:3-12 documents a browser behaviour this same commit demonstrates does not exist. The header of the module designated the single authority on this states that Firefox browser.* methods "take no callback at all - a function passed where an options argument is expected is simply never invoked, so the call looks like it succeeded and silently never completes", and that the previous code "is broken on Firefox in exactly that way". TODO.md:48-59, the commit message and the PR body all record the opposite as measured on Firefox 153.0.3, and record that all four flows pass against the unconverted code. The refuted claim is repeated twice more in the tests: tests/e2e/firefox/run.js:168 ("hand a Chrome-style callback to the promise-only browser.* namespace and simply never complete") and :707 ("a callback the browser.* namespace never invoked"). The tree cannot assert both; whichever is right, a reader arriving at browserApi.js will take its header as the reason the module exists. Acceptable: rewrite those three comments to the measured behaviour and to the real justification (one namespace, one call shape, composes with the async handlers), keeping the link to #153.
2. src/shared/browserApi.js:164-178 turns a loud failure into a silent one for persisted wallet state.storageGet() resolves {} and storageSet() resolves as a no-op when storage.local is absent. src/shared/state.js:114 (saveState) and :118 (loadState) and src/background/index.js:65 (getState) now consume that, so a missing storage namespace makes the wallet read back as no wallet and makes every save discard silently, with nothing logged and no throw. Before this commit state.js resolved chrome.storage.local at module load and threw immediately in that case. Silent defaulting is the class of defect this repo rejects, and it is worse here than for the pre-existing callers because the value being defaulted is the user's wallet. The rationale comment at :83-84 ("Null rather than a throw because two callers degrade rather than fail on it") no longer describes the caller set. Acceptable: storageGet/storageSet reject when there is no storage.local, and the callers that genuinely want to degrade (src/shared/phishingDomains.js) keep using storageLocal() directly and keep their own null check.
3. Raised, not asserted - I did not probe this.src/popup/views/approval.js:444 - show() became async and src/popup/index.js:232 does not await it, so a throw in showTxApproval/showSignApproval or in the DOM writes after the first await is now an unhandled rejection rather than an uncaught error. Neither harness's capture is documented to fail a run on an unhandled rejection (tests/e2e/harness.js listens on pageerror/console; tests/e2e/firefox/driver.js reads nsIConsoleService, non-warning entries only). Confirm before dismissing.
Verified green on 34c1b00, reproduced independently in a fresh clone: make check inside a fresh docker build (28 suites / 681 tests, script/test-verify-build 18/18, prettier --check clean, zero CACHED layers), make test-e2e 37/37, make test-e2e-firefox 8/8, CI green, fast-forward onto next at 9dcd875, one commit ending (closes #153), no attribution trailers. Probe against vacuity: suppressing the windowId write-back in openApprovalWindow() turns step 8 into not ok 8, so the closed-window/4001 assertion and issue call site 4 are genuinely exercised.
FAIL - needs-rework.
**1. `src/shared/browserApi.js:3-12` documents a browser behaviour this same commit demonstrates does not exist.** The header of the module designated the single authority on this states that Firefox `browser.*` methods "take no callback at all - a function passed where an options argument is expected is simply never invoked, so the call looks like it succeeded and silently never completes", and that the previous code "is broken on Firefox in exactly that way". `TODO.md:48-59`, the commit message and the PR body all record the opposite as measured on Firefox 153.0.3, and record that all four flows pass against the unconverted code. The refuted claim is repeated twice more in the tests: `tests/e2e/firefox/run.js:168` ("hand a Chrome-style callback to the promise-only `browser.*` namespace and simply never complete") and `:707` ("a callback the `browser.*` namespace never invoked"). The tree cannot assert both; whichever is right, a reader arriving at `browserApi.js` will take its header as the reason the module exists. Acceptable: rewrite those three comments to the measured behaviour and to the real justification (one namespace, one call shape, composes with the `async` handlers), keeping the link to https://git.eeqj.de/sneak/AutistMask/issues/153.
**2. `src/shared/browserApi.js:164-178` turns a loud failure into a silent one for persisted wallet state.** `storageGet()` resolves `{}` and `storageSet()` resolves as a no-op when `storage.local` is absent. `src/shared/state.js:114` (`saveState`) and `:118` (`loadState`) and `src/background/index.js:65` (`getState`) now consume that, so a missing storage namespace makes the wallet read back as no wallet and makes every save discard silently, with nothing logged and no throw. Before this commit `state.js` resolved `chrome.storage.local` at module load and threw immediately in that case. Silent defaulting is the class of defect this repo rejects, and it is worse here than for the pre-existing callers because the value being defaulted is the user's wallet. The rationale comment at `:83-84` ("Null rather than a throw because two callers degrade rather than fail on it") no longer describes the caller set. Acceptable: `storageGet`/`storageSet` reject when there is no `storage.local`, and the callers that genuinely want to degrade (`src/shared/phishingDomains.js`) keep using `storageLocal()` directly and keep their own null check.
**3. Raised, not asserted - I did not probe this.** `src/popup/views/approval.js:444` - `show()` became `async` and `src/popup/index.js:232` does not await it, so a throw in `showTxApproval`/`showSignApproval` or in the DOM writes after the first `await` is now an unhandled rejection rather than an uncaught error. Neither harness's capture is documented to fail a run on an unhandled rejection (`tests/e2e/harness.js` listens on `pageerror`/`console`; `tests/e2e/firefox/driver.js` reads `nsIConsoleService`, non-warning entries only). Confirm before dismissing.
Verified green on `34c1b00`, reproduced independently in a fresh clone: `make check` inside a fresh `docker build` (28 suites / 681 tests, `script/test-verify-build` 18/18, `prettier --check` clean, zero `CACHED` layers), `make test-e2e` 37/37, `make test-e2e-firefox` 8/8, CI green, fast-forward onto `next` at `9dcd875`, one commit ending ` (closes #153)`, no attribution trailers. Probe against vacuity: suppressing the `windowId` write-back in `openApprovalWindow()` turns step 8 into `not ok 8`, so the closed-window/4001 assertion and issue call site 4 are genuinely exercised.
Reworked at 3b069d8, rebased onto next at 51e84ae.
1 — fixed. The browserApi.js header now states the measured behaviour (Firefox 153.0.3 browser.* honours a trailing Chrome-style callback, returns no promise when one is given, and populates runtime.lastError) and gives the real justification: uniformity, one namespace and one call shape, composing with the async handlers. Same for the two test comments (tests/e2e/firefox/run.js, the dApp-section header and the closed-window step). Two more sites carried the same refuted claim and are fixed too: the lastError() comment ("never populated for a browser.* call") and the storage note that said the popup flows work "while everything in the issue above does not". grep for promise-only / never invoked / never complete / broken on Firefox over src/, tests/, docs/, README.md, TODO.md now returns nothing.
2 — fixed.storageGet() and storageSet() reject where storage.local is absent, with the method named in the message. src/shared/phishingDomains.js was already the only caller that degrades, and it already takes storageLocal() directly with its own null check — unchanged. storageLocal()'s comment now says which caller it is null for. TODO.md and the commit message record the decision.
3 — rebutted with a probe, not dismissed. The premise does not hold: an unhandled rejection fails a run on both harnesses. Probe: throw new Error(...) placed at the END of show()'s site-approval branch, past its first await and after every DOM write, so the approval view still renders and the failure is the rejection alone.
Firefox: not ok 5 - eth_requestAccounts approved returns the selected address / uncaught extension errors during this step / Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2. Steps 6-8 stayed green, confirming the UI was intact.
Chrome: not ok 32 and not ok 33, both pageerror: PROBE past the first await in show(), 38/40, exit 2.
So show() is left unawaited and un-catched; adding a .catch would have bought nothing. The call site carries a two-line note saying so, and README.md records the demonstration in the end-to-end section (it previously documented neither harness as covering this). An earlier variant of the probe that threw BEFORE the DOM writes turned 4 steps red for the obvious reason and proves nothing about capture — discarded, not reported.
Gates, all after the rebase, which took one conflict in TODO.md against #285 (both Completed Steps entries kept, mine re-dated to the landing date) — resolved and make check re-run afterwards:
make test-e2e-firefox — green, 8/8, exit 0, with the one ALLOWED_ERRORS entry for #275 printed as designed.
Not verified / unchanged from the last round: content-script error capture is still unproven (no probe has forced a throw inside a content script); the toolbar-anchored popup presentation is still undrivable on either browser; eth_signTypedData_v4 is still Chrome-only. No harness assertion was weakened.
Reworked at `3b069d8`, rebased onto `next` at `51e84ae`.
**1 — fixed.** The `browserApi.js` header now states the measured behaviour (Firefox 153.0.3 `browser.*` honours a trailing Chrome-style callback, returns no promise when one is given, and populates `runtime.lastError`) and gives the real justification: uniformity, one namespace and one call shape, composing with the `async` handlers. Same for the two test comments (`tests/e2e/firefox/run.js`, the dApp-section header and the closed-window step). Two more sites carried the same refuted claim and are fixed too: the `lastError()` comment ("never populated for a `browser.*` call") and the storage note that said the popup flows work "while everything in the issue above does not". `grep` for `promise-only` / `never invoked` / `never complete` / `broken on Firefox` over `src/`, `tests/`, `docs/`, `README.md`, `TODO.md` now returns nothing.
**2 — fixed.** `storageGet()` and `storageSet()` reject where `storage.local` is absent, with the method named in the message. `src/shared/phishingDomains.js` was already the only caller that degrades, and it already takes `storageLocal()` directly with its own null check — unchanged. `storageLocal()`'s comment now says which caller it is null for. `TODO.md` and the commit message record the decision.
**3 — rebutted with a probe, not dismissed.** The premise does not hold: an unhandled rejection fails a run on **both** harnesses. Probe: `throw new Error(...)` placed at the END of `show()`'s site-approval branch, past its first `await` and after every DOM write, so the approval view still renders and the failure is the rejection alone.
- Firefox: `not ok 5 - eth_requestAccounts approved returns the selected address` / `uncaught extension errors during this step` / `Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript)`, `7/8`, exit 2. Steps 6-8 stayed green, confirming the UI was intact.
- Chrome: `not ok 32` and `not ok 33`, both `pageerror: PROBE past the first await in show()`, `38/40`, exit 2.
So `show()` is left unawaited and un-`catch`ed; adding a `.catch` would have bought nothing. The call site carries a two-line note saying so, and `README.md` records the demonstration in the end-to-end section (it previously documented neither harness as covering this). An earlier variant of the probe that threw BEFORE the DOM writes turned 4 steps red for the obvious reason and proves nothing about capture — discarded, not reported.
**Gates**, all after the rebase, which took one conflict in `TODO.md` against [#285](https://git.eeqj.de/sneak/AutistMask/pulls/285) (both Completed Steps entries kept, mine re-dated to the landing date) — resolved and `make check` re-run afterwards:
- `make check` — green. 29 suites, 703 tests; `script/test-verify-build` 18/18; `prettier --check` clean.
- `make test-e2e` — green, 40/40, exit 0.
- `make test-e2e-firefox` — green, 8/8, exit 0, with the one `ALLOWED_ERRORS` entry for [#275](https://git.eeqj.de/sneak/AutistMask/issues/275) printed as designed.
**Not verified / unchanged from the last round**: content-script error *capture* is still unproven (no probe has forced a throw inside a content script); the toolbar-anchored popup presentation is still undrivable on either browser; `eth_signTypedData_v4` is still Chrome-only. No harness assertion was weakened.
next moved to d9d50f0 (#188, which also touches tests/e2e/network.js) while the above was being written. Rebased onto it, clean this time; head is now c0432eb. Re-run on that: make check green (29 suites / 703 tests, script/test-verify-build 18/18, prettier --check clean), make test-e2e44/44 exit 0 (the four new Chrome cases from #188 included), make test-e2e-firefox8/8 exit 0. The gate numbers in the PR body are from the previous rebase; these supersede them.
`next` moved to `d9d50f0` ([#188](https://git.eeqj.de/sneak/AutistMask/pulls/188), which also touches `tests/e2e/network.js`) while the above was being written. Rebased onto it, clean this time; head is now `c0432eb`. Re-run on that: `make check` green (29 suites / 703 tests, `script/test-verify-build` 18/18, `prettier --check` clean), `make test-e2e` **44/44** exit 0 (the four new Chrome cases from #188 included), `make test-e2e-firefox` **8/8** exit 0. The gate numbers in the PR body are from the previous rebase; these supersede them.
c0432eb committer identity.git log -1 --format='%an <%ae> | %cn <%ce>' returns clawbot <clawbot@noreply.example.org> | sneak <sneak@sneak.berlin>. The committer field on a commit no human wrote or reviewed is the repo owner — the misattribution #186 tracks, introduced by the rebase onto d9d50f0, minutes after #186 (comment) recorded option (a) as enforced operationally. Every commit on next (d9d50f0, 51e84ae, 0be20d7, 9dcd875, c755a5e) is clawbot on both fields; this head is the outlier. Acceptable: re-commit with user.name/user.email set to clawbot and force-push, no content change. Waivable at the owner's call — #186 (comment) records that squash-merge rewrites both fields, so it never reaches next.
The .catch removal is justified — probe re-derived independently, not taken on trust. A throw at the end of show()'s site-approval branch, past the first await and after every DOM write: Firefox not ok 5 - eth_requestAccounts approved returns the selected address / uncaught extension errors during this step / Error: ... (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2, steps 6-8 green; Chrome not ok 36 and not ok 37, both pageerror, 42/44, exit 2. Firefox reports it as an ordinary non-warning nsIScriptError in category content javascript — the generic unhandled-rejection reporter, nothing incidental to that call site — so capture generalises to any unawaited async call in an extension page. Leaving approval.show() unawaited is correct and the rebuttal stands.
The refuted premise, and whether #153 still closes. The grep for promise-only / never invoked / never complete / broken on Firefox / non-functional across src/ tests/ docs/ README.md TODO.md returns nothing. It still closes: DoD checkbox 1 holds (outside browserApi.js, grep -rnE "typeof browser|typeof chrome|\bchrome\.|\bbrowser\." src/ returns three prose comments and no code), and the six manual checks are the harness assertions sanctioned in #153 (comment). What landed is uniformity plus Firefox dApp coverage rather than a repair, which the commit, PR body and TODO.md all say.
Everything else re-checked and green in a fresh clone at c0432eb: make check (29 suites / 703 tests, script/test-verify-build 18/18, prettier --check clean, exit 0); CI green; fast-forward onto next; single commit, title ends (closes #153), base next, TODO.md in the same commit; both rebases dropped nothing (TODO.md and tests/e2e/network.js are additions only, #285's entry survives); no Claude/Anthropic references or attribution trailers; every deleted runtime.lastError check was an empty no-op now equivalently .catch(() => {}), and invoke() reads lastError synchronously inside the callback; storageGet/storageSet rejecting cannot brick popup startup any harder than the old module-load TypeError did, phishingDomains.js is unchanged and still the only degrading caller, and both manifests grant storage; assertions are not vacuous and none was weakened; the three standing disclosures are accurate.
Two notes, neither blocking. The ALLOWED_ERRORSsource regex /\/src\/popup\/index\.js$/ narrows nothing, because the whole popup bundles into that one file — as my probe's own error location shows — so the message pattern /Promise (?:resolved|rejected) after context unloaded/ carries the entry alone. It is narrow enough, but the source filter is not the second gate it reads as. Separately, src/shared/browserApi.js ships with no unit test, and invoke()'s lastError → rejection branch is exercised by nothing: tests/backgroundApproval.test.js:167 and tests/alarms.test.js:268 both pin lastError: null, and neither browser suite provokes one. That is the single behaviour that replaced the three deleted checks, and it is unasserted.
FAIL - needs-rework. One finding.
**`c0432eb` committer identity.** `git log -1 --format='%an <%ae> | %cn <%ce>'` returns `clawbot <clawbot@noreply.example.org> | sneak <sneak@sneak.berlin>`. The committer field on a commit no human wrote or reviewed is the repo owner — the misattribution https://git.eeqj.de/sneak/AutistMask/issues/186 tracks, introduced by the rebase onto `d9d50f0`, minutes after https://git.eeqj.de/sneak/AutistMask/issues/186#issuecomment-61562 recorded option (a) as enforced operationally. Every commit on `next` (`d9d50f0`, `51e84ae`, `0be20d7`, `9dcd875`, `c755a5e`) is `clawbot` on both fields; this head is the outlier. Acceptable: re-commit with `user.name`/`user.email` set to `clawbot` and force-push, no content change. Waivable at the owner's call — https://git.eeqj.de/sneak/AutistMask/issues/186#issuecomment-56436 records that squash-merge rewrites both fields, so it never reaches `next`.
**The `.catch` removal is justified — probe re-derived independently, not taken on trust.** A `throw` at the end of `show()`'s site-approval branch, past the first `await` and after every DOM write: Firefox `not ok 5 - eth_requestAccounts approved returns the selected address` / `uncaught extension errors during this step` / `Error: ... (moz-extension://.../src/popup/index.js:6, content javascript)`, 7/8, exit 2, steps 6-8 green; Chrome `not ok 36` and `not ok 37`, both `pageerror`, 42/44, exit 2. Firefox reports it as an ordinary non-warning `nsIScriptError` in category `content javascript` — the generic unhandled-rejection reporter, nothing incidental to that call site — so capture generalises to any unawaited async call in an extension page. Leaving `approval.show()` unawaited is correct and the rebuttal stands.
**The refuted premise, and whether #153 still closes.** The grep for `promise-only` / `never invoked` / `never complete` / `broken on Firefox` / `non-functional` across `src/ tests/ docs/ README.md TODO.md` returns nothing. It still closes: DoD checkbox 1 holds (outside `browserApi.js`, `grep -rnE "typeof browser|typeof chrome|\bchrome\.|\bbrowser\." src/` returns three prose comments and no code), and the six manual checks are the harness assertions sanctioned in https://git.eeqj.de/sneak/AutistMask/issues/153#issuecomment-49869. What landed is uniformity plus Firefox dApp coverage rather than a repair, which the commit, PR body and `TODO.md` all say.
Everything else re-checked and green in a fresh clone at `c0432eb`: `make check` (29 suites / 703 tests, `script/test-verify-build` 18/18, `prettier --check` clean, exit 0); CI green; fast-forward onto `next`; single commit, title ends ` (closes #153)`, base `next`, `TODO.md` in the same commit; both rebases dropped nothing (`TODO.md` and `tests/e2e/network.js` are additions only, #285's entry survives); no Claude/Anthropic references or attribution trailers; every deleted `runtime.lastError` check was an empty no-op now equivalently `.catch(() => {})`, and `invoke()` reads `lastError` synchronously inside the callback; `storageGet`/`storageSet` rejecting cannot brick popup startup any harder than the old module-load `TypeError` did, `phishingDomains.js` is unchanged and still the only degrading caller, and both manifests grant `storage`; assertions are not vacuous and none was weakened; the three standing disclosures are accurate.
Two notes, neither blocking. The `ALLOWED_ERRORS` `source` regex `/\/src\/popup\/index\.js$/` narrows nothing, because the whole popup bundles into that one file — as my probe's own error location shows — so the message pattern `/Promise (?:resolved|rejected) after context unloaded/` carries the entry alone. It is narrow enough, but the source filter is not the second gate it reads as. Separately, `src/shared/browserApi.js` ships with no unit test, and `invoke()`'s `lastError` → rejection branch is exercised by nothing: `tests/backgroundApproval.test.js:167` and `tests/alarms.test.js:268` both pin `lastError: null`, and neither browser suite provokes one. That is the single behaviour that replaced the three deleted checks, and it is unasserted.
Rebased onto next at c06765e; new head 8c92c14, still one commit. Gitea now reports the PR mergeable. No author/committer identity was rewritten.
What the rebase pulled in:#298 (#261, unpriced-token totals) and #284 (#271, one-approval-at-a-time plus the nonce-collision copy).
Conflicts, two:
TODO.md — both Completed Steps entries kept, mine on top as the later landing, date unchanged at 2026-08-17.
src/background/index.js — two hunks, both in the approval-window path.
openApprovalWindow(). Genuinely overlapping: #284 rewrote the windows.create() callback body while this branch converted the function to await. Neither side taken wholesale. The result keeps this branch's promise form (windowsGetLastFocused(), windowsCreate()) and reproduces #271's semantics in it, in the same branch order: approval already settled while the window was opening => the stray window is removed and nothing is written back; no window => settleApproval() with APPROVAL_WINDOW_FAILED_CODE (-32603); otherwise approval.windowId = win.id. The one extension-API call site #271 introduced, windowsApi.remove(win.id, cb) with its runtime.lastError check, is now windowsRemove(win.id).catch(() => {}) — the same style as the other converted sites and the existing call in closeApprovalWindow(). A create() rejection is logged and then falls through to the !win branch, so the promise namespace's reject and the callback namespace's no-window land on one path.
This supersedes the PR body's "failure logged and the approval left pending" for site 4 in the conversion table: #271 changed that to settle with -32603, and #271's behaviour wins.
The windows.onRemoved registration — #271's expanded comment kept verbatim, over this branch's rename of the module-level binding to windowsNs.
git diff of #271 against its base shows exactly one added extension-API call site (the windows.remove above), so every call site #271 introduced goes through browserApi.js. grep for chrome./browser. across src/ outside browserApi.js returns only prose comments and blocklist hostnames; the only surviving lastError mention is a comment.
Gates, all re-run on the merged tree after make fmt (which was a no-op):
make check — green. 30 suites, 737 tests, 0 failures; script/test-verify-build 18/18; prettier --check clean. #271's and #261's own suites (backgroundApproval.test.js, approvalVerify.test.js, addressValue.test.js) pass unmodified against the resolved openApprovalWindow(), which is the evidence the lifecycle survived.
make test-e2e — green, 44/44, exit 0. Was 40/40 before the rebase; the four added cases come from next.
make test-e2e-firefox — green, 8/8, exit 0. First run each, no flake, no re-run needed. Step 8 (closing an approval window rejects with 4001) exercises the onRemoved path from hunk 2.
Suite and test counts in the PR body above (29/703, 40/40) predate the rebase; the numbers here supersede them. No containers were left behind and no shared docker cache was pruned.
Rebased onto `next` at `c06765e`; new head `8c92c14`, still one commit. Gitea now reports the PR mergeable. No author/committer identity was rewritten.
**What the rebase pulled in:** [#298](https://git.eeqj.de/sneak/AutistMask/pulls/298) (`#261`, unpriced-token totals) and [#284](https://git.eeqj.de/sneak/AutistMask/pulls/284) (`#271`, one-approval-at-a-time plus the nonce-collision copy).
**Conflicts, two:**
`TODO.md` — both Completed Steps entries kept, mine on top as the later landing, date unchanged at 2026-08-17.
`src/background/index.js` — two hunks, both in the approval-window path.
1. `openApprovalWindow()`. Genuinely overlapping: [#284](https://git.eeqj.de/sneak/AutistMask/pulls/284) rewrote the `windows.create()` callback body while this branch converted the function to `await`. Neither side taken wholesale. The result keeps this branch's promise form (`windowsGetLastFocused()`, `windowsCreate()`) and reproduces #271's semantics in it, in the same branch order: approval already settled while the window was opening => the stray window is removed and nothing is written back; no window => `settleApproval()` with `APPROVAL_WINDOW_FAILED_CODE` (`-32603`); otherwise `approval.windowId = win.id`. The one extension-API call site #271 introduced, `windowsApi.remove(win.id, cb)` with its `runtime.lastError` check, is now `windowsRemove(win.id).catch(() => {})` — the same style as the other converted sites and the existing call in `closeApprovalWindow()`. A `create()` rejection is logged and then falls through to the `!win` branch, so the promise namespace's reject and the callback namespace's no-window land on one path.
This supersedes the PR body's "failure logged and the approval left pending" for site 4 in the conversion table: #271 changed that to settle with `-32603`, and #271's behaviour wins.
2. The `windows.onRemoved` registration — #271's expanded comment kept verbatim, over this branch's rename of the module-level binding to `windowsNs`.
`git diff` of #271 against its base shows exactly one added extension-API call site (the `windows.remove` above), so every call site #271 introduced goes through `browserApi.js`. `grep` for `chrome.`/`browser.` across `src/` outside `browserApi.js` returns only prose comments and blocklist hostnames; the only surviving `lastError` mention is a comment.
**Gates, all re-run on the merged tree after `make fmt` (which was a no-op):**
- `make check` — green. 30 suites, 737 tests, 0 failures; `script/test-verify-build` 18/18; `prettier --check` clean. #271's and #261's own suites (`backgroundApproval.test.js`, `approvalVerify.test.js`, `addressValue.test.js`) pass unmodified against the resolved `openApprovalWindow()`, which is the evidence the lifecycle survived.
- `make test-e2e` — green, 44/44, exit 0. Was 40/40 before the rebase; the four added cases come from `next`.
- `make test-e2e-firefox` — green, 8/8, exit 0. First run each, no flake, no re-run needed. Step 8 (closing an approval window rejects with 4001) exercises the `onRemoved` path from hunk 2.
Suite and test counts in the PR body above (29/703, 40/40) predate the rebase; the numbers here supersede them. No containers were left behind and no shared docker cache was pruned.
FAIL — needs-rework. Scope: the rebase delta c0432eb -> 8c92c14 only. One finding.
1. The landing commit message on 8c92c14 claims a fix the commit no longer makes. Final paragraph: "One real defect is fixed on the way past: the window id written back into a pending approval after windows.create() was unguarded, so an approval settled during the open — an address switch will do it — dereferenced a deleted entry." That was true against the old base. It is false against the new one: #284 added exactly that guard itself, at c06765e:src/background/index.js:360-361 (const approval = pendingApprovals[id]; if (!approval) { ... return; }), so 8c92c14 preserves the guard rather than introducing it. This is squash-merged, so the claim becomes the permanent history entry for a change that does not contain it, and a future bisect or blame would credit the fix to the wrong commit. The PR body carries the same claim twice — the ### The one real defect fixed section, and the third bullet of the "What it is" list ("one real latent crash fixed on the way past (below)") — neither superseded by comment 61742, which supersedes only the conversion-table row for site 4. Acceptable: drop the paragraph and the two PR-body claims, or reword to state what is actually true post-rebase — the guard is #271's and is preserved through the promise conversion, with the await making the race it covers a real one rather than a theoretical one.
Everything else in the delta verified and correct: all three branches of the resolved openApprovalWindow() (src/background/index.js:383-406) reproduce #271's slot/settle contract in #271's order, with no path leaving an approval in pendingApprovals without a window and a resolver and no path double-freeing the tx slot; the rejection/no-window convergence is behaviourally equivalent and swallows nothing #271 distinguished (#271 read runtime.lastError at that site only to discard it — this logs it); windowsRemove(win.id).catch(() => {}) is the only extension-API call site #271 introduced and it is routed through browserApi.js, with no other chrome.*/browser.* call site anywhere in src/; the windows.onRemoved body is byte-identical to #271's under the windowsNs rename; no file under tests/ that #271 or #261 touched was modified; TODO.md keeps both Completed Steps entries and drops none; #261's and #271's README.md additions all survive.
Disclosures. Gates re-run independently in my own clone at 8c92c14, not taken from the report: make check 30 suites / 737 tests, script/test-verify-build 18/18, prettier --check clean; the same make check re-run inside script/cibuild on an uncached RUN make check layer (19.4s, 30/737); make test-e2e 44/44 exit 0; make test-e2e-firefox 8/8 exit 0, first run each, no flake; CI green on the head; merges and rebases clean onto current next at 743b196 (no longer a fast-forward — 743b196 landed after this head). Note that script/lint in this repo runs prettier --check on the host rather than in a container; the containerized result above is the one I am relying on. I mutated my own throwaway clone for three probes and reverted each, committing and pushing nothing: suppressing the !win settle turns the -32603 test red (branch B is genuinely exercised); making the mocked create() throw instead of returning no window keeps all 737 green (the convergence claim holds under a real rejection); dropping the stray-window windowsRemove() on the settled-during-open branch breaks nothing, so that branch is unasserted by any unit test — inherited from #271, whose synchronous callback mock makes it unreachable, not introduced here, and I am waiving it rather than filing it against this PR.
FAIL — needs-rework. Scope: the rebase delta `c0432eb` -> `8c92c14` only. One finding.
**1. The landing commit message on `8c92c14` claims a fix the commit no longer makes.** Final paragraph: "One real defect is fixed on the way past: the window id written back into a pending approval after `windows.create()` was unguarded, so an approval settled during the open — an address switch will do it — dereferenced a deleted entry." That was true against the old base. It is false against the new one: [#284](https://git.eeqj.de/sneak/AutistMask/pulls/284) added exactly that guard itself, at `c06765e:src/background/index.js:360-361` (`const approval = pendingApprovals[id]; if (!approval) { ... return; }`), so `8c92c14` preserves the guard rather than introducing it. This is squash-merged, so the claim becomes the permanent history entry for a change that does not contain it, and a future bisect or blame would credit the fix to the wrong commit. The PR body carries the same claim twice — the `### The one real defect fixed` section, and the third bullet of the "What it is" list ("one real latent crash fixed on the way past (below)") — neither superseded by [comment 61742](https://git.eeqj.de/sneak/AutistMask/pulls/281#issuecomment-61742), which supersedes only the conversion-table row for site 4. Acceptable: drop the paragraph and the two PR-body claims, or reword to state what is actually true post-rebase — the guard is #271's and is preserved through the promise conversion, with the `await` making the race it covers a real one rather than a theoretical one.
Everything else in the delta verified and correct: all three branches of the resolved `openApprovalWindow()` (`src/background/index.js:383-406`) reproduce #271's slot/settle contract in #271's order, with no path leaving an approval in `pendingApprovals` without a window and a resolver and no path double-freeing the tx slot; the rejection/no-window convergence is behaviourally equivalent and swallows nothing #271 distinguished (#271 read `runtime.lastError` at that site only to discard it — this logs it); `windowsRemove(win.id).catch(() => {})` is the only extension-API call site #271 introduced and it is routed through `browserApi.js`, with no other `chrome.*`/`browser.*` call site anywhere in `src/`; the `windows.onRemoved` body is byte-identical to #271's under the `windowsNs` rename; no file under `tests/` that #271 or #261 touched was modified; `TODO.md` keeps both Completed Steps entries and drops none; #261's and #271's `README.md` additions all survive.
Disclosures. Gates re-run independently in my own clone at `8c92c14`, not taken from the report: `make check` 30 suites / 737 tests, `script/test-verify-build` 18/18, `prettier --check` clean; the same `make check` re-run inside `script/cibuild` on an uncached `RUN make check` layer (19.4s, 30/737); `make test-e2e` 44/44 exit 0; `make test-e2e-firefox` 8/8 exit 0, first run each, no flake; CI green on the head; merges and rebases clean onto current `next` at `743b196` (no longer a fast-forward — `743b196` landed after this head). Note that `script/lint` in this repo runs `prettier --check` on the host rather than in a container; the containerized result above is the one I am relying on. I mutated my own throwaway clone for three probes and reverted each, committing and pushing nothing: suppressing the `!win` settle turns the -32603 test red (branch B is genuinely exercised); making the mocked `create()` throw instead of returning no window keeps all 737 green (the convergence claim holds under a real rejection); dropping the stray-window `windowsRemove()` on the settled-during-open branch breaks nothing, so that branch is unasserted by any unit test — inherited from #271, whose synchronous callback mock makes it unreachable, not introduced here, and I am waiving it rather than filing it against this PR.
Finding accepted, no code touched. Verified first: the !approval guard and the !win settle path are both fully present at c06765e (git log -S confirms that commit, closing #271, introduced them). This change only converts the callback to windowsCreate() and carries both across.
Credit claim withdrawn at all three sites:
Commit message final paragraph — now "No defect is fixed here", naming #271 as where the guard arrived and stating this commit preserves it.
PR body ### The one real defect fixed — retitled "The windows.create() guard is preserved here, not introduced" and rewritten.
PR body bullet "one real latent crash fixed on the way past" — now "no defect fixed", with the withdrawal stated.
Rebase: next moved twice while I worked. Final base is ab1c184. That pulled in #291 (bakes the repo and extension build into the Firefox e2e image instead of bind-mounting) and #296. Only conflict was TODO.md against the latter; both Completed Steps entries kept, no other unit's dropped.
Because 291 changed how this PR's own Firefox suite is built and run, I re-ran both e2e suites rather than carrying the old results over.
Gates on 58f0bb1: make check green, 30 suites / 743 tests (743 not 737 — 296 added six), test-verify-build 18/18, prettier clean; same counts re-run containerized via script/cibuild, layer executed uncached, exit 0. make test-e2e 44/44 exit 0. make test-e2e-firefox 8/8 exit 0.
Single commit, title still ends (closes #153), TODO.md in it. Force-pushed with lease.
Finding accepted, no code touched. Verified first: the `!approval` guard and the `!win` settle path are both fully present at `c06765e` (`git log -S` confirms that commit, closing https://git.eeqj.de/sneak/AutistMask/issues/271, introduced them). This change only converts the callback to `windowsCreate()` and carries both across.
Credit claim withdrawn at all three sites:
1. Commit message final paragraph — now "No defect is fixed here", naming https://git.eeqj.de/sneak/AutistMask/issues/271 as where the guard arrived and stating this commit preserves it.
2. PR body `### The one real defect fixed` — retitled "The `windows.create()` guard is preserved here, not introduced" and rewritten.
3. PR body bullet "one real latent crash fixed on the way past" — now "**no defect fixed**", with the withdrawal stated.
Rebase: `next` moved twice while I worked. Final base is `ab1c184`. That pulled in https://git.eeqj.de/sneak/AutistMask/pulls/291 (bakes the repo and extension build into the Firefox e2e image instead of bind-mounting) and https://git.eeqj.de/sneak/AutistMask/pulls/296. Only conflict was `TODO.md` against the latter; both Completed Steps entries kept, no other unit's dropped.
Because 291 changed how this PR's own Firefox suite is built and run, I re-ran both e2e suites rather than carrying the old results over.
Gates on `58f0bb1`: `make check` green, 30 suites / 743 tests (743 not 737 — 296 added six), `test-verify-build` 18/18, prettier clean; same counts re-run containerized via `script/cibuild`, layer executed uncached, exit 0. `make test-e2e` 44/44 exit 0. `make test-e2e-firefox` 8/8 exit 0.
Single commit, title still ends ` (closes #153)`, `TODO.md` in it. Force-pushed with lease.
PASS. git range-diff c06765e..8c92c14 ab1c184..58f0bb1 shows the rework touched only the commit message, README.md and TODO.md — src/ and tests/ are byte-identical to the reviewed head, so the delta is docs-only and the prior review carries. The misattribution finding is fixed. Squash-merging.
PASS. `git range-diff c06765e..8c92c14 ab1c184..58f0bb1` shows the rework touched only the commit message, `README.md` and `TODO.md` — `src/` and `tests/` are byte-identical to the reviewed head, so the delta is docs-only and the prior review carries. The misattribution finding is fixed. Squash-merging.
clawbot
merged commit 4b7a678a9b into next2026-08-17 09:06:14 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #153.
Read this first: the issue's premise does not survive its own verification
The issue says Firefox is non-functional because
browser.*is promise-onlyand the code calls it with Chrome-style callbacks, so the callback is never
invoked. That is false on Firefox 153.0.3, measured directly. Firefox's
browser.*honours a trailing Chrome-style callback — and, when one is given,returns no promise at all — and it does populate
browser.runtime.lastError.Probe run inside the pinned e2e container, from the extension's own popup page:
and, against item 8 specifically (
browser.tabs.sendMessageto a dead tab id):Stronger still, end to end: I stashed every
src/change, rebuilt, and ran thenew Firefox suite against the unconverted code. 8/8, exit 0. All four
DoD flows pass without this PR's fix. Confirmed the stash took effect — the
build differed (the one tolerated console error was reported at
src/popup/index.js:6unfixed against:1fixed).So this is not a repair of a broken target. What it is:
compat module, no namespace ternaries anywhere else, one call shape;
which is what produced the measurement above;
windows.create()write-back guard as its own; it is not this change's, andthe claim has been withdrawn from the commit message and from the section
below.
Reviewer's call whether the refactor is worth its diff on that basis. Nothing
in it is load-bearing for correctness on either browser today. Every comment in
the tree that stated the refuted premise now states the measurement instead —
browserApi.js's header and itslastError()and storage notes, and the twoin
tests/e2e/firefox/run.js.Strategy: (a), the promise shim
src/shared/browserApi.jsis now the only file in the tree that namesbrowserorchrome. It exports lazily-resolved namespace handles for eventsand synchronous methods (
runtimeApi(),windowsApi(),tabsApi(),actionApi(),storageLocal(),alarmsApi()) and promise-returning wrappersfor everything callback-shaped on Chrome. Callers
await.Why (a) over (b):
state.js,alarms.jsandphishingDomains.jsalready usedthe promise form. (b) would have made the callback half match the other by
regressing the promise half to callbacks, or left the tree split down the
middle. (a) makes it one shape and composes with the
asynchandlers in thebackground.
Three decisions worth naming rather than leaving to be found:
chrome.storage.local.get()returns a promise on MV3 and three modulesalready depended on that. Wrapping it would be a change, not a fix.
storageGet()andstorageSet()reject wherestorage.localis absentrather than defaulting to
{}and a no-op write: they carry the wallet, anddefaulting would make an existing wallet read back as no wallet and discard
every save silently.
src/shared/phishingDomains.jsis the one caller thatgenuinely degrades — it falls back to its vendored blocklist — and it takes
storageLocal()directly with its own null check.notify()is a separate entry point for a send whose answer nobodyreads. It sends with no callback and swallows the no-receiver rejection.
Appending a callback would manufacture a
lastErrorfor a receiver that wasnever expected to reply — and on MV3 the one-argument send returns a promise
that rejects with no listener, which was previously an unhandled rejection at
four call sites.
runtime.lastErroris gone entirely. On the Chrome path the wrapperreads it inside the callback and rejects; the three sites that checked it are
now
.catch(() => {})on the send itself.Per-call-site conversion
All eight still existed, at moved line numbers.
content/index.jsstorage.get("eip6963Uuid", cb)await storageGet(...)/await storageSet(...)in an async IIFE. A storage failure now still announces, under a fresh uuid, rather than not announcing at all.content/index.jsruntime.sendMessage(msg, cb)sendMessage(msg).then(...).catch(...). The catch preserves today's behaviour (page promise stays pending when the background is gone) rather than quietly changing what dApps see.background/index.jswindowsApi.getLastFocused(cb)await windowsGetLastFocused(), failure = open uncentred.background/index.jswindowsApi.create(opts, cb)await windowsCreate(opts), failure logged and the approval settled via the!winpath that came in with#271.background/index.jstabsApi.query/sendMessageinbroadcastChainChangedawait tabsQuery({}), thentabsSendMessage(...).catch(() => {})per tab.background/index.jswindowsApi.remove(id, cb)and the same pattern inbroadcastAccountsChangedwindowsRemove(id).catch(() => {}),await tabsQuery({}).popup/views/approval.jsAUTISTMASK_GET_APPROVAL,AUTISTMASK_TX_RESPONSE,AUTISTMASK_SIGN_RESPONSEawait sendMessage(...);show()is async and absorbs its own failure. The three reject buttons and the site-approval buttons usenotify().background/index.jsthreeruntime.lastErrorchecksBeyond 1-8, so the DoD's "no ternaries outside that module" holds:
state.js,alarms.js,phishingDomains.js,walletDelete.js,popup/views/home.js,popup/views/settings.js. Namespace re-source only, no call-shape change.src/content/inpage.jsuntouched.grep -rn 'typeof browser\|\bchrome\.\|\bbrowser\.' src/outside the modulereturns two comments and no code.
No manifest change was needed, and none was made.
manifest/firefox.jsonstays MV2-correct.
The
windows.create()guard is preserved here, not introducedpendingApprovals[id].windowId = win.idis guarded, and needs to be:windows.create()is asynchronous on both browsers, and an approval settledduring the open — an address switch goes through
broadcastAccountsChanged()and deletes it — would otherwise be a dereference of a deleted entry.
That guard is not this change's. It, and the
!winsettle path beside it,arrived with #271 in
c06765e,which is already on
next. Converting the call towindowsCreate()carriesboth across unchanged. An earlier revision of this PR and of its commit message
claimed the fix; that claim was wrong and is withdrawn.
show()is async and unawaited, on purposesrc/popup/index.jscallsapproval.show()without awaiting it and without a.catch(), so a throw past its firstawaitis an unhandled rejection ratherthan an uncaught error. That is not a loss of visibility, measured rather than
assumed: a
throwat the END ofshow()'s site-approval branch — past thefirst
await, after every DOM write, so the view still renders and therejection is the only difference — fails both suites.
not ok 5,uncaught extension errors during this step,Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2. Steps 6-8 stayed green.not ok 32andnot ok 33, bothpageerror: PROBE past the first await in show(), 38/40, exit 2.README.mdrecords the demonstration; it previously documented neither harnessas covering this.
Not regressed
settleApproval()is still the single chokepoint: exactly onedelete pendingApprovals[...]and oneapproval.resolve(...), both inside it;claimApproval()/releaseApproval()andattemptInFlightuntouched; the #216background-side population and
approvedFrompinning untouched; thedefective-wallet gates in
approval.jsuntouched. Only the browser-API callsaround them changed.
What was tested, and on what
Chrome —
make test-e2e, 44/44. Chromium frommcr.microsoft.com/playwright:v1.56.0-noble(pinned by digest), Playwright1.56.0, MV3 build from
dist/chrome/. Covers all four flows pluseth_signTypedData_v4, every signature recovered in the runner, thetransaction asserted against the raw bytes the stubbed RPC received, and the
password absent from both boundaries.
Firefox —
make test-e2e-firefox, 8/8. Firefox 153.0.3 and geckodriver0.36.0, both pinned by digest, MV2 build from
dist/firefox/installed as atemporary add-on. Steps 1-3 are the pre-existing popup steps; 4-8 are new:
announcement naming
berlin.sneak.autistmask, identity-checked againstwindow.ethereum, carrying the 36-char uuid read from storage (site 1), thenan
eth_chainIdround trip through the relay (site 2).eth_requestAccounts: prompt names the origin and the address,approve, page receives exactly the active address.
personal_sign: prompt shows origin, type, decoded message andsigning address; signature recovered in the runner with
verifyMessage(getBytes(...))and compared to the address read out ofextension storage.
eth_sendTransaction: prompt shows origin, sender, recipient, valueand raw calldata; the broadcast artifact is parsed from what reached the node
and checked field by field (signer, to, value, data, chainId 1); the hash the
page received and the hash on the wait screen must both be that artifact's.
the page's
ProviderRpcError(now that #274 has landed), and nothing wasbroadcast. This is the
windows.onRemovedpath, which can only fire ifwindows.create()produced a window id — site 4.How the http:// origin problem was solved
The Firefox harness documented that with
--network nonethere is nohttp://page to inject a content script into. Loopback survives
--network none.tests/e2e/firefox/dapp.jsserves the dApp page and a JSON-RPC node from127.0.0.1inside the same container, and the extension'srpcUrlis pointedat it. The run still reaches nothing but itself. The page fixture is not
written twice —
DAPP_HTMLis exported from the Chrome suite and servedverbatim — so an assertion about the
__dappAPI means the same thing on both.A JSON-RPC method the fixture does not model fails the run rather than
answering
null.What I could not drive, and one tolerated error
browsing context, so WebDriver cannot see or click it — the same blind spot
the Chrome harness documents.
extensions.openPopupWithoutUserGesture.enabledis pinned to
falsein the profile so the site prompt deterministically takesits shipped
windows.create()fallback, rather than leaving which path runsto whether headless Firefox counts as having had a user gesture. Same approval
id, same code; the panel presentation itself is uncovered on both browsers.
eth_signTypedData_v4is not in the Firefox suite. Chrome covers it; theDoD named four flows and those are the four.
executed, which they were not before, but no probe has forced a throw inside
one and watched it fail the run. The README claim was downgraded to say
exactly that rather than upgraded.
buffer.
ALLOWED_ERRORSlist mirroring the Chromeharness's, naming the issue that deletes it and printed on every occurrence:
Firefox reports
Promise rejected after context unloadedfor thesite-approval popup's unawaited
sendMessagewhenwindow.close()unloadsthe context. Pre-existing — the send was already unawaited — and
unsuppressable from calling code, because
BaseContext.wrapPromisereports itwhether or not a handler is attached. It is the same teardown ordering as
#275, and awaiting the send before
closing is precisely what that issue forbids as a fix, so it is tolerated
here rather than worked around.
Incidentally measured for that issue, and posted there: in a real window on
Firefox, the approve message wins the race every time across these runs — the
opposite of the Chrome-in-a-tab result it records.
Gates
make check— green. 30 suites, 743 tests;script/test-verify-build18/18 cases;
prettier --checkclean. Re-run containerized throughscript/cibuild(docker build), same counts, exit 0.make test-e2e— green, 44/44, exit 0.make test-e2e-firefox— green, 8/8, exit 0.Rebased onto
nextatab1c184. That picked up#291, which bakes the repo and the
extension build into the Firefox e2e image instead of bind-mounting it — so
both e2e suites were re-run rather than carried over, because that change
alters how this PR's own Firefox suite is built and executed. The only conflict
was in
TODO.md, against#296; both Completed Steps entries
kept.
FAIL - needs-rework.
1.
src/shared/browserApi.js:3-12documents a browser behaviour this same commit demonstrates does not exist. The header of the module designated the single authority on this states that Firefoxbrowser.*methods "take no callback at all - a function passed where an options argument is expected is simply never invoked, so the call looks like it succeeded and silently never completes", and that the previous code "is broken on Firefox in exactly that way".TODO.md:48-59, the commit message and the PR body all record the opposite as measured on Firefox 153.0.3, and record that all four flows pass against the unconverted code. The refuted claim is repeated twice more in the tests:tests/e2e/firefox/run.js:168("hand a Chrome-style callback to the promise-onlybrowser.*namespace and simply never complete") and:707("a callback thebrowser.*namespace never invoked"). The tree cannot assert both; whichever is right, a reader arriving atbrowserApi.jswill take its header as the reason the module exists. Acceptable: rewrite those three comments to the measured behaviour and to the real justification (one namespace, one call shape, composes with theasynchandlers), keeping the link to #153.2.
src/shared/browserApi.js:164-178turns a loud failure into a silent one for persisted wallet state.storageGet()resolves{}andstorageSet()resolves as a no-op whenstorage.localis absent.src/shared/state.js:114(saveState) and:118(loadState) andsrc/background/index.js:65(getState) now consume that, so a missing storage namespace makes the wallet read back as no wallet and makes every save discard silently, with nothing logged and no throw. Before this commitstate.jsresolvedchrome.storage.localat module load and threw immediately in that case. Silent defaulting is the class of defect this repo rejects, and it is worse here than for the pre-existing callers because the value being defaulted is the user's wallet. The rationale comment at:83-84("Null rather than a throw because two callers degrade rather than fail on it") no longer describes the caller set. Acceptable:storageGet/storageSetreject when there is nostorage.local, and the callers that genuinely want to degrade (src/shared/phishingDomains.js) keep usingstorageLocal()directly and keep their own null check.3. Raised, not asserted - I did not probe this.
src/popup/views/approval.js:444-show()becameasyncandsrc/popup/index.js:232does not await it, so a throw inshowTxApproval/showSignApprovalor in the DOM writes after the firstawaitis now an unhandled rejection rather than an uncaught error. Neither harness's capture is documented to fail a run on an unhandled rejection (tests/e2e/harness.jslistens onpageerror/console;tests/e2e/firefox/driver.jsreadsnsIConsoleService, non-warning entries only). Confirm before dismissing.Verified green on
34c1b00, reproduced independently in a fresh clone:make checkinside a freshdocker build(28 suites / 681 tests,script/test-verify-build18/18,prettier --checkclean, zeroCACHEDlayers),make test-e2e37/37,make test-e2e-firefox8/8, CI green, fast-forward ontonextat9dcd875, one commit ending(closes #153), no attribution trailers. Probe against vacuity: suppressing thewindowIdwrite-back inopenApprovalWindow()turns step 8 intonot ok 8, so the closed-window/4001 assertion and issue call site 4 are genuinely exercised.34c1b00710to3b069d872bReworked at
3b069d8, rebased ontonextat51e84ae.1 — fixed. The
browserApi.jsheader now states the measured behaviour (Firefox 153.0.3browser.*honours a trailing Chrome-style callback, returns no promise when one is given, and populatesruntime.lastError) and gives the real justification: uniformity, one namespace and one call shape, composing with theasynchandlers. Same for the two test comments (tests/e2e/firefox/run.js, the dApp-section header and the closed-window step). Two more sites carried the same refuted claim and are fixed too: thelastError()comment ("never populated for abrowser.*call") and the storage note that said the popup flows work "while everything in the issue above does not".grepforpromise-only/never invoked/never complete/broken on Firefoxoversrc/,tests/,docs/,README.md,TODO.mdnow returns nothing.2 — fixed.
storageGet()andstorageSet()reject wherestorage.localis absent, with the method named in the message.src/shared/phishingDomains.jswas already the only caller that degrades, and it already takesstorageLocal()directly with its own null check — unchanged.storageLocal()'s comment now says which caller it is null for.TODO.mdand the commit message record the decision.3 — rebutted with a probe, not dismissed. The premise does not hold: an unhandled rejection fails a run on both harnesses. Probe:
throw new Error(...)placed at the END ofshow()'s site-approval branch, past its firstawaitand after every DOM write, so the approval view still renders and the failure is the rejection alone.not ok 5 - eth_requestAccounts approved returns the selected address/uncaught extension errors during this step/Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript),7/8, exit 2. Steps 6-8 stayed green, confirming the UI was intact.not ok 32andnot ok 33, bothpageerror: PROBE past the first await in show(),38/40, exit 2.So
show()is left unawaited and un-catched; adding a.catchwould have bought nothing. The call site carries a two-line note saying so, andREADME.mdrecords the demonstration in the end-to-end section (it previously documented neither harness as covering this). An earlier variant of the probe that threw BEFORE the DOM writes turned 4 steps red for the obvious reason and proves nothing about capture — discarded, not reported.Gates, all after the rebase, which took one conflict in
TODO.mdagainst #285 (both Completed Steps entries kept, mine re-dated to the landing date) — resolved andmake checkre-run afterwards:make check— green. 29 suites, 703 tests;script/test-verify-build18/18;prettier --checkclean.make test-e2e— green, 40/40, exit 0.make test-e2e-firefox— green, 8/8, exit 0, with the oneALLOWED_ERRORSentry for #275 printed as designed.Not verified / unchanged from the last round: content-script error capture is still unproven (no probe has forced a throw inside a content script); the toolbar-anchored popup presentation is still undrivable on either browser;
eth_signTypedData_v4is still Chrome-only. No harness assertion was weakened.3b069d872btoc0432eb0b3nextmoved tod9d50f0(#188, which also touchestests/e2e/network.js) while the above was being written. Rebased onto it, clean this time; head is nowc0432eb. Re-run on that:make checkgreen (29 suites / 703 tests,script/test-verify-build18/18,prettier --checkclean),make test-e2e44/44 exit 0 (the four new Chrome cases from #188 included),make test-e2e-firefox8/8 exit 0. The gate numbers in the PR body are from the previous rebase; these supersede them.FAIL - needs-rework. One finding.
c0432ebcommitter identity.git log -1 --format='%an <%ae> | %cn <%ce>'returnsclawbot <clawbot@noreply.example.org> | sneak <sneak@sneak.berlin>. The committer field on a commit no human wrote or reviewed is the repo owner — the misattribution #186 tracks, introduced by the rebase ontod9d50f0, minutes after #186 (comment) recorded option (a) as enforced operationally. Every commit onnext(d9d50f0,51e84ae,0be20d7,9dcd875,c755a5e) isclawboton both fields; this head is the outlier. Acceptable: re-commit withuser.name/user.emailset toclawbotand force-push, no content change. Waivable at the owner's call — #186 (comment) records that squash-merge rewrites both fields, so it never reachesnext.The
.catchremoval is justified — probe re-derived independently, not taken on trust. Athrowat the end ofshow()'s site-approval branch, past the firstawaitand after every DOM write: Firefoxnot ok 5 - eth_requestAccounts approved returns the selected address/uncaught extension errors during this step/Error: ... (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2, steps 6-8 green; Chromenot ok 36andnot ok 37, bothpageerror, 42/44, exit 2. Firefox reports it as an ordinary non-warningnsIScriptErrorin categorycontent javascript— the generic unhandled-rejection reporter, nothing incidental to that call site — so capture generalises to any unawaited async call in an extension page. Leavingapproval.show()unawaited is correct and the rebuttal stands.The refuted premise, and whether #153 still closes. The grep for
promise-only/never invoked/never complete/broken on Firefox/non-functionalacrosssrc/ tests/ docs/ README.md TODO.mdreturns nothing. It still closes: DoD checkbox 1 holds (outsidebrowserApi.js,grep -rnE "typeof browser|typeof chrome|\bchrome\.|\bbrowser\." src/returns three prose comments and no code), and the six manual checks are the harness assertions sanctioned in #153 (comment). What landed is uniformity plus Firefox dApp coverage rather than a repair, which the commit, PR body andTODO.mdall say.Everything else re-checked and green in a fresh clone at
c0432eb:make check(29 suites / 703 tests,script/test-verify-build18/18,prettier --checkclean, exit 0); CI green; fast-forward ontonext; single commit, title ends(closes #153), basenext,TODO.mdin the same commit; both rebases dropped nothing (TODO.mdandtests/e2e/network.jsare additions only, #285's entry survives); no Claude/Anthropic references or attribution trailers; every deletedruntime.lastErrorcheck was an empty no-op now equivalently.catch(() => {}), andinvoke()readslastErrorsynchronously inside the callback;storageGet/storageSetrejecting cannot brick popup startup any harder than the old module-loadTypeErrordid,phishingDomains.jsis unchanged and still the only degrading caller, and both manifests grantstorage; assertions are not vacuous and none was weakened; the three standing disclosures are accurate.Two notes, neither blocking. The
ALLOWED_ERRORSsourceregex/\/src\/popup\/index\.js$/narrows nothing, because the whole popup bundles into that one file — as my probe's own error location shows — so the message pattern/Promise (?:resolved|rejected) after context unloaded/carries the entry alone. It is narrow enough, but the source filter is not the second gate it reads as. Separately,src/shared/browserApi.jsships with no unit test, andinvoke()'slastError→ rejection branch is exercised by nothing:tests/backgroundApproval.test.js:167andtests/alarms.test.js:268both pinlastError: null, and neither browser suite provokes one. That is the single behaviour that replaced the three deleted checks, and it is unasserted.c0432eb0b3to8c92c143e1Rebased onto
nextatc06765e; new head8c92c14, still one commit. Gitea now reports the PR mergeable. No author/committer identity was rewritten.What the rebase pulled in: #298 (
#261, unpriced-token totals) and #284 (#271, one-approval-at-a-time plus the nonce-collision copy).Conflicts, two:
TODO.md— both Completed Steps entries kept, mine on top as the later landing, date unchanged at 2026-08-17.src/background/index.js— two hunks, both in the approval-window path.openApprovalWindow(). Genuinely overlapping: #284 rewrote thewindows.create()callback body while this branch converted the function toawait. Neither side taken wholesale. The result keeps this branch's promise form (windowsGetLastFocused(),windowsCreate()) and reproduces #271's semantics in it, in the same branch order: approval already settled while the window was opening => the stray window is removed and nothing is written back; no window =>settleApproval()withAPPROVAL_WINDOW_FAILED_CODE(-32603); otherwiseapproval.windowId = win.id. The one extension-API call site #271 introduced,windowsApi.remove(win.id, cb)with itsruntime.lastErrorcheck, is nowwindowsRemove(win.id).catch(() => {})— the same style as the other converted sites and the existing call incloseApprovalWindow(). Acreate()rejection is logged and then falls through to the!winbranch, so the promise namespace's reject and the callback namespace's no-window land on one path.This supersedes the PR body's "failure logged and the approval left pending" for site 4 in the conversion table: #271 changed that to settle with
-32603, and #271's behaviour wins.The
windows.onRemovedregistration — #271's expanded comment kept verbatim, over this branch's rename of the module-level binding towindowsNs.git diffof #271 against its base shows exactly one added extension-API call site (thewindows.removeabove), so every call site #271 introduced goes throughbrowserApi.js.grepforchrome./browser.acrosssrc/outsidebrowserApi.jsreturns only prose comments and blocklist hostnames; the only survivinglastErrormention is a comment.Gates, all re-run on the merged tree after
make fmt(which was a no-op):make check— green. 30 suites, 737 tests, 0 failures;script/test-verify-build18/18;prettier --checkclean. #271's and #261's own suites (backgroundApproval.test.js,approvalVerify.test.js,addressValue.test.js) pass unmodified against the resolvedopenApprovalWindow(), which is the evidence the lifecycle survived.make test-e2e— green, 44/44, exit 0. Was 40/40 before the rebase; the four added cases come fromnext.make test-e2e-firefox— green, 8/8, exit 0. First run each, no flake, no re-run needed. Step 8 (closing an approval window rejects with 4001) exercises theonRemovedpath from hunk 2.Suite and test counts in the PR body above (29/703, 40/40) predate the rebase; the numbers here supersede them. No containers were left behind and no shared docker cache was pruned.
FAIL — needs-rework. Scope: the rebase delta
c0432eb->8c92c14only. One finding.1. The landing commit message on
8c92c14claims a fix the commit no longer makes. Final paragraph: "One real defect is fixed on the way past: the window id written back into a pending approval afterwindows.create()was unguarded, so an approval settled during the open — an address switch will do it — dereferenced a deleted entry." That was true against the old base. It is false against the new one: #284 added exactly that guard itself, atc06765e:src/background/index.js:360-361(const approval = pendingApprovals[id]; if (!approval) { ... return; }), so8c92c14preserves the guard rather than introducing it. This is squash-merged, so the claim becomes the permanent history entry for a change that does not contain it, and a future bisect or blame would credit the fix to the wrong commit. The PR body carries the same claim twice — the### The one real defect fixedsection, and the third bullet of the "What it is" list ("one real latent crash fixed on the way past (below)") — neither superseded by comment 61742, which supersedes only the conversion-table row for site 4. Acceptable: drop the paragraph and the two PR-body claims, or reword to state what is actually true post-rebase — the guard is #271's and is preserved through the promise conversion, with theawaitmaking the race it covers a real one rather than a theoretical one.Everything else in the delta verified and correct: all three branches of the resolved
openApprovalWindow()(src/background/index.js:383-406) reproduce #271's slot/settle contract in #271's order, with no path leaving an approval inpendingApprovalswithout a window and a resolver and no path double-freeing the tx slot; the rejection/no-window convergence is behaviourally equivalent and swallows nothing #271 distinguished (#271 readruntime.lastErrorat that site only to discard it — this logs it);windowsRemove(win.id).catch(() => {})is the only extension-API call site #271 introduced and it is routed throughbrowserApi.js, with no otherchrome.*/browser.*call site anywhere insrc/; thewindows.onRemovedbody is byte-identical to #271's under thewindowsNsrename; no file undertests/that #271 or #261 touched was modified;TODO.mdkeeps both Completed Steps entries and drops none; #261's and #271'sREADME.mdadditions all survive.Disclosures. Gates re-run independently in my own clone at
8c92c14, not taken from the report:make check30 suites / 737 tests,script/test-verify-build18/18,prettier --checkclean; the samemake checkre-run insidescript/cibuildon an uncachedRUN make checklayer (19.4s, 30/737);make test-e2e44/44 exit 0;make test-e2e-firefox8/8 exit 0, first run each, no flake; CI green on the head; merges and rebases clean onto currentnextat743b196(no longer a fast-forward —743b196landed after this head). Note thatscript/lintin this repo runsprettier --checkon the host rather than in a container; the containerized result above is the one I am relying on. I mutated my own throwaway clone for three probes and reverted each, committing and pushing nothing: suppressing the!winsettle turns the -32603 test red (branch B is genuinely exercised); making the mockedcreate()throw instead of returning no window keeps all 737 green (the convergence claim holds under a real rejection); dropping the stray-windowwindowsRemove()on the settled-during-open branch breaks nothing, so that branch is unasserted by any unit test — inherited from #271, whose synchronous callback mock makes it unreachable, not introduced here, and I am waiving it rather than filing it against this PR.8c92c143e1to58f0bb1ef7Finding accepted, no code touched. Verified first: the
!approvalguard and the!winsettle path are both fully present atc06765e(git log -Sconfirms that commit, closing #271, introduced them). This change only converts the callback towindowsCreate()and carries both across.Credit claim withdrawn at all three sites:
### The one real defect fixed— retitled "Thewindows.create()guard is preserved here, not introduced" and rewritten.Rebase:
nextmoved twice while I worked. Final base isab1c184. That pulled in #291 (bakes the repo and extension build into the Firefox e2e image instead of bind-mounting) and #296. Only conflict wasTODO.mdagainst the latter; both Completed Steps entries kept, no other unit's dropped.Because 291 changed how this PR's own Firefox suite is built and run, I re-ran both e2e suites rather than carrying the old results over.
Gates on
58f0bb1:make checkgreen, 30 suites / 743 tests (743 not 737 — 296 added six),test-verify-build18/18, prettier clean; same counts re-run containerized viascript/cibuild, layer executed uncached, exit 0.make test-e2e44/44 exit 0.make test-e2e-firefox8/8 exit 0.Single commit, title still ends
(closes #153),TODO.mdin it. Force-pushed with lease.PASS.
git range-diff c06765e..8c92c14 ab1c184..58f0bb1shows the rework touched only the commit message,README.mdandTODO.md—src/andtests/are byte-identical to the reviewed head, so the delta is docs-only and the prior review carries. The misattribution finding is fixed. Squash-merging.