Skip to content

Commit 8077384

Browse files
authored
fix: Fix redux store event handler leak in WCv2 (MetaMask#27932)
## **Description** Fixes an issue where after a WCv2 connection could still send chainChanged events to the WC relay when the connection was removed. This was happening regardless of if the connection was removed on the wallet side or signaled removed from the WC side. This PR fixes that by properly unsubscribing from the store. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null Not user facing. ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/WAPI-1355 ## **Manual testing steps** 1. Open app in iOS expo build 2. Open js debugger 3. add breakpoint to onStoreChange in WalletConnect2Session.ts 4. Connect a new WC session via [demo app](https://react-app.walletconnect.com/) 5. You should see onStoreChange fire 6. Visit settings, experimental, and disconnect this session (long tap) 7. You should not see onStoreChange fire anymore 8. reconnect to demo dapp 9. You should see onStoreChange fire 10. disconnect from the demo dapp 11. you should not see onStoreChange fire anymore Visit https://react-app.walletconnect.com/ in the native browser and smoke test ## **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 Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/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-mobile/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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches WalletConnect v2 session lifecycle/cleanup, so mistakes could leave sessions partially disconnected or change disconnect timing. Logic is straightforward and covered by updated unit tests, limiting risk. > > **Overview** > Fixes a WalletConnect v2 listener leak by storing the Redux `store.subscribe` unsubscribe in `WalletConnect2Session` and invoking it during `removeListeners()`. > > Updates `WC2Manager.removeAll()` to **tear down every `WalletConnect2Session` via `removeListeners()` before clearing the sessions map**, preventing orphaned subscriptions/bridges when `session_delete` fires after local state is cleared. Tests were extended to assert the unsubscribe is called and `removeAll()` invokes `removeListeners()` for each session. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 7633be5. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent d4cdad3 commit 8077384

4 files changed

Lines changed: 35 additions & 4 deletions

File tree

app/core/WalletConnect/WalletConnect2Session.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ describe('WalletConnect2Session', () => {
219219
let mockClient: IWalletKit;
220220
let mockSession: SessionTypes.Struct;
221221
let mockNavigation: NavigationContainerRef;
222+
let mockStoreUnsubscribe: jest.Mock;
222223

223224
const testChainId = '0x89';
224225
const testNetworkClientId = `test-network-${parseInt(testChainId, 16)}`;
@@ -296,6 +297,9 @@ describe('WalletConnect2Session', () => {
296297
},
297298
});
298299

300+
mockStoreUnsubscribe = jest.fn();
301+
(store.subscribe as jest.Mock).mockReturnValue(mockStoreUnsubscribe);
302+
299303
session = new WalletConnect2Session({
300304
web3Wallet: mockClient,
301305
session: mockSession,
@@ -438,6 +442,7 @@ describe('WalletConnect2Session', () => {
438442

439443
await session.removeListeners();
440444

445+
expect(mockStoreUnsubscribe).toHaveBeenCalled();
441446
expect(mockOnDisconnect).toHaveBeenCalled();
442447
});
443448

app/core/WalletConnect/WalletConnect2Session.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ class WalletConnect2Session {
7272
private lastChainId: Hex;
7373
private isHandlingChainChange = false;
7474
private _isHandlingRequest = false;
75+
private storeUnsubscribe: (() => void) | null = null;
7576

7677
public session: SessionTypes.Struct;
7778

@@ -164,7 +165,7 @@ class WalletConnect2Session {
164165
this.checkPendingRequests();
165166
this.lastChainId = this.getCurrentChainId();
166167
// Subscribe to store changes to detect chain switches
167-
store.subscribe(this.onStoreChange.bind(this));
168+
this.storeUnsubscribe = store.subscribe(this.onStoreChange.bind(this));
168169
}
169170

170171
/**
@@ -791,6 +792,8 @@ class WalletConnect2Session {
791792
};
792793

793794
removeListeners = async () => {
795+
this.storeUnsubscribe?.();
796+
this.storeUnsubscribe = null;
794797
this.backgroundBridge.onDisconnect();
795798
};
796799

app/core/WalletConnect/WalletConnectV2.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,23 @@ describe('WC2Manager', () => {
735735
JSON.stringify({}),
736736
);
737737
});
738+
739+
it('calls removeListeners on each WalletConnect2Session before clearing local sessions', async () => {
740+
const removeListenersA = jest.fn().mockResolvedValue(undefined);
741+
const removeListenersB = jest.fn().mockResolvedValue(undefined);
742+
(manager as unknown as { sessions: Record<string, unknown> }).sessions = {
743+
'topic-a': { removeListeners: removeListenersA },
744+
'topic-b': { removeListeners: removeListenersB },
745+
};
746+
747+
await manager.removeAll();
748+
749+
expect(removeListenersA).toHaveBeenCalledTimes(1);
750+
expect(removeListenersB).toHaveBeenCalledTimes(1);
751+
expect(
752+
(manager as unknown as { sessions: Record<string, unknown> }).sessions,
753+
).toEqual({});
754+
});
738755
});
739756

740757
describe('WC2Manager isWalletConnect', () => {

app/core/WalletConnect/WalletConnectV2.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,15 @@ export class WC2Manager {
382382

383383
public async removeAll() {
384384
this.deeplinkSessions = {};
385+
// Tear down WalletConnect2Session (Redux subscription, BackgroundBridge)
386+
// before clearing the map. Otherwise session_delete may run after this.sessions
387+
// is empty and skip removeListeners(), leaking listeners and relay churn.
388+
const wcSessions = Object.values(this.sessions);
389+
await Promise.allSettled(
390+
wcSessions.map((wcSession) => wcSession.removeListeners()),
391+
);
392+
this.sessions = {};
393+
385394
const actives = this.web3Wallet.getActiveSessions() || {};
386395
Object.values(actives).forEach(async (session) => {
387396
this.web3Wallet
@@ -394,9 +403,6 @@ export class WC2Manager {
394403
});
395404
});
396405

397-
// Clear local sessions
398-
this.sessions = {};
399-
400406
await StorageWrapper.setItem(
401407
AppConstants.WALLET_CONNECT.DEEPLINK_SESSIONS,
402408
JSON.stringify(this.deeplinkSessions),

0 commit comments

Comments
 (0)