fix: a throw inside handleRpc hangs the dApp's promise forever with no error #280

New Issue

clawbot · 2026-08-12T13:47:51+02:00

clawbot commented

2026-08-12 13:47:51 +02:00

src/background/index.js:868 — handleRpc(...).then((response) => sendResponse(response)) has no .catch().

If anything inside handleRpc throws, sendResponse is never called, the content script posts nothing back, and the page's window.ethereum.request() promise stays pending forever, with no error and no timeout. The dApp has no way to distinguish it from a slow wallet.

Found by the independent review of #278. Pre-existing and untouched by that PR, which is why it was not blocked there — but the reviewer's point stands that it is a worse failure mode than the one that PR fixed: #274 was a rejection arriving without its code, and this is a rejection never arriving at all.

handleRpc is large and does real work — state loads, provider calls, transaction population, approval plumbing — so "it does not throw today" is not a property anyone is maintaining.

Implementation requirements

A rejected handleRpc must still sendResponse with an error the page can reject on, so the promise settles. Use a code where one is defined for the situation; a generic internal error is acceptable where none is, but the message must be a full sentence per the repo's Language & Labeling rules.
Do not swallow the failure silently — it should still be visible in the background console, since the e2e harness fails a run on an uncaught error and that is how this class gets caught in future.
Check the sibling handlers for the same shape. The same .then() without .catch() pattern may appear on other message paths.
A unit test that makes handleRpc throw and asserts a page-facing rejection is produced. An e2e case would be better if one can be provoked without a contrived hook — say which you did.

Definition of done

A throw inside handleRpc produces a page-facing rejection rather than a pending promise.
The rejection carries a code where one applies, and a full-sentence message.
Any sibling handler with the same missing-catch shape is fixed or explicitly ruled out in the PR body.
A test makes handleRpc throw and asserts the page rejects, demonstrated failing first.
TODO.md updated in the same commit.
make check passes.

Also noted, not a defect today

An error carrying data but no code drops data, because it falls to the plain-Error branch in the provider. Unreachable at present — nothing emits data — so it is recorded here rather than filed separately.

`src/background/index.js:868` — `handleRpc(...).then((response) => sendResponse(response))` has no `.catch()`. If anything inside `handleRpc` throws, `sendResponse` is never called, the content script posts nothing back, and the page's `window.ethereum.request()` promise stays pending **forever**, with no error and no timeout. The dApp has no way to distinguish it from a slow wallet. Found by the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/278. Pre-existing and untouched by that PR, which is why it was not blocked there — but the reviewer's point stands that it is a **worse failure mode than the one that PR fixed**: https://git.eeqj.de/sneak/AutistMask/issues/274 was a rejection arriving without its code, and this is a rejection never arriving at all. `handleRpc` is large and does real work — state loads, provider calls, transaction population, approval plumbing — so "it does not throw today" is not a property anyone is maintaining. ## Implementation requirements - A rejected `handleRpc` must still `sendResponse` with an error the page can reject on, so the promise settles. Use a code where one is defined for the situation; a generic internal error is acceptable where none is, but the message must be a full sentence per the repo's Language & Labeling rules. - Do not swallow the failure silently — it should still be visible in the background console, since the e2e harness fails a run on an uncaught error and that is how this class gets caught in future. - Check the sibling handlers for the same shape. The same `.then()` without `.catch()` pattern may appear on other message paths. - A unit test that makes `handleRpc` throw and asserts a page-facing rejection is produced. An e2e case would be better if one can be provoked without a contrived hook — say which you did. ## Definition of done - [ ] A throw inside `handleRpc` produces a page-facing rejection rather than a pending promise. - [ ] The rejection carries a code where one applies, and a full-sentence message. - [ ] Any sibling handler with the same missing-`catch` shape is fixed or explicitly ruled out in the PR body. - [ ] A test makes `handleRpc` throw and asserts the page rejects, demonstrated failing first. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes. ## Also noted, not a defect today An error carrying `data` but no `code` drops `data`, because it falls to the plain-`Error` branch in the provider. Unreachable at present — nothing emits `data` — so it is recorded here rather than filed separately.

clawbot added this to the 1.0.0 milestone 2026-08-12 13:47:58 +02:00

clawbot commented

2026-08-12 13:51:55 +02:00

Plan.

Shape: a rejected handleRpc answers with { 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". No EIP-1193 4xxx code is invented for it, and the cause is not put in message — the page gets a stable sentence, the background console gets the throw.

Visibility: .catch() logs the method and the error through log.errorf before it settles the page, so the failure is on the background console rather than swallowed. Note the e2e harness cannot see it either way — tests/e2e/harness.js documents that Playwright exposes no error event for service workers — so the console line is the evidence, and the unit test is what fails on regression.

Siblings: the two async IIFEs in AUTISTMASK_TX_RESPONSE / AUTISTMASK_SIGN_RESPONSE are the same shape one level down — every statement is inside a try, but a throw from a catch block escapes an unhandled promise and neither the popup nor the page is answered. Both get a last-resort .catch() that settles the approval through settleApproval() (the existing chokepoint, no new delete/resolve) and answers the popup. The remaining handlers are synchronous and are ruled out in the PR body.

Test: a unit test drives a real failure — chrome.storage.local.get rejecting, which getState() awaits unguarded — and asserts the page-facing { error: { code, message } } reaches sendResponse. Demonstrated failing first against the current code. No e2e case: provoking a storage failure in a real browser needs a contrived hook, and the harness cannot observe worker errors anyway.

Plan. Shape: a rejected `handleRpc` answers with `{ 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". No EIP-1193 4xxx code is invented for it, and the cause is not put in `message` — the page gets a stable sentence, the background console gets the throw. Visibility: `.catch()` logs the method and the error through `log.errorf` before it settles the page, so the failure is on the background console rather than swallowed. Note the e2e harness cannot see it either way — `tests/e2e/harness.js` documents that Playwright exposes no error event for service workers — so the console line is the evidence, and the unit test is what fails on regression. Siblings: the two async IIFEs in `AUTISTMASK_TX_RESPONSE` / `AUTISTMASK_SIGN_RESPONSE` are the same shape one level down — every statement is inside a `try`, but a throw from a `catch` block escapes an unhandled promise and neither the popup nor the page is answered. Both get a last-resort `.catch()` that settles the approval through `settleApproval()` (the existing chokepoint, no new `delete`/`resolve`) and answers the popup. The remaining handlers are synchronous and are ruled out in the PR body. Test: a unit test drives a real failure — `chrome.storage.local.get` rejecting, which `getState()` awaits unguarded — and asserts the page-facing `{ error: { code, message } }` reaches `sendResponse`. Demonstrated failing first against the current code. No e2e case: provoking a storage failure in a real browser needs a contrived hook, and the harness cannot observe worker errors anyway.

clawbot referenced this issue from a commit

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

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

clawbot referenced a pull request that will close this issue

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

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#280