fix: background bridge event leak by wenfix · Pull Request #29040 · MetaMask/metamask-mobile

wenfix · 2026-04-20T12:10:44Z

Description

BackgroundBridge subscribes to seven controller messenger events in its constructor but onDisconnect() only tore down three. After a session disconnects, the leaked subscriptions kept firing — emitting chainChanged / accountsChanged notifications and lock/unlock events to dead sessions.

Root causes of the four leaks:

SelectedNetworkController:stateChange — missing from onDisconnect() entirely.
KeyringController:lock / unlock — subscribed with this.onLock.bind(this) / this.onUnlock.bind(this) inline, so each call produced a fresh reference that couldn't be matched for unsubscription.
PermissionController:stateChange — subscribed with an anonymous arrow function, same reference-matching problem.

Fix:

Store stable bound references (this._handleLock, this._handleUnlock, this._handlePermissionControllerStateChange) in the constructor and pass them to both subscribe and tryUnsubscribe.
Add matching tryUnsubscribe calls for all four events in onDisconnect().
Make onDisconnect() idempotent: early-return on this.disconnected, and convert the existing CAIP unsubscribe calls (which throw on a missing sub) to tryUnsubscribe. onDisconnect is invoked from ~6 call sites and could previously throw on a second call.

Affects both SDKConnectV2 and WalletConnect since they share this class.

Changelog

CHANGELOG entry: Fixed a subscription leak in BackgroundBridge where disconnected SDK and WalletConnect sessions could continue to receive network, account, permission, and lock/unlock notifications.

Related issues

Fixes: WAPI-1367

Manual testing steps

Feature: BackgroundBridge subscription teardown

  Scenario: SDKConnectV2 session stops receiving state events after disconnect
    Given a dapp has an active SDKConnectV2 session with the wallet
    And dev logs for notifyChainChanged / notifySelectedAddressChanged are being observed

    When the user disconnects the session
    And the user switches networks
    And the user switches the selected account
    And the user locks and unlocks the wallet
    Then no chain-changed, account-changed, or lock-state notifications are emitted to the disconnected session

  Scenario: WalletConnect v2 session stops receiving state events after disconnect
    Given a dapp has an active WalletConnect v2 session with the wallet

    When the user disconnects the session
    And the user performs the same network / account / lock actions
    Then no chain-changed or account-changed notifications are emitted to the disconnected session

  Scenario: Calling onDisconnect multiple times is safe
    Given a BackgroundBridge instance has been disconnected

    When onDisconnect is invoked again
    Then no unsubscribe throws and no teardown work repeats

Screenshots/Recordings

N/A — no UI changes.

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
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 format if applicable
I've applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Performance checks (if applicable)

I've tested on Android
- Ideally on a mid-range device; emulator is acceptable
I've tested with a power user scenario
- Use these power-user SRPs to import wallets with many accounts and tokens
I've instrumented key operations with Sentry traces for production performance metrics
- See trace() for usage and addToken for an example

N/A — subscription-teardown fix, no user-visible flow or new instrumented operations.

For performance guidelines and tooling, see the Performance Guide.

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.

Note

Medium Risk
Changes connection teardown logic for provider sessions; mistakes could drop needed notifications or leave subscriptions around, impacting WalletConnect/SDK session behavior.

Overview
Fixes BackgroundBridge event-subscription leaks by using stable handler references for KeyringController lock/unlock and PermissionController:stateChange, and by adding missing teardowns (including SelectedNetworkController:stateChange) in onDisconnect().

Makes onDisconnect() idempotent (early-return when already disconnected) and switches CAIP-related unsubscriptions to tryUnsubscribe to avoid errors on repeated teardown calls. Adds focused unit tests asserting correct handler-based unsubscription and safe repeated onDisconnect() invocation.

^{Reviewed by Cursor Bugbot for commit e4e1440. Bugbot is set up for automated code reviews on this repo. Configure here.}

github-actions · 2026-04-20T12:10:53Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit bbcf777. Configure here.}

github-actions · 2026-04-28T22:11:40Z

🔍 Smart E2E Test Selection

Selected E2E tags: SmokeBrowser, SmokeConfirmations, SmokeNetworkExpansion, SmokeNetworkAbstractions, SmokeMultiChainAPI, SmokeWalletPlatform
Selected Performance tags: None (no tests recommended)
Risk Level: medium
AI Confidence: 85%

click to see 🤖 AI reasoning details

E2E Test Selection:

The changes to BackgroundBridge.js are bug fixes focused on:

