Skip to content

Commit dc737ce

Browse files
mcmiremcmire
authored
Fix NetworkController to handle clearing of messenger (#5116)
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 d158e46 commit dc737ce

File tree

4 files changed

+280
-12
lines changed

4 files changed

+280
-12
lines changed

‎eslint-warning-thresholds.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
"n/no-unsupported-features/node-builtins": 4,
2020
"n/prefer-global/text-encoder": 4,
2121
"n/prefer-global/text-decoder": 4,
22-
"prettier/prettier": 116,
22+
"prettier/prettier": 114,
2323
"promise/always-return": 3,
2424
"promise/catch-or-return": 2,
2525
"promise/param-names": 8,

‎packages/network-controller/CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1414
### Fixed
1515

1616
- Fix `selectAvailableNetworkClientIds` so that it is properly memoized ([#5193](https://github.com/MetaMask/core/pull/5193))
17+
- 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))
18+
- This error could occur if the NetworkController's messenger is cleared of subscriptions, as in a "destroy" step.
1719

1820
## [22.1.1]
1921

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

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1334,10 +1334,30 @@ export class NetworkController extends BaseController<
13341334
let networkChanged = false;
13351335
const listener = () => {
13361336
networkChanged = true;
1337-
this.messagingSystem.unsubscribe(
1338-
'NetworkController:networkDidChange',
1339-
listener,
1340-
);
1337+
try {
1338+
this.messagingSystem.unsubscribe(
1339+
'NetworkController:networkDidChange',
1340+
listener,
1341+
);
1342+
} catch (error) {
1343+
// In theory, this `catch` should not be necessary given that this error
1344+
// would occur "inside" of the call to `#determineEIP1559Compatibility`
1345+
// below and so it should be caught by the `try`/`catch` below (it is
1346+
// impossible to reproduce in tests for that reason). However, somehow
1347+
// it occurs within Mobile and so we have to add our own `try`/`catch`
1348+
// here.
1349+
/* istanbul ignore next */
1350+
if (
1351+
!(error instanceof Error) ||
1352+
error.message !==
1353+
'Subscription not found for event: NetworkController:networkDidChange'
1354+
) {
1355+
// Again, this error should not happen and is impossible to reproduce
1356+
// in tests.
1357+
/* istanbul ignore next */
1358+
throw error;
1359+
}
1360+
}
13411361
};
13421362
this.messagingSystem.subscribe(
13431363
'NetworkController:networkDidChange',
@@ -1404,10 +1424,21 @@ export class NetworkController extends BaseController<
14041424
// in the process of being called, so we don't need to go further.
14051425
return;
14061426
}
1407-
this.messagingSystem.unsubscribe(
1408-
'NetworkController:networkDidChange',
1409-
listener,
1410-
);
1427+
1428+
try {
1429+
this.messagingSystem.unsubscribe(
1430+
'NetworkController:networkDidChange',
1431+
listener,
1432+
);
1433+
} catch (error) {
1434+
if (
1435+
!(error instanceof Error) ||
1436+
error.message !==
1437+
'Subscription not found for event: NetworkController:networkDidChange'
1438+
) {
1439+
throw error;
1440+
}
1441+
}
14111442

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

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

Lines changed: 238 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1580,6 +1580,111 @@ describe('NetworkController', () => {
15801580
});
15811581
});
15821582

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

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

1137311606
resolve(
11374-
controller.getNetworkConfigurationByNetworkClientId(networkClientId),
11607+
controller.getNetworkConfigurationByNetworkClientId(
11608+
networkClientId,
11609+
),
1137511610
);
1137611611
});
1137711612
});

0 commit comments

Comments
 (0)