Skip to content

Commit 3b85633

Browse files
authored
fix: stop polling on environment close (#29707)
## **Description** When the asset controllers start polling, they add their polling tokens to app state, keyed by the particular environment (popup vs fullscreen). But these polling tokens in app state were not being used during cleanup. This PR updates `onEnvironmentTypeClosed` to stop polling by those tokens. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29707?quickstart=1) ## **Related issues** ## **Manual testing steps** 1. Open MM in fullscreen 2. Open MM popup 3. Close the popup 4. `onEnvironmentTypeClosed` should fire for the popup environment 5. Each polling token should be found and removed by one of the controllers ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 8038f4d commit 3b85633

File tree

3 files changed

+25
-14
lines changed

3 files changed

+25
-14
lines changed

app/scripts/controllers/account-tracker-controller.test.ts

+17-8
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,23 @@ describe('AccountTrackerController', () => {
368368
});
369369
});
370370

371+
it('should gracefully handle unknown polling tokens', async () => {
372+
await withController(({ controller, blockTrackerFromHookStub }) => {
373+
jest.spyOn(controller, 'updateAccounts').mockResolvedValue();
374+
375+
const pollingToken =
376+
controller.startPollingByNetworkClientId('mainnet');
377+
378+
controller.stopPollingByPollingToken('unknown-token');
379+
controller.stopPollingByPollingToken(pollingToken);
380+
381+
expect(blockTrackerFromHookStub.removeListener).toHaveBeenCalledWith(
382+
'latest',
383+
expect.any(Function),
384+
);
385+
});
386+
});
387+
371388
it('should not unsubscribe from the block tracker if called with one of multiple active polling tokens for a given networkClient', async () => {
372389
await withController(({ controller, blockTrackerFromHookStub }) => {
373390
jest.spyOn(controller, 'updateAccounts').mockResolvedValue();
@@ -391,14 +408,6 @@ describe('AccountTrackerController', () => {
391408
}).toThrow('pollingToken required');
392409
});
393410
});
394-
395-
it('should error if no matching pollingToken is found', async () => {
396-
await withController(({ controller }) => {
397-
expect(() => {
398-
controller.stopPollingByPollingToken('potato');
399-
}).toThrow('pollingToken not found');
400-
});
401-
});
402411
});
403412

404413
describe('stopAll', () => {

app/scripts/controllers/account-tracker-controller.ts

-5
Original file line numberDiff line numberDiff line change
@@ -403,20 +403,15 @@ export default class AccountTrackerController extends BaseController<
403403
if (!pollingToken) {
404404
throw new Error('pollingToken required');
405405
}
406-
let found = false;
407406
this.#pollingTokenSets.forEach((tokenSet, key) => {
408407
if (tokenSet.has(pollingToken)) {
409-
found = true;
410408
tokenSet.delete(pollingToken);
411409
if (tokenSet.size === 0) {
412410
this.#pollingTokenSets.delete(key);
413411
this.#unsubscribeWithNetworkClientId(key);
414412
}
415413
}
416414
});
417-
if (!found) {
418-
throw new Error('pollingToken not found');
419-
}
420415
}
421416

422417
/**

app/scripts/metamask-controller.js

+8-1
Original file line numberDiff line numberDiff line change
@@ -7011,7 +7011,7 @@ export default class MetamaskController extends EventEmitter {
70117011

70127012
/**
70137013
* A method that is called by the background when a particular environment type is closed (fullscreen, popup, notification).
7014-
* Currently used to stop polling in the gasFeeController for only that environement type
7014+
* Currently used to stop polling controllers for only that environement type
70157015
*
70167016
* @param environmentType
70177017
*/
@@ -7021,8 +7021,15 @@ export default class MetamaskController extends EventEmitter {
70217021
const pollingTokensToDisconnect =
70227022
this.appStateController.state[appStatePollingTokenType];
70237023
pollingTokensToDisconnect.forEach((pollingToken) => {
7024+
// We don't know which controller the token is associated with, so try them all.
7025+
// Consider storing the tokens per controller in state instead.
70247026
this.gasFeeController.stopPollingByPollingToken(pollingToken);
70257027
this.currencyRateController.stopPollingByPollingToken(pollingToken);
7028+
this.tokenRatesController.stopPollingByPollingToken(pollingToken);
7029+
this.tokenDetectionController.stopPollingByPollingToken(pollingToken);
7030+
this.tokenListController.stopPollingByPollingToken(pollingToken);
7031+
this.tokenBalancesController.stopPollingByPollingToken(pollingToken);
7032+
this.accountTrackerController.stopPollingByPollingToken(pollingToken);
70267033
this.appStateController.removePollingToken(
70277034
pollingToken,
70287035
appStatePollingTokenType,

0 commit comments

Comments
 (0)