Arrow function conversion for onLock/onUnlock: Previously subscribed with .bind(this) which creates a new function reference each time, making unsubscription impossible. Now using arrow class fields as stable references. This fixes a memory leak and ensures proper cleanup.
Named _handlePermissionControllerStateChange property: Previously an anonymous inline function passed to controllerMessenger.subscribe, making it impossible to unsubscribe. Now stored as a named property for proper cleanup.
onDisconnect idempotency guard: Added if (this.disconnected) return at the top to prevent double-disconnect issues (previously this.disconnected = true was set after destructuring, allowing race conditions).
Missing unsubscriptions added: SelectedNetworkController:stateChange, KeyringController:lock, KeyringController:unlock, and PermissionController:stateChange were not being unsubscribed on disconnect — now they are.
unsubscribe → tryUnsubscribe: More defensive cleanup that won't throw if a listener wasn't registered.

Impact Assessment:

BackgroundBridge is the core bridge between dApps and the wallet engine
Used by: BrowserTab (in-app browser), WalletConnect2Session, SDKConnect, SDKConnectV2, DaimoPayModal
The fixes affect all dApp connection lifecycle events (lock/unlock, permission changes, network changes, account changes)
These are correctness fixes — the behavior should be the same or better, but any regression could break dApp connectivity

Tags selected:

SmokeBrowser: BrowserTab directly uses BackgroundBridge for dApp connections; browser navigation and dApp interaction tests validate the connection lifecycle
SmokeConfirmations: All dApp-initiated transactions/signatures go through BackgroundBridge; lock/unlock and permission change events affect confirmation flows
SmokeNetworkExpansion: Solana dApp connections use BackgroundBridge; handleSolanaAccountChangedFromScopeChanges and related handlers are in the disconnect cleanup
SmokeNetworkAbstractions: Chain permission system and dApp chain switch requests go through BackgroundBridge's PermissionController subscription
SmokeMultiChainAPI: CAIP-25 session management relies on BackgroundBridge for permission state changes and session scope updates
SmokeWalletPlatform: EVM provider events (accountsChanged, chainChanged) for dApp communication are managed by BackgroundBridge; WalletConnect integration affects wallet platform tests

Dependent tags verified:

SmokeMultiChainAPI → requires SmokeNetworkAbstractions ✓ and SmokeNetworkExpansion ✓ (both included)
SmokeNetworkExpansion → SmokeConfirmations ✓ (included for Solana flows)

Performance Test Selection:
The changes are bug fixes to event subscription/unsubscription logic in BackgroundBridge. They fix memory leaks (which could theoretically improve performance over time) but don't directly affect UI rendering, data loading, account/network list components, or critical user flow timing. No performance test tags are warranted for these correctness fixes.

View GitHub Actions results

sonarqubecloud · 2026-04-28T22:29:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2026-04-28T22:30:41Z

✅ E2E Fixture Validation — Schema is up to date
12 value mismatches detected (expected — fixture represents an existing user).
View details

adonesky1

LGTM! Great work!

fix: background bridge event leak

0558aee

metamaskbotv2 Bot added the team-wallet-integrations Wallet Integrations team label Apr 20, 2026

github-actions Bot added the size-M label Apr 20, 2026

fix: format

466fad9

jiexi reviewed Apr 20, 2026

View reviewed changes

Comment thread app/core/BackgroundBridge/BackgroundBridge.js Outdated

refactor: use fat arrow class properties

eba9ee7

wenfix marked this pull request as ready for review April 21, 2026 09:31

wenfix requested a review from a team as a code owner April 21, 2026 09:31

Merge branch 'main' into wapi-1367

bbcf777

cursor Bot reviewed Apr 28, 2026

View reviewed changes

Comment thread app/core/BackgroundBridge/BackgroundBridge.js

use tryUnsubscribe

e4e1440

jiexi approved these changes Apr 30, 2026

View reviewed changes

adonesky1 approved these changes May 1, 2026

View reviewed changes

adonesky1 added this pull request to the merge queue May 1, 2026

Merged via the queue into main with commit ac1d674 May 1, 2026
106 checks passed

adonesky1 deleted the wapi-1367 branch May 1, 2026 19:22

github-actions Bot locked and limited conversation to collaborators May 1, 2026

metamaskbotv2 Bot added the release-7.77.0 Issue or pull request that will be included in release 7.77.0 label May 1, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: background bridge event leak#29040

fix: background bridge event leak#29040
adonesky1 merged 5 commits into
mainfrom
wapi-1367

wenfix commented Apr 20, 2026 •

edited by cursor Bot

Loading

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

sonarqubecloud Bot commented Apr 28, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

adonesky1 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

wenfix commented Apr 20, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

Pre-merge reviewer checklist

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 28, 2026

🔍 Smart E2E Test Selection

Uh oh!

sonarqubecloud Bot commented Apr 28, 2026

Quality Gate passed

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wenfix commented Apr 20, 2026 •

edited by cursor Bot

Loading