fix: Fix Firefox detatched-window memory leak by avoiding DocumentPictureInPicture instantiation in Snow hook cp-13.39.2 - #44352
Conversation
|
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. |
metamask-ci
Bot
added
INVALID-PR-TEMPLATE
MajorLift
changed the title
Builds ready [01a9491]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 9 warn · 🔴 1 fail)
Bundle sizes
|
The extension consumes snow.prod.js as a LavaMoat static shim; patching only the source file left the shipped bundle unchanged.
|
@metamaskbot update-policies |
|
LavaMoat validation is still running. Please retry |
Builds ready [4ea9538]
⚡ Performance Benchmarks (Total: 🟢 12 pass · 🟡 11 warn · 🔴 1 fail)
Bundle sizes
|
The lockfile still pinned hash=693c32 from the source-only patch, so installs resolved the stale patched artifact and the shipped bundle kept the instance-form hook.
|
Builds ready [d4dd55f]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 10 warn · 🔴 0 fail)
Bundle sizes
|
MajorLift
changed the title
MajorLift
changed the title
DocumentPictureInPicture instantiation in Snow hook cp-13.39.1
| - if (!win?.documentPictureInPicture?.requestWindow) { | ||
| + // Reach requestWindow through the interface object's prototype rather | ||
| + // than win.documentPictureInPicture: the attribute getter lazily | ||
| + // instantiates a per-window DocumentPictureInPicture, and on Firefox | ||
| + // (which shipped the API in 151) the instance's preserved wrapper pins | ||
| + // the entire window realm after the window closes, leaking every | ||
| + // protected window's document. Prototype access does not instantiate, | ||
| + // and instance method lookup falls through to the patched prototype. | ||
| + // Upstream: https://github.com/LavaMoat/snow/pull/171 | ||
| + const proto = win?.DocumentPictureInPicture?.prototype; | ||
| + if (!proto?.requestWindow) { | ||
| return; | ||
| } | ||
| - win.documentPictureInPicture.requestWindow = hook(win, win.documentPictureInPicture.requestWindow, hookDocumentPictureInPicture); | ||
| + proto.requestWindow = hook(win, proto.requestWindow, hookDocumentPictureInPicture); |
There was a problem hiding this comment.
The key fix is just the three LoC here.
| - if (!win?.documentPictureInPicture?.requestWindow) { | ||
| + const proto = win?.DocumentPictureInPicture?.prototype; | ||
| + if (!proto?.requestWindow) { | ||
| return; | ||
| } | ||
| - win.documentPictureInPicture.requestWindow = hook(win, win.documentPictureInPicture.requestWindow, hookDocumentPictureInPicture); | ||
| + proto.requestWindow = hook(win, proto.requestWindow, hookDocumentPictureInPicture); |
There was a problem hiding this comment.
Same change as above
| diff --git a/snow.prod.js b/snow.prod.js | ||
| index 62108e44f9a71377a777caa6d8757582e0c6b657..1193c272c39595f9cefccf16ffa4cdfa9bb3d5d1 100644 | ||
| --- a/snow.prod.js | ||
| +++ b/snow.prod.js |
There was a problem hiding this comment.
This is just the newly emitted build.
| "@keystonehq/bc-ur-registry-eth": "^0.22.1", | ||
| "@lavamoat/lavadome-react": "0.0.20", | ||
| "@lavamoat/snow": "^2.0.4", | ||
| "@lavamoat/snow": "patch:@lavamoat/snow@npm%3A2.0.4#~/.yarn/patches/@lavamoat-snow-npm-2.0.4-6af84c19b4.patch", |
There was a problem hiding this comment.
Fix applied as yarn patch.
MajorLift
changed the title
DocumentPictureInPicture instantiation in Snow hook cp-13.39.1DocumentPictureInPicture instantiation in Snow hook cp-13.39.2
HowardBraham
added
the
retry-ci
github-actions
Bot
removed
the
retry-ci
metamaskbot
added
the
release-13.41.0
metamaskbot
added
release-13.40.0
|
Missing release label release-13.40.0 on PR. Adding release label release-13.40.0 on PR and removing other release labels(release-13.41.0), as PR was added to branch 13.40.0 when release was cut. |
🧪 Validation RunVerdict: ✅ reproduced — Claim: the snow hook's per-instance accessor assignment retains closed windows, and moving it to the prototype releases them. Both halves reproduce against the patch as written: 8 of 8 windows retained before, 0 of 8 after, in every one of 8 replicates. Evidence: 6 replicates, clean box, controls valid in every run · five instruments, one run · Firefox 153.0 headless · captured retroactively for the pr-validate showcase, not at review time Note Trial run of the MetaMask evidence skills — Warning This comment previously reported the fix arm at 0 retained and a verdict of proven. That was wrong and is corrected here. Those numbers came from three runs on a loaded, disk-constrained machine. Six replicates on a clean isolated box return The leak is in none of the changed lines — it lives in a native window's lifecycle a pre-existing hook triggers, so no acquire/release pair in the diff accounts for it. Reading can't settle it, so the mechanism was run against controls.
Six consecutive replicates, each a fresh browser launch, every one Re-run on a second isolated box after the labels were fixed; the figures reproduce byte-for-byte. What this establishes: the pre-fix pattern retains every closed window — What the synthetic pair could not establish: that the prototype assignment fixes it. Note The synthetic arms model something the patch does not do. The assignment target, isolated
The Left unmeasured: this varies the target inside a chrome-context harness, where the installed functions are built in the harness's realm rather than the page's. That is constant across every arm and so cannot produce the Instrument battery — what could see it and what could not
Collected because a memory investigation collects the battery regardless of which member is expected to move — the joint pattern is what localizes the retention. Here it says the retained thing is a wrapper and a closure, not document byte-mass: layout and style are already torn down at Note An earlier version of this harness printed Reproducing it — one chrome-context call per arm, nothing persists between callsHarness: Each arm opens N windows, applies its treatment, closes them, forces 15× GC+CC, and counts surviving weak references — all inside a single marionette call. That matters: a page global written in one marionette call is Run it on an isolated machine. The discrepancy that produced the retracted numbers above came from measuring on a contended host; the same harness on a clean box is stable to the replicate. The fix under test — function hookRequest(win) {
- if (!win?.documentPictureInPicture?.requestWindow) {
+ const proto = win?.DocumentPictureInPicture?.prototype;
+ if (!proto?.requestWindow) {
return;
}
- win.documentPictureInPicture.requestWindow = hook(win, win.documentPictureInPicture.requestWindow, hookDocumentPictureInPicture);
+ proto.requestWindow = hook(win, proto.requestWindow, hookDocumentPictureInPicture);
} |
This branch was previously deployed
5 participants
Motivation
On Firefox, every closed MetaMask window (popup, notification, home) is retained in memory (#42891 and recent internal reports).
win?.documentPictureInPicture?.requestWindowin every window. The read lazily instantiates a per-windowDocumentPictureInPicture, and Firefox's cycle collector fails to break the instance's preserved-wrapper cycle after window close, pinning the entire realm.Description
Applies the upstream Snow fix (LavaMoat/snow#171) as a Yarn patch: the hook moves from the per-window instance to
DocumentPictureInPicture.prototype, reached through the interface object, which does not invoke the instance-creating getter.thisstays the instance, same wrapped native. Snow's PiP realm monitoring is preserved.snow.prod.jsandsnow.jsin addition tosrc/request.js: the extension consumes Snow's prebuilt bundle as a LavaMoat static shim (LavamoatPlugin), and webpack never processes@lavamoat/snow/**.@lavamoat/snowis bumped to a release containing the upstream fix. The bump is the removal trigger.Cherry-pick candidacy
Verification
d4dd55f), probed directlyRelated issues
Fixes: #42891
Manual testing steps
about:debugging→ Load Temporary Add-on. Onboard with a test SRP.DocumentPictureInPicture.prototype.requestWindow.name→"open"(stock:"requestWindow").about:memory→ Minimize memory usage → Measure → filterdetached: nomoz-extensionUI-document cohorts (stock: ~70 entries / ~105MB per cycle).Screenshots/Recordings
Heap-graph traces, memory reports, and retaining-path analysis: see the verification bundle above.
Pre-merge author checklist
CHANGELOG entry: Fixed a memory leak on Firefox where closed MetaMask windows were retained in memory, growing extension memory use with every popup open