Skip to content

Commit ffa69e9

Browse files
mcmiremcmire
committed
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.
1 parent 7c38c24 commit ffa69e9

File tree

3 files changed

+272
-8
lines changed

3 files changed

+272
-8
lines changed

‎packages/network-controller/CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079))
1313

14+
### Fixed
15+
16+
- 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))
17+
- This error could occur if the NetworkController's messenger is cleared of subscriptions, as in a "destroy" step.
18+
1419
## [22.1.1]
1520

1621
### Changed

‎packages/network-controller/src/NetworkController.ts‎

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1300,10 +1300,30 @@ export class NetworkController extends BaseController<
13001300
let networkChanged = false;
13011301
const listener = () => {
13021302
networkChanged = true;
1303-
this.messagingSystem.unsubscribe(
1304-
'NetworkController:networkDidChange',
1305-
listener,
1306-
);
1303+
try {
1304+
this.messagingSystem.unsubscribe(
1305+
'NetworkController:networkDidChange',
1306+
listener,
1307+
);
1308+
} catch (error) {
1309+
// In theory, this `catch` should not be necessary given that this error
1310+
// would occur "inside" of the call to `#determineEIP1559Compatibility`
1311+
// below and so it should be caught by the `try`/`catch` below (it is
1312+
// impossible to reproduce in tests for that reason). However, somehow
1313+
// it occurs within Mobile and so we have to add our own `try`/`catch`
1314+
// here.
1315+
/* istanbul ignore next */
1316+
if (
1317+
!(error instanceof Error) ||
1318+
error.message !==
1319+
'Subscription not found for event: NetworkController:networkDidChange'
1320+
) {
1321+
// Again, this error should not happen and is impossible to reproduce
1322+
// in tests.
1323+
/* istanbul ignore next */
1324+
throw error;
1325+
}
1326+
}
13071327
};
13081328
this.messagingSystem.subscribe(
13091329
'NetworkController:networkDidChange',
@@ -1370,10 +1390,21 @@ export class NetworkController extends BaseController<
13701390
// in the process of being called, so we don't need to go further.
13711391
return;
13721392
}
1373-
this.messagingSystem.unsubscribe(
1374-
'NetworkController:networkDidChange',
1375-
listener,
1376-
);
1393+
1394+
try {
1395+
this.messagingSystem.unsubscribe(
1396+
'NetworkController:networkDidChange',
1397+
listener,
1398+
);
1399+
} catch (error) {
1400+
if (
1401+
!(error instanceof Error) ||
1402+
error.message !==
1403+
'Subscription not found for event: NetworkController:networkDidChange'
1404+
) {
1405+
throw error;
1406+
}
1407+
}
13771408

13781409
this.update((state) => {
13791410
const meta = state.networksMetadata[state.selectedNetworkClientId];

‎packages/network-controller/tests/NetworkController.test.ts‎

Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1579,6 +1579,109 @@ describe('NetworkController', () => {
15791579
});
15801580
});
15811581

