From ffa69e90c0b2295adafdc63365857edb60f302ae Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Wed, 8 Jan 2025 13:21:50 -0700 Subject: [PATCH] Fix NetworkController to handle clearing of messenger If the network controller is in the process of executing the `lookupNetwork` step and the messenger is cleared of subscriptions, then it may throw an error that the `networkDidChange` subscription is missing. This happens in Mobile when it destroys the engine. There are actually two places where `lookupNetwork` subscribes to `networkDidChange` and then unsubscribes, but the aforementioned error is only replicable with one of them. However, we handle both cases just in case. --- packages/network-controller/CHANGELOG.md | 5 + .../src/NetworkController.ts | 47 +++- .../tests/NetworkController.test.ts | 228 ++++++++++++++++++ 3 files changed, 272 insertions(+), 8 deletions(-) diff --git a/packages/network-controller/CHANGELOG.md b/packages/network-controller/CHANGELOG.md index e9c30aab0df..dabf3c069aa 100644 --- a/packages/network-controller/CHANGELOG.md +++ b/packages/network-controller/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) +### Fixed + +- Fix `lookupNetwork` so that it will no longer throw an error if `networkDidChange` subscriptions have been removed before it returns ([#5116](https://github.com/MetaMask/core/pull/5116)) + - This error could occur if the NetworkController's messenger is cleared of subscriptions, as in a "destroy" step. + ## [22.1.1] ### Changed diff --git a/packages/network-controller/src/NetworkController.ts b/packages/network-controller/src/NetworkController.ts index 0e20648bde8..0d749e71e07 100644 --- a/packages/network-controller/src/NetworkController.ts +++ b/packages/network-controller/src/NetworkController.ts @@ -1300,10 +1300,30 @@ export class NetworkController extends BaseController< let networkChanged = false; const listener = () => { networkChanged = true; - this.messagingSystem.unsubscribe( - 'NetworkController:networkDidChange', - listener, - ); + try { + this.messagingSystem.unsubscribe( + 'NetworkController:networkDidChange', + listener, + ); + } catch (error) { + // In theory, this `catch` should not be necessary given that this error + // would occur "inside" of the call to `#determineEIP1559Compatibility` + // below and so it should be caught by the `try`/`catch` below (it is + // impossible to reproduce in tests for that reason). However, somehow + // it occurs within Mobile and so we have to add our own `try`/`catch` + // here. + /* istanbul ignore next */ + if ( + !(error instanceof Error) || + error.message !== + 'Subscription not found for event: NetworkController:networkDidChange' + ) { + // Again, this error should not happen and is impossible to reproduce + // in tests. + /* istanbul ignore next */ + throw error; + } + } }; this.messagingSystem.subscribe( 'NetworkController:networkDidChange', @@ -1370,10 +1390,21 @@ export class NetworkController extends BaseController< // in the process of being called, so we don't need to go further. return; } - this.messagingSystem.unsubscribe( - 'NetworkController:networkDidChange', - listener, - ); + + try { + this.messagingSystem.unsubscribe( + 'NetworkController:networkDidChange', + listener, + ); + } catch (error) { + if ( + !(error instanceof Error) || + error.message !== + 'Subscription not found for event: NetworkController:networkDidChange' + ) { + throw error; + } + } this.update((state) => { const meta = state.networksMetadata[state.selectedNetworkClientId]; diff --git a/packages/network-controller/tests/NetworkController.test.ts b/packages/network-controller/tests/NetworkController.test.ts index 3f224b7170f..eddd91140c5 100644 --- a/packages/network-controller/tests/NetworkController.test.ts +++ b/packages/network-controller/tests/NetworkController.test.ts @@ -1579,6 +1579,109 @@ describe('NetworkController', () => { }); }); + describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => { + it('does not throw an error', async () => { + const infuraProjectId = 'some-infura-project-id'; + + await withController( + { + state: { + selectedNetworkClientId: infuraNetworkType, + }, + infuraProjectId, + }, + async ({ controller, messenger }) => { + const fakeProvider = buildFakeProvider([ + // Called during provider initialization + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + // Called via `lookupNetwork` directly + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + ]); + const fakeNetworkClient = buildFakeClient(fakeProvider); + mockCreateNetworkClient() + .calledWith({ + chainId: ChainId[infuraNetworkType], + infuraProjectId, + network: infuraNetworkType, + ticker: NetworksTicker[infuraNetworkType], + type: NetworkClientType.Infura, + }) + .mockReturnValue(fakeNetworkClient); + await controller.initializeProvider(); + + const lookupNetworkPromise = controller.lookupNetwork(); + messenger.clearSubscriptions(); + expect(await lookupNetworkPromise).toBeUndefined(); + }, + ); + }); + }); + + describe('if removing the networkDidChange subscription fails for an unknown reason', () => { + it('re-throws the error', async () => { + const infuraProjectId = 'some-infura-project-id'; + + await withController( + { + state: { + selectedNetworkClientId: infuraNetworkType, + }, + infuraProjectId, + }, + async ({ controller, messenger }) => { + const fakeProvider = buildFakeProvider([ + // Called during provider initialization + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + // Called via `lookupNetwork` directly + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + ]); + const fakeNetworkClient = buildFakeClient(fakeProvider); + mockCreateNetworkClient() + .calledWith({ + chainId: ChainId[infuraNetworkType], + infuraProjectId, + network: infuraNetworkType, + ticker: NetworksTicker[infuraNetworkType], + type: NetworkClientType.Infura, + }) + .mockReturnValue(fakeNetworkClient); + await controller.initializeProvider(); + + const lookupNetworkPromise = controller.lookupNetwork(); + const error = new Error('oops'); + jest + .spyOn(messenger, 'unsubscribe') + .mockImplementation((eventType) => { + if (eventType === 'NetworkController:networkDidChange') { + throw error; + } + }); + await expect(lookupNetworkPromise).rejects.toThrow(error); + }, + ); + }); + }); + lookupNetworkTests({ expectedNetworkClientType: NetworkClientType.Infura, initialState: { @@ -1889,6 +1992,131 @@ describe('NetworkController', () => { }); }); + describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => { + it('does not throw an error', async () => { + const infuraProjectId = 'some-infura-project-id'; + + await withController( + { + state: { + selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA', + networkConfigurationsByChainId: { + '0x1337': buildCustomNetworkConfiguration({ + chainId: '0x1337', + nativeCurrency: 'TEST', + rpcEndpoints: [ + buildCustomRpcEndpoint({ + networkClientId: 'AAAA-AAAA-AAAA-AAAA', + url: 'https://test.network', + }), + ], + }), + }, + }, + infuraProjectId, + }, + async ({ controller, messenger }) => { + const fakeProvider = buildFakeProvider([ + // Called during provider initialization + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + // Called via `lookupNetwork` directly + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + ]); + const fakeNetworkClient = buildFakeClient(fakeProvider); + mockCreateNetworkClient() + .calledWith({ + chainId: '0x1337', + rpcUrl: 'https://test.network', + ticker: 'TEST', + type: NetworkClientType.Custom, + }) + .mockReturnValue(fakeNetworkClient); + await controller.initializeProvider(); + + const lookupNetworkPromise = controller.lookupNetwork(); + messenger.clearSubscriptions(); + await lookupNetworkPromise; + }, + ); + }); + }); + + describe('if removing the networkDidChange subscription fails for an unknown reason', () => { + it('re-throws the error', async () => { + const infuraProjectId = 'some-infura-project-id'; + + await withController( + { + state: { + selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA', + networkConfigurationsByChainId: { + '0x1337': buildCustomNetworkConfiguration({ + chainId: '0x1337', + nativeCurrency: 'TEST', + rpcEndpoints: [ + buildCustomRpcEndpoint({ + networkClientId: 'AAAA-AAAA-AAAA-AAAA', + url: 'https://test.network', + }), + ], + }), + }, + }, + infuraProjectId, + }, + async ({ controller, messenger }) => { + const fakeProvider = buildFakeProvider([ + // Called during provider initialization + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + // Called via `lookupNetwork` directly + { + request: { + method: 'eth_getBlockByNumber', + }, + response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE, + }, + ]); + const fakeNetworkClient = buildFakeClient(fakeProvider); + mockCreateNetworkClient() + .calledWith({ + chainId: '0x1337', + rpcUrl: 'https://test.network', + ticker: 'TEST', + type: NetworkClientType.Custom, + }) + .mockReturnValue(fakeNetworkClient); + await controller.initializeProvider(); + + const lookupNetworkPromise = controller.lookupNetwork(); + const error = new Error('oops'); + jest + .spyOn(messenger, 'unsubscribe') + .mockImplementation((eventType) => { + if (eventType === 'NetworkController:networkDidChange') { + throw error; + } + }); + await expect(lookupNetworkPromise).rejects.toThrow(error); + }, + ); + }); + }); + lookupNetworkTests({ expectedNetworkClientType: NetworkClientType.Custom, initialState: {