refactor: one shared extension-API module, and drive the dApp flows on Firefox (closes #153) #281

clawbot · 2026-08-12T13:57:00+02:00

clawbot commented

2026-08-12 13:57:00 +02:00

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-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;
one real latent crash fixed on the way past (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.

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, which is exactly why the popup wallet flows worked on
Firefox. (b) would have made the callback half "correct" by regressing the
working 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.
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 left pending.
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 one real defect fixed

pendingApprovals[id].windowId = win.id was unguarded. windows.create() is
asynchronous on both browsers, and an approval settled during the open — an
address switch goes through broadcastAccountsChanged() and deletes it — makes
that a dereference of a deleted entry. Guarded now, with the reason at the site.

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, 37/37. 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. 28 suites, 681 tests; script/test-verify-build
18/18 cases; prettier --check clean.
make test-e2e — green, 37/37.
make test-e2e-firefox — green, 8/8.

All three re-run after the rebase onto next at 9dcd875.

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; - one real latent crash fixed on the way past (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. ## 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, which is exactly why the popup wallet flows worked on Firefox. (b) would have made the callback half "correct" by regressing the working 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. - **`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 left pending. | | 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 one real defect fixed `pendingApprovals[id].windowId = win.id` was unguarded. `windows.create()` is asynchronous on both browsers, and an approval settled during the open — an address switch goes through `broadcastAccountsChanged()` and deletes it — makes that a dereference of a deleted entry. Guarded now, with the reason at the site. ### 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`, 37/37.** 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.** 28 suites, 681 tests; `script/test-verify-build` 18/18 cases; `prettier --check` clean. - `make test-e2e` — **green, 37/37.** - `make test-e2e-firefox` — **green, 8/8.** All three re-run after the rebase onto `next` at `9dcd875`.

clawbot added 1 commit 2026-08-12 13:57:01 +02:00

refactor: one shared extension-API module, and drive the dApp flows on Firefox (closes #153 )

check / check (push) Successful in 28s

Details

34c1b00710

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.

clawbot added the needs-review label 2026-08-14 06:00:54 +02:00

clawbot self-assigned this 2026-08-14 06:00:54 +02:00

clawbot commented

2026-08-14 06:06:30 +02:00

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 #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.

clawbot added needs-rework and removed needs-review labels 2026-08-14 06:07:04 +02:00

check / check (push) Successful in 28s

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/issue-153-browser-api-compat:fix/issue-153-browser-api-compat

git checkout fix/issue-153-browser-api-compat

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#281