1582+
describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => {
1583+
it('does not throw an error', async () => {
1584+
const infuraProjectId = 'some-infura-project-id';
1585+
1586+
await withController(
1587+
{
1588+
state: {
1589+
selectedNetworkClientId: infuraNetworkType,
1590+
},
1591+
infuraProjectId,
1592+
},
1593+
async ({ controller, messenger }) => {
1594+
const fakeProvider = buildFakeProvider([
1595+
// Called during provider initialization
1596+
{
1597+
request: {
1598+
method: 'eth_getBlockByNumber',
1599+
},
1600+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
1601+
},
1602+
// Called via `lookupNetwork` directly
1603+
{
1604+
request: {
1605+
method: 'eth_getBlockByNumber',
1606+
},
1607+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
1608+
},
1609+
]);
1610+
const fakeNetworkClient = buildFakeClient(fakeProvider);
1611+
mockCreateNetworkClient()
1612+
.calledWith({
1613+
chainId: ChainId[infuraNetworkType],
1614+
infuraProjectId,
1615+
network: infuraNetworkType,
1616+
ticker: NetworksTicker[infuraNetworkType],
1617+
type: NetworkClientType.Infura,
1618+
})
1619+
.mockReturnValue(fakeNetworkClient);
1620+
await controller.initializeProvider();
1621+
1622+
const lookupNetworkPromise = controller.lookupNetwork();
1623+
messenger.clearSubscriptions();
1624+
expect(await lookupNetworkPromise).toBeUndefined();
1625+
},
1626+
);
1627+
});
1628+
});
1629+
1630+
describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
1631+
it('re-throws the error', async () => {
1632+
const infuraProjectId = 'some-infura-project-id';
1633+
1634+
await withController(
1635+
{
1636+
state: {
1637+
selectedNetworkClientId: infuraNetworkType,
1638+
},
1639+
infuraProjectId,
1640+
},
1641+
async ({ controller, messenger }) => {
1642+
const fakeProvider = buildFakeProvider([
1643+
// Called during provider initialization
1644+
{
1645+
request: {
1646+
method: 'eth_getBlockByNumber',
1647+
},
1648+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
1649+
},
1650+
// Called via `lookupNetwork` directly
1651+
{
1652+
request: {
1653+
method: 'eth_getBlockByNumber',
1654+
},
1655+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
1656+
},
1657+
]);
1658+
const fakeNetworkClient = buildFakeClient(fakeProvider);
1659+
mockCreateNetworkClient()
1660+
.calledWith({
1661+
chainId: ChainId[infuraNetworkType],
1662+
infuraProjectId,
1663+
network: infuraNetworkType,
1664+
ticker: NetworksTicker[infuraNetworkType],
1665+
type: NetworkClientType.Infura,
1666+
})
1667+
.mockReturnValue(fakeNetworkClient);
1668+
await controller.initializeProvider();
1669+
1670+
const lookupNetworkPromise = controller.lookupNetwork();
1671+
const error = new Error('oops');
1672+
jest
1673+
.spyOn(messenger, 'unsubscribe')
1674+
.mockImplementation((eventType) => {
1675+
if (eventType === 'NetworkController:networkDidChange') {
1676+
throw error;
1677+
}
1678+
});
1679+
await expect(lookupNetworkPromise).rejects.toThrow(error);
1680+
},
1681+
);
1682+
});
1683+
});
1684+
15821685
lookupNetworkTests({
15831686
expectedNetworkClientType: NetworkClientType.Infura,
15841687
initialState: {
@@ -1889,6 +1992,131 @@ describe('NetworkController', () => {
18891992
});
18901993
});
18911994

1995+
describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => {
1996+
it('does not throw an error', async () => {
1997+
const infuraProjectId = 'some-infura-project-id';
1998+
1999+
await withController(
2000+
{
2001+
state: {
2002+
selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
2003+
networkConfigurationsByChainId: {
2004+
'0x1337': buildCustomNetworkConfiguration({
2005+
chainId: '0x1337',
2006+
nativeCurrency: 'TEST',
2007+
rpcEndpoints: [
2008+
buildCustomRpcEndpoint({
2009+
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
2010+
url: 'https://test.network',
2011+
}),
2012+
],
2013+
}),
2014+
},
2015+
},
2016+
infuraProjectId,
2017+
},
2018+
async ({ controller, messenger }) => {
2019+
const fakeProvider = buildFakeProvider([
2020+
// Called during provider initialization
2021+
{
2022+
request: {
2023+
method: 'eth_getBlockByNumber',
2024+
},
2025+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
2026+
},
2027+
// Called via `lookupNetwork` directly
2028+
{
2029+
request: {
2030+
method: 'eth_getBlockByNumber',
2031+
},
2032+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
2033+
},
2034+
]);
2035+
const fakeNetworkClient = buildFakeClient(fakeProvider);
2036+
mockCreateNetworkClient()
2037+
.calledWith({
2038+
chainId: '0x1337',
2039+
rpcUrl: 'https://test.network',
2040+
ticker: 'TEST',
2041+
type: NetworkClientType.Custom,
2042+
})
2043+
.mockReturnValue(fakeNetworkClient);
2044+
await controller.initializeProvider();
2045+
2046+
const lookupNetworkPromise = controller.lookupNetwork();
2047+
messenger.clearSubscriptions();
2048+
await lookupNetworkPromise;
2049+
},
2050+
);
2051+
});
2052+
});
2053+
2054+
describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
2055+
it('re-throws the error', async () => {
2056+
const infuraProjectId = 'some-infura-project-id';
2057+
2058+
await withController(
2059+
{
2060+
state: {
2061+
selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
2062+
networkConfigurationsByChainId: {
2063+
'0x1337': buildCustomNetworkConfiguration({
2064+
chainId: '0x1337',
2065+
nativeCurrency: 'TEST',
2066+
rpcEndpoints: [
2067+
buildCustomRpcEndpoint({
2068+
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
2069+
url: 'https://test.network',
2070+
}),
2071+
],
2072+
}),
2073+
},
2074+
},
2075+
infuraProjectId,
2076+
},
2077+
async ({ controller, messenger }) => {
2078+
const fakeProvider = buildFakeProvider([
2079+
// Called during provider initialization
2080+
{
2081+
request: {
2082+
method: 'eth_getBlockByNumber',
2083+
},
2084+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
2085+
},
2086+
// Called via `lookupNetwork` directly
2087+
{
2088+
request: {
2089+
method: 'eth_getBlockByNumber',
2090+
},
2091+
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
2092+
},
2093+
]);
2094+
const fakeNetworkClient = buildFakeClient(fakeProvider);
2095+
mockCreateNetworkClient()
2096+
.calledWith({
2097+
chainId: '0x1337',
2098+
rpcUrl: 'https://test.network',
2099+
ticker: 'TEST',
2100+
type: NetworkClientType.Custom,
2101+
})
2102+
.mockReturnValue(fakeNetworkClient);
2103+
await controller.initializeProvider();
2104+
2105+
const lookupNetworkPromise = controller.lookupNetwork();
2106+
const error = new Error('oops');
2107+
jest
2108+
.spyOn(messenger, 'unsubscribe')
2109+
.mockImplementation((eventType) => {
2110+
if (eventType === 'NetworkController:networkDidChange') {
2111+
throw error;
2112+
}
2113+
});
2114+
await expect(lookupNetworkPromise).rejects.toThrow(error);
2115+
},
2116+
);
2117+
});
2118+
});
2119+
18922120
lookupNetworkTests({
18932121
expectedNetworkClientType: NetworkClientType.Custom,
18942122
initialState: {

0 commit comments

Comments
 (0)