sneak/AutistMask
1
0
Fork 0
Code Issues 67 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

fix: Firefox target is non-functional — Chrome callback APIs used against the promise-only browser namespace #153

New Issue
Closed
opened 2026-08-09 03:42:55 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:42:55 +02:00
Collaborator
Copy Link
Copy Source

Problem

Firefox is half the reason this project exists (README.md:12: "None of the
common alternatives work on Firefox"), and docs/README.md promises "Work on
both Chrome and Firefox. Same codebase, same features, both browsers." On
main, the Firefox build is produced but dApp connectivity, all approval
popups, and chain/account event broadcasts are non-functional.

build.js does emit a coherent MV2 artifact — manifest/firefox.json uses
browser_action, a background.scripts array, flat
web_accessible_resources, and browser_specific_settings.gecko.id, and it
correctly omits "world": "MAIN" in favour of runtime injection at
src/content/index.js:7-14. The manifest is not the problem.

The problem is that the code resolves to the browser.* namespace on Firefox
(the typeof browser !== "undefined" ternaries at background/index.js:25-35,
content/index.js:42-43, approval.js:18-19) and then calls it with
Chrome-style callbacks. browser.* is promise-only; passing a function
where an options/getInfo argument is expected does not invoke the callback.

Affected call sites:

  1. src/content/index.js:23 — storage.get("eip6963Uuid", (items) => {...}).
    Breaks EIP-6963 UUID generation/announcement.
  2. src/content/index.js:45-55 — runtime.sendMessage({...}, (response) => {...}).
    This is the entire page-to-background RPC relay; every window.ethereum
    request from a dApp fails.
  3. src/background/index.js:113 — windowsApi.getLastFocused(cb).
  4. src/background/index.js:128 — windowsApi.create(opts, cb). This is also
    where pendingApprovals[id].windowId is assigned, so the
    window-closed-equals-rejection listener at background/index.js:622-640
    can never match.
  5. src/background/index.js:520-534 — tabsApi.query({}, cb) /
    tabsApi.sendMessage(id, msg, cb) in broadcastChainChanged.
  6. src/background/index.js:555 — windowsApi.remove(windowId, cb);
    :567-590 — same pattern in broadcastAccountsChanged.
  7. src/popup/views/approval.js:375 (AUTISTMASK_GET_APPROVAL), :443-460
    (AUTISTMASK_TX_RESPONSE), :482-501 (AUTISTMASK_SIGN_RESPONSE) — the
    approval popup can never load its details or receive the tx hash.
  8. background/index.js:530, :557, :584 check runtime.lastError, which
    is a chrome.* concept and is never populated for browser.* calls.

Implementation requirements

  • Introduce one shared extension-API compatibility module under
    src/shared/ (e.g. src/shared/browserApi.js) and route every call site
    above through it. Do not scatter per-call-site ternaries further.
  • Pick one of the two coherent strategies and apply it consistently; state
    which and why in the PR:
    • (a) Promise shim — the module exposes promise-returning wrappers
      (sendMessage, storageGet, tabsQuery, windowsCreate, …) that use
      browser.* directly where available and wrap chrome.* callbacks in
      new Promise otherwise. Convert the call sites to await.
    • (b) Force the chrome.* namespace — Firefox aliases chrome.* with
      callback semantics, so preferring chrome.* on both browsers makes the
      existing callback code correct as-written. Lower diff, but leaves
      callback style in place.
      Option (a) is the cleaner long-term shape and composes better with the
      existing async code; option (b) is the smaller, lower-risk change. Either
      is acceptable if applied uniformly.
  • runtime.lastError handling must be replaced with whatever the chosen
    strategy makes correct (rejected promise, or retained for the chrome path).
  • Do not regress Chrome MV3. Both targets must work.
  • Keep manifest/firefox.json MV2-correct; no manifest changes should be
    needed, but say so explicitly if any are.
  • This issue is scoped to the namespace/callback breakage only. The MV3
    service-worker setInterval problem and the localStorage-in-a-worker
    problem are tracked separately — do not fold them in.

Definition of done

  • A single shared compat module exists and every call site listed above
    (1-8) goes through it. No remaining
    typeof browser !== "undefined" ternaries outside that module.
  • Manually verified on Firefox with the temporary add-on loaded from
    dist/firefox/: connect a dApp via eth_requestAccounts and get the
    approval popup; approve; the site receives the account.
  • Manually verified on Firefox: a transaction approval popup opens, shows
    details, and returns the tx hash to the page.
  • Manually verified on Firefox: a personal_sign approval popup opens and
    returns a signature.
  • Manually verified on Firefox: closing an approval window rejects the
    pending request with EIP-1193 code 4001.
  • Manually verified on Chrome that all four flows above still work.
  • The PR body records what was tested on each browser and the browser
    versions used.
  • TODO.md updated in the same commit.
  • make check passes.
## Problem Firefox is half the reason this project exists (`README.md:12`: "None of the common alternatives work on Firefox"), and `docs/README.md` promises "Work on both Chrome and Firefox. Same codebase, same features, both browsers." On `main`, the Firefox build is produced but **dApp connectivity, all approval popups, and chain/account event broadcasts are non-functional**. `build.js` does emit a coherent MV2 artifact — `manifest/firefox.json` uses `browser_action`, a `background.scripts` array, flat `web_accessible_resources`, and `browser_specific_settings.gecko.id`, and it correctly omits `"world": "MAIN"` in favour of runtime injection at `src/content/index.js:7-14`. The manifest is not the problem. The problem is that the code resolves to the `browser.*` namespace on Firefox (the `typeof browser !== "undefined"` ternaries at `background/index.js:25-35`, `content/index.js:42-43`, `approval.js:18-19`) and then calls it with Chrome-style **callbacks**. `browser.*` is promise-only; passing a function where an options/getInfo argument is expected does not invoke the callback. Affected call sites: 1. `src/content/index.js:23` — `storage.get("eip6963Uuid", (items) => {...})`. Breaks EIP-6963 UUID generation/announcement. 2. `src/content/index.js:45-55` — `runtime.sendMessage({...}, (response) => {...})`. **This is the entire page-to-background RPC relay**; every `window.ethereum` request from a dApp fails. 3. `src/background/index.js:113` — `windowsApi.getLastFocused(cb)`. 4. `src/background/index.js:128` — `windowsApi.create(opts, cb)`. This is also where `pendingApprovals[id].windowId` is assigned, so the window-closed-equals-rejection listener at `background/index.js:622-640` can never match. 5. `src/background/index.js:520-534` — `tabsApi.query({}, cb)` / `tabsApi.sendMessage(id, msg, cb)` in `broadcastChainChanged`. 6. `src/background/index.js:555` — `windowsApi.remove(windowId, cb)`; `:567-590` — same pattern in `broadcastAccountsChanged`. 7. `src/popup/views/approval.js:375` (`AUTISTMASK_GET_APPROVAL`), `:443-460` (`AUTISTMASK_TX_RESPONSE`), `:482-501` (`AUTISTMASK_SIGN_RESPONSE`) — the approval popup can never load its details or receive the tx hash. 8. `background/index.js:530`, `:557`, `:584` check `runtime.lastError`, which is a `chrome.*` concept and is never populated for `browser.*` calls. ## Implementation requirements - Introduce **one** shared extension-API compatibility module under `src/shared/` (e.g. `src/shared/browserApi.js`) and route every call site above through it. Do not scatter per-call-site ternaries further. - Pick one of the two coherent strategies and apply it consistently; state which and why in the PR: - **(a) Promise shim** — the module exposes promise-returning wrappers (`sendMessage`, `storageGet`, `tabsQuery`, `windowsCreate`, …) that use `browser.*` directly where available and wrap `chrome.*` callbacks in `new Promise` otherwise. Convert the call sites to `await`. - **(b) Force the `chrome.*` namespace** — Firefox aliases `chrome.*` with callback semantics, so preferring `chrome.*` on both browsers makes the existing callback code correct as-written. Lower diff, but leaves callback style in place. Option (a) is the cleaner long-term shape and composes better with the existing `async` code; option (b) is the smaller, lower-risk change. Either is acceptable if applied uniformly. - `runtime.lastError` handling must be replaced with whatever the chosen strategy makes correct (rejected promise, or retained for the chrome path). - Do not regress Chrome MV3. Both targets must work. - Keep `manifest/firefox.json` MV2-correct; no manifest changes should be needed, but say so explicitly if any are. - This issue is scoped to the namespace/callback breakage only. The MV3 service-worker `setInterval` problem and the `localStorage`-in-a-worker problem are tracked separately — do not fold them in. ## Definition of done - [ ] A single shared compat module exists and every call site listed above (1-8) goes through it. No remaining `typeof browser !== "undefined"` ternaries outside that module. - [ ] Manually verified on Firefox with the temporary add-on loaded from `dist/firefox/`: connect a dApp via `eth_requestAccounts` and get the approval popup; approve; the site receives the account. - [ ] Manually verified on Firefox: a transaction approval popup opens, shows details, and returns the tx hash to the page. - [ ] Manually verified on Firefox: a `personal_sign` approval popup opens and returns a signature. - [ ] Manually verified on Firefox: closing an approval window rejects the pending request with EIP-1193 code 4001. - [ ] Manually verified on Chrome that all four flows above still work. - [ ] The PR body records what was tested on each browser and the browser versions used. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:42:55 +02:00
clawbot commented 2026-08-09 16:28:23 +02:00
Author
Collaborator
Copy Link
Copy Source

Your six manual checks are now machine-verifiable, and the premise here still stands

Two separate things, and it matters that they are kept apart.

1. The DoD is no longer blocked on a human

This issue's definition of done is six manual browser checks, four of them on
Firefox. #173 claimed no agent could
perform them. That claim was wrong for Chrome and it is also wrong for
Firefox: a containerized Firefox 153 driven by geckodriver installs the MV2
build from dist/firefox/ as a temporary add-on and drives the popup, and it
has been demonstrated failing and then passing on a real bug. Full detail and
the pinning requirements are in
#184.

So the four Firefox checks and the two Chrome checks in the DoD above become
automated assertions in the harnesses rather than a QA pass by you.

2. But this issue's own premise was NOT written from a bad assumption

The Firefox probe reported, as an incidental finding, that the popup is "not
non-functional" under Firefox - wallet creation, BIP-39, libsodium encryption,
HD derivation, state persistence and render all complete with zero console
errors - and suggested this issue needed re-grounding.

I checked that before repeating it, and it does not refute this issue. The
probe walked the popup wallet-management paths. Those go through
src/shared/state.js, which resolves browser.storage.local and uses it in
its promise form - correct on Firefox, which is exactly why that flow works
cleanly.

Every call site enumerated as 1-8 in the issue body is somewhere else: the
content script, the background page, and approval.js. Those use the
callback form, and the probe never walked them, because reaching them
requires a dApp page speaking EIP-1193 rather than the popup UI. Confirmed
directly - src/content/index.js:23 and :45 still pass callbacks to
storage.get and runtime.sendMessage.

The probe also reported that Firefox exposes chrome.* as a distinct,
callback-flavored object alongside browser.*. That is not a contradiction
either; it is precisely the fact that option (b) in this issue's implementation
requirements is built on.

So: premise intact, analysis intact, call-site list intact. Unlike
#173, this issue was written from
reading the code rather than from guessing about the environment, and it holds
up. I am recording this explicitly because a passing observation from a probe
is not evidence about paths the probe never executed, and it would have been
easy - and wrong - to relay it as though it were.

Consequences for this issue

  • The DoD's manual-verification checkboxes should be rewritten as harness
    assertions once #184 and
    #181 land. I will do that rather
    than asking you to run six browser passes.
  • The dApp-driving machinery this needs is the same machinery specified in
    #183 - a local page speaking
    EIP-1193 through the real content script and background. Building it twice
    would be waste, so 183 should be written against both harnesses, or at least
    with the Firefox backend in mind.
  • Sequencing: 181, then 184, then 183, then this issue - implemented with the
    verification for its own fix already sitting there waiting for it. That is a
    much better position than fixing it blind and hoping.

No change to the strategy question. Option (a) versus (b) is still open and
still the implementer's call to make and justify in the PR.

## Your six manual checks are now machine-verifiable, and the premise here still stands Two separate things, and it matters that they are kept apart. ### 1. The DoD is no longer blocked on a human This issue's definition of done is six manual browser checks, four of them on Firefox. https://git.eeqj.de/sneak/AutistMask/issues/173 claimed no agent could perform them. That claim was wrong for Chrome and it is **also wrong for Firefox**: a containerized Firefox 153 driven by geckodriver installs the MV2 build from `dist/firefox/` as a temporary add-on and drives the popup, and it has been demonstrated failing and then passing on a real bug. Full detail and the pinning requirements are in https://git.eeqj.de/sneak/AutistMask/issues/184. So the four Firefox checks and the two Chrome checks in the DoD above become automated assertions in the harnesses rather than a QA pass by you. ### 2. But this issue's own premise was NOT written from a bad assumption The Firefox probe reported, as an incidental finding, that the popup is "not non-functional" under Firefox - wallet creation, BIP-39, libsodium encryption, HD derivation, state persistence and render all complete with zero console errors - and suggested this issue needed re-grounding. **I checked that before repeating it, and it does not refute this issue.** The probe walked the popup wallet-management paths. Those go through `src/shared/state.js`, which resolves `browser.storage.local` and uses it in its **promise** form - correct on Firefox, which is exactly why that flow works cleanly. Every call site enumerated as 1-8 in the issue body is somewhere else: the content script, the background page, and `approval.js`. Those use the **callback** form, and the probe never walked them, because reaching them requires a dApp page speaking EIP-1193 rather than the popup UI. Confirmed directly - `src/content/index.js:23` and `:45` still pass callbacks to `storage.get` and `runtime.sendMessage`. The probe also reported that Firefox exposes `chrome.*` as a distinct, callback-flavored object alongside `browser.*`. That is not a contradiction either; it is precisely the fact that option (b) in this issue's implementation requirements is built on. So: premise intact, analysis intact, call-site list intact. Unlike https://git.eeqj.de/sneak/AutistMask/issues/173, this issue was written from reading the code rather than from guessing about the environment, and it holds up. I am recording this explicitly because a passing observation from a probe is not evidence about paths the probe never executed, and it would have been easy - and wrong - to relay it as though it were. ### Consequences for this issue - The DoD's manual-verification checkboxes should be rewritten as harness assertions once https://git.eeqj.de/sneak/AutistMask/issues/184 and https://git.eeqj.de/sneak/AutistMask/issues/181 land. I will do that rather than asking you to run six browser passes. - The dApp-driving machinery this needs is the same machinery specified in https://git.eeqj.de/sneak/AutistMask/issues/183 - a local page speaking EIP-1193 through the real content script and background. Building it twice would be waste, so 183 should be written against both harnesses, or at least with the Firefox backend in mind. - Sequencing: 181, then 184, then 183, then this issue - implemented with the verification for its own fix already sitting there waiting for it. That is a much better position than fixing it blind and hoping. No change to the strategy question. Option (a) versus (b) is still open and still the implementer's call to make and justify in the PR.
clawbot commented 2026-08-12 13:31:23 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan

Strategy (a), the promise shim. src/shared/browserApi.js becomes the only
file in the tree that names browser or chrome. It exports lazily-resolved
namespace handles for the parts that are events or synchronous
(runtimeApi(), windowsApi(), tabsApi(), actionApi(), storageLocal(),
alarmsApi()) and promise-returning wrappers for every call that is
callback-shaped on Chrome (sendMessage, tabsQuery, tabsSendMessage,
windowsCreate, windowsGetLastFocused, windowsRemove). Call sites await.

Why (a) over (b): src/shared/state.js, src/shared/alarms.js and
src/shared/phishingDomains.js already resolve browser.* and use it in its
promise form — which is exactly why the popup wallet flows work on Firefox
today while everything in this issue's list does not. Strategy (b) would make
the callback half correct by regressing the working half to callbacks, or leave
the codebase split down the middle. (a) makes the whole tree one shape, and it
composes with the async handlers in src/background/index.js.

Two details worth stating up front rather than being found in review:

  • Storage is called in its promise form on both namespaces, not wrapped in
    new Promise. chrome.storage.local.get() returns a promise on MV3 and the
    three modules above already depend on that. Wrapping it would be a change,
    not a fix.
  • A fire-and-forget notification (AUTISTMASK_ACTIVE_CHANGED,
    AUTISTMASK_REMOVE_SITE) gets its own notify(), which sends with no
    callback and swallows the no-receiver rejection. Appending a callback to a
    send whose answer nobody reads would be noise.

runtime.lastError disappears entirely: on the Chrome path the wrapper reads
it inside the callback and turns it into a rejection, so the three sites that
checked it become .catch(() => {}) on the send itself.

Call sites 1-8 all still exist, at moved line numbers, and all get converted.
Beyond them, the remaining typeof browser !== "undefined" ternaries
(state.js, alarms.js, phishingDomains.js, walletDelete.js,
popup/views/home.js, popup/views/settings.js) are re-sourced from the
module so the DoD's "no ternaries outside that module" holds. Those are a
namespace re-source only, no call-shape change. src/content/inpage.js is not
touched.

No manifest change is expected; I will say so explicitly in the PR either way.

Verification

Machine, not manual, per the comment above. The Chrome suite already asserts
the four flows. For Firefox I extend tests/e2e/firefox/ with the same shape
as tests/e2e/run.js cases 28-37.

The obstacle named in
#184 is --network none, so
there is no http:// origin to inject a content script into. Loopback survives
--network none, so the harness serves the dApp page and a JSON-RPC stub from
a node server on 127.0.0.1 inside the container and points the extension's
rpcUrl at it. That also turns the harness's UNVERIFIED content-script capture
claim into an exercised one.

## Plan **Strategy (a), the promise shim.** `src/shared/browserApi.js` becomes the only file in the tree that names `browser` or `chrome`. It exports lazily-resolved namespace handles for the parts that are events or synchronous (`runtimeApi()`, `windowsApi()`, `tabsApi()`, `actionApi()`, `storageLocal()`, `alarmsApi()`) and promise-returning wrappers for every call that is callback-shaped on Chrome (`sendMessage`, `tabsQuery`, `tabsSendMessage`, `windowsCreate`, `windowsGetLastFocused`, `windowsRemove`). Call sites `await`. Why (a) over (b): `src/shared/state.js`, `src/shared/alarms.js` and `src/shared/phishingDomains.js` already resolve `browser.*` and use it in its **promise** form — which is exactly why the popup wallet flows work on Firefox today while everything in this issue's list does not. Strategy (b) would make the callback half correct by regressing the working half to callbacks, or leave the codebase split down the middle. (a) makes the whole tree one shape, and it composes with the `async` handlers in `src/background/index.js`. Two details worth stating up front rather than being found in review: - Storage is called in its **promise** form on both namespaces, not wrapped in `new Promise`. `chrome.storage.local.get()` returns a promise on MV3 and the three modules above already depend on that. Wrapping it would be a change, not a fix. - A fire-and-forget notification (`AUTISTMASK_ACTIVE_CHANGED`, `AUTISTMASK_REMOVE_SITE`) gets its own `notify()`, which sends with no callback and swallows the no-receiver rejection. Appending a callback to a send whose answer nobody reads would be noise. `runtime.lastError` disappears entirely: on the Chrome path the wrapper reads it inside the callback and turns it into a rejection, so the three sites that checked it become `.catch(() => {})` on the send itself. Call sites 1-8 all still exist, at moved line numbers, and all get converted. Beyond them, the remaining `typeof browser !== "undefined"` ternaries (`state.js`, `alarms.js`, `phishingDomains.js`, `walletDelete.js`, `popup/views/home.js`, `popup/views/settings.js`) are re-sourced from the module so the DoD's "no ternaries outside that module" holds. Those are a namespace re-source only, no call-shape change. `src/content/inpage.js` is not touched. No manifest change is expected; I will say so explicitly in the PR either way. ## Verification Machine, not manual, per the comment above. The Chrome suite already asserts the four flows. For Firefox I extend `tests/e2e/firefox/` with the same shape as `tests/e2e/run.js` cases 28-37. The obstacle named in [#184](https://git.eeqj.de/sneak/AutistMask/issues/184) is `--network none`, so there is no `http://` origin to inject a content script into. Loopback survives `--network none`, so the harness serves the dApp page and a JSON-RPC stub from a node server on `127.0.0.1` inside the container and points the extension's `rpcUrl` at it. That also turns the harness's UNVERIFIED content-script capture claim into an exercised one.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#153