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

fix: answer the page when a background handler throws (closes #280) #282

Open
clawbot wants to merge 1 commits from issue-280-handlerpc-catch into next
pull from: issue-280-handlerpc-catch
merge into: sneak:next
Conversation 1 Commits 1 Files Changed 3 +265 -6
clawbot commented 2026-08-14 06:06:51 +02:00
Collaborator
Copy Link

Closes #280.

What changed

handleRpc(...).then(sendResponse) had no .catch(), and sendResponse is the
only thing that settles the dApp's window.ethereum.request() promise. Any throw
inside handleRpc sent nothing back, the content script posted nothing, and the
page's promise stayed pending forever — no error, no timeout, indistinguishable
from a slow wallet.

A rejected handleRpc now answers:

{ error: { code: -32603, message: "AutistMask could not complete this request because of an internal error." } }

-32603 is the JSON-RPC internal error EIP-1474 defines and EIP-1193 defers to for
RPC-layer failures. No EIP-1193 4xxx code describes "the wallet broke", and none was
invented. The cause is not put in message: the page gets a stable full sentence,
and log.errorf puts the method and the throw on the background console, so the
failure is visible rather than swallowed.

Sibling-handler ruling

Complete inventory of async escape points in src/background/index.js —
grep -n "\.then\|\.catch\|(async ()" returns five sites:

site ruling
handleRpc(...).then(...) fixed, above
(async () => {...})() behind AUTISTMASK_TX_RESPONSE fixed
(async () => {...})() behind AUTISTMASK_SIGN_RESPONSE fixed
result.catch(...) in openApproval already caught
startBackgroundJobs().catch(...) already caught, already logged

The two IIFEs are the same shape one level down. Every statement is inside a try,
but a throw out of one of the catch blocks escapes as an unhandled rejection, and
neither the popup nor the page is ever answered. Each gets a last-resort .catch()
that settles the approval through settleApproval() — the existing chokepoint,
with no new delete or resolve — and then answers the popup. The transaction
one reports stage: "broadcast", because it cannot tell whether the transaction
reached the network, and that is the wording that does not invite a second send.

Every other handler on the message path (AUTISTMASK_GET_APPROVAL,
AUTISTMASK_APPROVAL_RESPONSE, AUTISTMASK_ACTIVE_CHANGED, AUTISTMASK_REMOVE_SITE,
and the synchronous branches of the two response handlers) is synchronous: it calls
sendResponse and returns before any await, so it cannot leave a promise pending.

Tests

Three unit tests in tests/backgroundApproval.test.js, each driven by a real
failure rather than a hook in the handler under test:

  • handleRpc: extension storage rejects, which getState() awaits unguarded, on a
    plain eth_accounts.
  • transaction path: describeTxFailure throws while classifying a genuine
    verification failure (an artifact signed at a nonce the approval never displayed).
  • sign path: failureIsRetryable throws while classifying a genuine verification
    failure (the active address moved after approval).

Node aborts the worker on an unhandled rejection; a service worker does not — the
promise simply never settles. The suite records unhandled rejections instead of dying
on them, so the assertion that the page was answered is what reports the failure,
and the recording is asserted empty alongside it.

No e2e case. Provoking a storage failure in a real browser needs a contrived hook,
and tests/e2e/harness.js documents that Playwright exposes no error event for
service workers, so the harness could not observe it either way. The background
console line is the visibility, and the unit test is what fails on regression.

Demonstrated failing first

make test on this branch with src/background/index.js reverted to its
next state (tests unchanged):

FAIL  tests/backgroundApproval.test.js
  ● a handler that throws still settles the page › a rejected handleRpc rejects the page instead of hanging it

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: {"error": {"code": -32603, "message": "AutistMask could not complete this request because of an internal error."}}

    Number of calls: 0

      1104 |         // arrives.
      1105 |         expect(answer.kept).toBe(true);
    > 1106 |         expect(answer.sendResponse).toHaveBeenCalledWith({
           |                                     ^
      1107 |             error: INTERNAL_ERROR,
      1108 |         });

  ● a handler that throws still settles the page › a rejected handleRpc rejects the page instead of hanging it

    storage unavailable

      at get (src/background/index.js:61:37)
      at getState (src/background/index.js:403:25)
      at handleRpc (src/background/index.js:868:9)

  ● a handler that throws still settles the page › a throw while handling a failed transaction settles both the page and the popup

    expect(received).toEqual(expected) // deep equality

    Expected: {"error": {"code": -32603, "message": "AutistMask could not complete this request because of an internal error."}}
    Received: null

      1149 |         expect(bg.broadcastTransaction).not.toHaveBeenCalled();
    > 1150 |         expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
           |                                  ^

  ● a handler that throws still settles the page › a throw while handling a failed signature settles both the page and the popup

    expect(received).toEqual(expected) // deep equality

    Expected: {"error": {"code": -32603, "message": "AutistMask could not complete this request because of an internal error."}}
    Received: null

      1191 |         await settleIncludingRejections();
      1192 |
    > 1193 |         expect(pending.result()).toEqual({ error: INTERNAL_ERROR });
           |                                  ^

Test Suites: 1 failed, 27 passed, 28 total
Tests:       3 failed, 681 passed, 684 total

Number of calls: 0 on the RPC path is precisely the defect: nothing was ever sent
back, so the page waits forever. Received: null on the two approval paths is the
same thing for the promise the approval holds.

make check

Green with the fix in place:

Test Suites: 28 passed, 28 total
Tests:       684 passed, 684 total
...
test-verify-build: 18 case(s) passed
Linting...
$ prettier --check .
All matched files use Prettier code style!
Checking formatting...
$ prettier --check .
All matched files use Prettier code style!

Also re-run inside the container via script/cibuild, which runs make check from
the Dockerfile. Step #11 [7/8] RUN make check executed (not CACHED — the
source change invalidates the layer), with the same 684 tests, 18 verify-build cases
and clean prettier; docker build exited 0. No prune of any kind was run.

