-
Notifications
You must be signed in to change notification settings - Fork 639
fix: memory leak from detached snap iframes #3790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts
Outdated
Show resolved
Hide resolved
|
looks good to me |
| // Navigate to about:blank before removing to ensure the browser | ||
| // unloads the previous document and cleans up its event listeners. | ||
| // Without this, Firefox may keep the detached iframe's document alive | ||
| // with all its event listeners, causing a memory leak. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a way to reproduce/prove this? When I tested locally, about:memory did not show any references to Snaps iframes after they were removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for getting to this:
Reproduction environment:
Firefox 146.0.1
MacOS 26.2
Steps for reproduction:
- Completely quit firefox & reopen to a fresh window
- Unlock metamask, close extension window
about:memory-> GC- Open a tab, navigate to app.aave.com (i think any dapp works). Connect wallet (disconnect if already connected)
- Wait a minute
about:memory-> GC -> Measure & Save a memory report- Wait another few minutes
about:memory-> GC -> Measure & Save a memory report
Evidence of Memory Leak
Controlled Test (Firefox)
Fresh browser profile, measured with about:memory after forcing GC:
| Time | Action | Active | Detached | Leaked |
|---|---|---|---|---|
| 0:00 | Fresh unlock | 0 | 0 | 0 MB |
| +1 min | Connect to dapp | 5 | 1 | ~7 MB |
| +5 min | Waiting around | 3 | 8 | ~47 MB |
| +7 min | Final measurement | 4 | 12 | ~68 MB |
Result: 68 MB leaked in 7 minutes of normal usage
What's happening
- Snaps create iframes at
execution.metamask.io - When terminated,
iframe.remove()is called - But event listeners keep references → iframes stay in memory
- Firefox reports them as
top(none)/detached/(removed from DOM, still in memory)
Note
Addresses memory leaks in snap execution lifecycle.
IframeExecutionService.terminateJob, cast toHTMLIFrameElement, navigate toabout:blank, then remove the iframe to ensure the prior document unloads (notably for Firefox)BaseSnapExecutor.onTerminate, removeunhandledrejectionanderrorlisteners if registered and clear handler refs to enable garbage collectionWritten by Cursor Bugbot for commit 4e1b218. This will update automatically on new commits. Configure here.
This fix should address:
MetaMask/metamask-extension#38847
MetaMask/metamask-extension#34428
MetaMask/metamask-extension#35567
My profiler showed a firefox extension (Metamask) hogging > 10GB of RAM, and the digging lead to finding event listeners aren't cleaned up on termination