-
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)
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, thank you very much for the fix. I'm another affected user and could verify that the change is resolving the memory leak. Just to be seen as an independent verification and that no other hidden issue is causing this (smoke test).
OS: Arch Linux
Firefox: 146.0.1
Test setup:
- Verified the issue is present with locally build "metamask-extension": The memory leak is even present when firefox is started with the extension and idling afterwards.
- Recompiled "metamask-extension" with a locally built "snaps" with the patch applied:
- Memory leak did not appear after ~15 mins of idling
- With connection to dApp: No memory leak
- No memory leak after ~4 hours of general usage.
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.
Hmm, not sure how I didn't spot the detached windows when testing the first time. It doesn't seem like the changes in this PR fixes the issue though, I still see detached windows after garbage collection has run 🤔
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