MetaMask / MetaMask/metamask-extension

Contain and report a failed polling teardown

Open
#45,614 1 comment 0 reactions 0 assignees View on GitHub
exalate-duplicate INVALID-ISSUE-TEMPLATE team-extension-platform
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

**What is this about?**

`onClientClosed` stops ten polling controllers when the last UI surface closes. All ten calls sit inside a single `try`/`catch` whose handler is `console.error`, and the throw inside them is reachable: `AbstractPollingControllerBase.stopAllPolling()` iterates its token sets calling `stopPollingByPollingToken`, which throws on a falsy token. One bad token skips every controller after it in the list, and beyond that guard each `_stopPollingByPollingTokenSetId` is subclass-implemented, so ten separate teardown implementations run inside one shared `try`.

The failure is silent and indistinguishable from success. Nothing reaches telemetry, and the observable result — the extension appears to have closed cleanly — is identical whether all ten stopped or only the first.

A second, smaller problem sits beside it. The teardown list contains UI-display pollers only: gas fees, currency rates, token rates, token detection, token list, token balances, static assets, account tracker, DeFi positions and subscriptions. Controllers whose work must outlive the UI — `smartTransactionsController`, `userOperationController`, `bridgeStatusController`, which track submitted transactions to confirmation — are deliberately absent, as are `bridgeController` and `configRegistryController`. Nothing in the code says so, so the list reads as incomplete rather than as a boundary, and a future reader is likely to "fix" it by adding controllers whose polling must not stop when the popup closes.

## Goal

Contain and report a teardown failure, and make the UI-scoped boundary explicit so the list cannot be widened by mistake.

## Before implementation

Before writing code, try to prove this issue is not worth doing. Make the requirement less dumb before optimizing anything. Look for code, state, or writes we can delete. If deletion does not solve it, prefer a smaller change using what already exists. Record the finding and close the issue if the proposal no longer makes sense.

## Open questions

- Should a controller that fails to stop be retried, or only reported?
- Are `bridgeController` and `configRegistryController` background-scoped for the same reason as the transaction trackers, or absent for a different one?
- Would controllers registering themselves as UI-scoped be safer than a hand-maintained list, given the boundary is the thing that must not drift?

**Acceptance Criteria**

- [ ] A throw from one `stopAllPolling()` call does not prevent the remaining calls from running
- [ ] A teardown failure is reported through the normal error path rather than `console.error`
- [ ] The UI-scoped boundary is documented at the teardown site, naming why transaction-tracking controllers are excluded
- [ ] A test asserts that a throwing controller does not suppress the teardown of the others

**References**

- Parent epic: #44253
- Sibling architectural test: #44802

Contributor guide

Open the contributing guide

Research direction

Start at onClientClosed and trace the shared try/catch into AbstractPollingControllerBase.stopAllPolling(), stopPollingByPollingToken, and the ten listed controllers. Check the normal error-reporting path and existing teardown tests before deciding whether deletion or a smaller change resolves the issue. Done means remaining controllers still stop after one throws, the failure is reported, the UI-scoped boundary explains excluded controllers, and a test covers the behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.