make fmt run and included; TODO.md updated in the same commit.

Closes [#280](https://git.eeqj.de/sneak/AutistMask/issues/280). ## What changed `handleRpc(...).then(sendResponse)` had no `.catch()`, and `sendResponse` is the only thing that settles the dApp's `window.ethereum.request()` promise. Any throw inside `handleRpc` sent nothing back, the content script posted nothing, and the page's promise stayed pending forever — no error, no timeout, indistinguishable from a slow wallet. A rejected `handleRpc` now answers: ```js { error: { code: -32603, message: "AutistMask could not complete this request because of an internal error." } } ``` `-32603` is the JSON-RPC internal error EIP-1474 defines and EIP-1193 defers to for RPC-layer failures. No EIP-1193 4xxx code describes "the wallet broke", and none was invented. The cause is not put in `message`: the page gets a stable full sentence, and `log.errorf` puts the method and the throw on the background console, so the failure is visible rather than swallowed. ## Sibling-handler ruling Complete inventory of async escape points in `src/background/index.js` — `grep -n "\.then\|\.catch\|(async ()"` returns five sites: | site | ruling | | --- | --- | | `handleRpc(...).then(...)` | fixed, above | | `(async () => {...})()` behind `AUTISTMASK_TX_RESPONSE` | fixed | | `(async () => {...})()` behind `AUTISTMASK_SIGN_RESPONSE` | fixed | | `result.catch(...)` in `openApproval` | already caught | | `startBackgroundJobs().catch(...)` | already caught, already logged | The two IIFEs are the same shape one level down. Every statement is inside a `try`, but a throw out of one of the `catch` blocks escapes as an unhandled rejection, and neither the popup nor the page is ever answered. Each gets a last-resort `.catch()` that settles the approval through `settleApproval()` — the existing chokepoint, with no new `delete` or `resolve` — and then answers the popup. The transaction one reports `stage: "broadcast"`, because it cannot tell whether the transaction reached the network, and that is the wording that does not invite a second send. Every other handler on the message path (`AUTISTMASK_GET_APPROVAL`, `AUTISTMASK_APPROVAL_RESPONSE`, `AUTISTMASK_ACTIVE_CHANGED`, `AUTISTMASK_REMOVE_SITE`, and the synchronous branches of the two response handlers) is synchronous: it calls `sendResponse` and returns before any await, so it cannot leave a promise pending. ## Tests Three unit tests in `tests/backgroundApproval.test.js`, each driven by a **real** failure rather than a hook in the handler under test: - `handleRpc`: extension storage rejects, which `getState()` awaits unguarded, on a plain `eth_accounts`. - transaction path: `describeTxFailure` throws while classifying a genuine verification failure (an artifact signed at a nonce the approval never displayed). - sign path: `failureIsRetryable` throws while classifying a genuine verification failure (the active address moved after approval). Node aborts the worker on an unhandled rejection; a service worker does not — the promise simply never settles. The suite records unhandled rejections instead of dying on them, so the assertion that the page *was* answered is what reports the failure, and the recording is asserted empty alongside it. **No e2e case.** Provoking a storage failure in a real browser needs a contrived hook, and `tests/e2e/harness.js` documents that Playwright exposes no error event for service workers, so the harness could not observe it either way. The background console line is the visibility, and the unit test is what fails on regression. ### Demonstrated failing first `make test` on this branch with `src/background/index.js` reverted to its `next` state (tests unchanged): ``` FAIL tests/backgroundApproval.test.js ● a handler that throws still settles the page › a rejected handleRpc rejects the page instead of hanging it expect(jest.fn()).toHaveBeenCalledWith(...expected) Expected: {"error": {"code": -32603, "message": "AutistMask could not complete this request because of an internal error."}} Number of calls: 0 1104 | // arrives. 1105 | expect(answer.kept).toBe(true); > 1106 | expect(answer.sendResponse).toHaveBeenCalledWith({ | ^ 1107 | error: INTERNAL_ERROR, 1108 | }); ● a handler that throws still settles the page › a rejected handleRpc rejects the page instead of hanging it storage unavailable at get (src/background/index.js:61:37) at getState (src/background/index.js:403:25) at handleRpc (src/background/index.js:868:9) ● a handler that throws still settles the page › a throw while handling a failed transaction settles both the page and the popup expect(received).toEqual(expected) // deep equality Expected: {"error": {"code": -32603, "message": "AutistMask could not complete this request because of an internal error."}} Received: null 1149 | expect(bg.broadcastTransaction).not.toHaveBeenCalled(); > 1150 | expect(pending.result()).toEqual({ error: INTERNAL_ERROR }); | ^ ● a handler that throws still settles the page › a throw while handling a failed signature settles both the page and the popup expect(received).toEqual(expected) // deep equality Expected: {"error": {"code": -32603, "message": "AutistMask could not complete this request because of an internal error."}} Received: null 1191 | await settleIncludingRejections(); 1192 | > 1193 | expect(pending.result()).toEqual({ error: INTERNAL_ERROR }); | ^ Test Suites: 1 failed, 27 passed, 28 total Tests: 3 failed, 681 passed, 684 total ``` `Number of calls: 0` on the RPC path is precisely the defect: nothing was ever sent back, so the page waits forever. `Received: null` on the two approval paths is the same thing for the promise the approval holds. ## `make check` Green with the fix in place: ``` Test Suites: 28 passed, 28 total Tests: 684 passed, 684 total ... test-verify-build: 18 case(s) passed Linting... $ prettier --check . All matched files use Prettier code style! Checking formatting... $ prettier --check . All matched files use Prettier code style! ``` Also re-run inside the container via `script/cibuild`, which runs `make check` from the `Dockerfile`. Step `#11 [7/8] RUN make check` executed (not `CACHED` — the source change invalidates the layer), with the same 684 tests, 18 verify-build cases and clean prettier; `docker build` exited 0. No prune of any kind was run. `make fmt` run and included; `TODO.md` updated in the same commit.
clawbot added 1 commit 2026-08-14 06:06:52 +02:00
fix: answer the page when a background handler throws (closes #280)
All checks were successful
check / check (push) Successful in 46s
Details
9665ac448e 
handleRpc(...).then(sendResponse) had no .catch(), and sendResponse is the only
thing that settles the dApp's window.ethereum.request() promise. Any throw
inside handleRpc therefore sent nothing back: the content script posted nothing,
and the page's promise stayed pending forever with no error and no timeout,
indistinguishable from a slow wallet. handleRpc does real work -- state loads,
provider calls, transaction population, approval plumbing -- so "it does not
throw today" was not a property anyone was maintaining.

A rejected handleRpc now answers { code: -32603, message }. -32603 is the
JSON-RPC internal error EIP-1474 defines and EIP-1193 defers to for RPC-layer
failures; no EIP-1193 4xxx code describes "the wallet broke" and none was
invented for it. The cause is not put in the message: the page gets a stable
sentence, the background console gets the method and the throw, so the failure
is visible rather than swallowed.

The two async IIFEs behind AUTISTMASK_TX_RESPONSE and AUTISTMASK_SIGN_RESPONSE
are the same shape one level down. Every statement is inside a try, but a throw
from one of the catch blocks escapes as an unhandled rejection and neither the
popup nor the page is answered. Each gets a last-resort .catch() that settles
the approval through settleApproval() -- the existing chokepoint, with no new
delete or resolve -- and answers the popup. The transaction one reports the
broadcast stage, because it cannot tell whether the transaction reached the
network and that is the wording that does not invite a second send. Every other
message handler on the path is synchronous and cannot leave a promise pending.

Each of the three is driven by a real failure rather than a hook in the handler:
a rejecting extension-storage read, which getState() awaits unguarded, and a
failure classifier that throws while classifying a genuine verification failure.
All three were demonstrated failing against the unfixed code, the RPC one with
sendResponse at zero calls, which is precisely the page-side hang.
clawbot added the needs-review label 2026-08-14 06:06:59 +02:00
clawbot self-assigned this 2026-08-14 06:06:59 +02:00
clawbot commented 2026-08-14 06:25:10 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework.

1. src/background/index.js:1103 — the transaction last-resort catch reports stage: TX_STAGE_BROADCAST on a path where the transaction provably never reached the network.

The IIFE can only reject from inside one of its own two catch blocks. The first (lines 1031-1052) runs entirely before provider.broadcastTransaction() is ever called, and this PR's own test asserts exactly that at tests/backgroundApproval.test.js:1150 (expect(bg.broadcastTransaction).not.toHaveBeenCalled()). With retryable: false and stage: "broadcast", describeSigningFailure() (src/shared/approvalVerify.js:652-655) appends " The transaction may still have reached the network. Check the account before sending it again." and src/popup/views/approval.js:659 renders it. So the one case the new test exercises tells the user their transaction may be on chain when it demonstrably is not — the exact copy defect #271 was filed over, whose definition of done reads "A transaction that failed on a nonce collision before broadcast reports copy that says so, not 'may still have reached the network'".

The justification given at src/background/index.js:1086-1088, at tests/backgroundApproval.test.js:1155-1156, in TODO.md and in the PR body — that the handler "cannot tell whether the transaction reached the network" — is false. It is one local away. Acceptable: let stage = TX_STAGE_VERIFY; in the IIFE, set to TX_STAGE_BROADCAST immediately before provider.broadcastTransaction(...), and reported from the last-resort catch — so a verify-phase escape reports TX_STAGE_VERIFY ("This request can no longer be signed. Please start it again from the site.") and only a broadcast-phase escape keeps TX_STAGE_BROADCAST, with a test asserting each.

2. tests/backgroundApproval.test.js:294-303 — the process.on("unhandledRejection") recorder is unnecessary and its comment states behaviour this repo does not exhibit.

The comment claims "Node aborts the worker process on an unhandled rejection". Under this repo's Jest 30 setup it does not. Removing only the process.on(...) registration (keeping const unhandledRejections = []) and re-running make test with src/background/index.js reverted to next gives an identical red result — Tests: 3 failed, 681 passed, 684 total, Test Suites: 1 failed, 27 passed, 28 total — with no worker abort; with the fix in place and the registration removed, 684 passed, 28 total. A stray Promise.reject() planted inside this same file also still fails its own test with the recorder installed, so it masks nothing either. The recorder and the three expect(unhandledRejections).toEqual([]) assertions at lines 1110, 1159 and 1199 are dead weight carrying a false rationale in a security-critical test file. Acceptable: delete the registration, the array and the three assertions, keeping settleIncludingRejections() with a comment describing what it actually waits for.

Everything else checked passes, including the claim interlock (holdsClaim: true can only ever retire the approval this attempt holds — every escape out of both IIFEs is synchronous with respect to the last claimApproval/releaseApproval mutation and .catch() runs as a microtask, and settleApproval() deletes before resolve(), so a throw after a successful broadcast makes the last-resort settle a no-op and the page keeps its txHash), the independently re-derived five-site sibling inventory, -32603 and its full-sentence message, anti-vacuity re-verified by reverting the fix locally, containerized make check (layer #11 [7/8] RUN make check ran uncached in 19.8s: 684 tests, 18 verify-build cases, prettier clean), and a clean merge onto current next (0be20d7) with 706 tests passing on the merged tree.

Disclosures: CI on 9665ac4 has been pending/"Waiting to run" since 06:06 with no runner picking it up, so it is neither green nor red — the containerized make check above is the substitute evidence. The sign path's last-resort deliberately omits stage, which is correct. make test-e2e was run five times: next baseline 40/40; PR head 36/37 then 33/37; PR merged with next 40/40 then 38/40 — different tests fail on each run and the merged tree went fully green once, so I attribute this to timing flakiness in the e2e harness under shared-host load rather than to this change, and it likely deserves its own issue. make test-e2e-firefox was not run.

FAIL — `needs-rework`. **1. `src/background/index.js:1103` — the transaction last-resort catch reports `stage: TX_STAGE_BROADCAST` on a path where the transaction provably never reached the network.** The IIFE can only reject from inside one of its own two `catch` blocks. The first (lines 1031-1052) runs entirely before `provider.broadcastTransaction()` is ever called, and this PR's own test asserts exactly that at `tests/backgroundApproval.test.js:1150` (`expect(bg.broadcastTransaction).not.toHaveBeenCalled()`). With `retryable: false` and `stage: "broadcast"`, `describeSigningFailure()` (`src/shared/approvalVerify.js:652-655`) appends " The transaction may still have reached the network. Check the account before sending it again." and `src/popup/views/approval.js:659` renders it. So the one case the new test exercises tells the user their transaction may be on chain when it demonstrably is not — the exact copy defect https://git.eeqj.de/sneak/AutistMask/issues/271 was filed over, whose definition of done reads "A transaction that failed on a nonce collision before broadcast reports copy that says so, not 'may still have reached the network'". The justification given at `src/background/index.js:1086-1088`, at `tests/backgroundApproval.test.js:1155-1156`, in `TODO.md` and in the PR body — that the handler "cannot tell whether the transaction reached the network" — is false. It is one local away. Acceptable: `let stage = TX_STAGE_VERIFY;` in the IIFE, set to `TX_STAGE_BROADCAST` immediately before `provider.broadcastTransaction(...)`, and reported from the last-resort catch — so a verify-phase escape reports `TX_STAGE_VERIFY` ("This request can no longer be signed. Please start it again from the site.") and only a broadcast-phase escape keeps `TX_STAGE_BROADCAST`, with a test asserting each. **2. `tests/backgroundApproval.test.js:294-303` — the `process.on("unhandledRejection")` recorder is unnecessary and its comment states behaviour this repo does not exhibit.** The comment claims "Node aborts the worker process on an unhandled rejection". Under this repo's Jest 30 setup it does not. Removing only the `process.on(...)` registration (keeping `const unhandledRejections = []`) and re-running `make test` with `src/background/index.js` reverted to `next` gives an identical red result — `Tests: 3 failed, 681 passed, 684 total`, `Test Suites: 1 failed, 27 passed, 28 total` — with no worker abort; with the fix in place and the registration removed, `684 passed, 28 total`. A stray `Promise.reject()` planted inside this same file also still fails its own test with the recorder installed, so it masks nothing either. The recorder and the three `expect(unhandledRejections).toEqual([])` assertions at lines 1110, 1159 and 1199 are dead weight carrying a false rationale in a security-critical test file. Acceptable: delete the registration, the array and the three assertions, keeping `settleIncludingRejections()` with a comment describing what it actually waits for. Everything else checked passes, including the claim interlock (`holdsClaim: true` can only ever retire the approval this attempt holds — every escape out of both IIFEs is synchronous with respect to the last `claimApproval`/`releaseApproval` mutation and `.catch()` runs as a microtask, and `settleApproval()` deletes before `resolve()`, so a throw after a successful broadcast makes the last-resort settle a no-op and the page keeps its `txHash`), the independently re-derived five-site sibling inventory, `-32603` and its full-sentence message, anti-vacuity re-verified by reverting the fix locally, containerized `make check` (layer `#11 [7/8] RUN make check` ran uncached in 19.8s: 684 tests, 18 verify-build cases, prettier clean), and a clean merge onto current `next` (`0be20d7`) with 706 tests passing on the merged tree. Disclosures: CI on `9665ac4` has been `pending`/"Waiting to run" since 06:06 with no runner picking it up, so it is neither green nor red — the containerized `make check` above is the substitute evidence. The sign path's last-resort deliberately omits `stage`, which is correct. `make test-e2e` was run five times: `next` baseline 40/40; PR head 36/37 then 33/37; PR merged with `next` 40/40 then 38/40 — different tests fail on each run and the merged tree went fully green once, so I attribute this to timing flakiness in the e2e harness under shared-host load rather than to this change, and it likely deserves its own issue. `make test-e2e-firefox` was not run.
clawbot added needs-rework and removed needs-review labels 2026-08-14 06:25:59 +02:00
All checks were successful
check / check (push) Successful in 46s
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 issue-280-handlerpc-catch:issue-280-handlerpc-catch
git checkout issue-280-handlerpc-catch
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-rework
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#282
Delete Branch "issue-280-handlerpc-catch"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?