User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

1. Bookmark five video pages from YouTube and exit Firefox.
2. Launch Firefox and open the previously bookmarked video pages in the same tab successively.

Actual results:

In about:memory of the content process, you can see under explicit > window-objects > top > active the five "window" entries of all the bookmarked urls that were opened at the second step. This memory is not cleared after clicking at "Minimize memory usage".

I should note that opening videos from the recommendations in the same tab, ie. by left-clicking them, does not present the same problem. To reproduce it, you have to either open bookmarks, or directly enter urls in the urlbar.

Expected results:

On the release channel, the memory is cleared after clicking the "Minimize memory usage" button, or after you wait for some time.

mozregression result:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=89f7028049edb2eca8e5f8731450278b33f84c27&tochange=f7b553bd9c0b77b0130d377f9a9c696c8c611a01

Set release status flags based on info from the regressing bug 1694538

Steven, could you set a priority and severity for this bug?

This sounds similar to bug 1722694.

I didn't spot that bug 1694538 had been flagged as regressor, but I ended up coming to the same conclusion.

That patch changed delegate marking to always mark keys the color of the delegate regardless of the color of the map. This causes problems because marking the key may cause the map to be marked too, e.g. via the global. This can lead to lots of stuff ending up black that otherwise would be gray and hence blocking cycle collection.

I'm not sure why this was changed, although I expect it's something we
discussed at the time.

I changed the two places that definitely need it but I'm not sure whether
markEntries needs to change too.

[Tracking Requested - why for this release]: This bug kind of lingered for a bit, but we should backport this to beta if possible. It sounds severe. There have been an uptick in reports of high JS memory usage in the last few months (see the bugs blocking bug 1683140) so maybe this will help with some issues users are seeing.

The situation of interest here is when you have a gray weakmap containing a CCW of a black key (in its own compartment). The CCW is a member of a reference cycle that does not include the map. (The CCW is also "reachable" from the map, in some sense.)

Say we mark the CCW gray (min(map color, delegate color) aka proxyPreserveColor). The cycle including the CCW is unlinked, and the key is removed from the map. The map itself remains alive because it is reachable from a non-garbage C++ object. The CCW can be regenerated from its target, and it will not be found in the map.

This relies on the map not marking the key. It appears that the cycle collector uses WeakMapTraceAction::Skip [1], so i believe it will not mark the key.

For correctness, I believe the existing code is necessary. But this delegate -> key edge is conservative, apparently too conservative for youtube's usage patterns.

[1] https://searchfox.org/mozilla-central/rev/9bc5dcea99c59dc18eae0de7064131aa20cfbb66/xpcom/base/CycleCollectedJSRuntime.cpp#392

After attempting to create a test case, the above scenario isn't a problem. Specifically, the entry will not be dropped because the CCW key will end up getting marked (at least gray) if its delegate is live, because the map is still live. In order for the CCW key to not get marked at all after the cycle collector runs and unlinks its edge to the CCW key, the map would need to be dead. But if the map is dead, then there's no need to keep the CCW key around.

Or in terms of edges: the current code has

• min(map, key) -> value
• key -> delegate
• delegate -> key

The proposed change has

• min(map, key) -> value
• key -> delegate
• min(map, delegate) -> key

Those are only different if color(map) < color(delegate), and in particular the key will only be dropped if it is entirely unmarked, which means min(map, delegate)=unmarked. If either the map or the delegate is unmarked, there's no need to preserve the key.

So I think I've argued myself into the position that this change is good, and I should have kept it that way in the first place. (I wasn't confident, and so went with the more conservative option, even though I wasn't sure it was causing any of the issues I was seeing.)

Maybe there are some tests that could be added to test_weakmaps.html? Or would that not have caught something like this?

https://hg.mozilla.org/mozilla-central/rev/383bc0cb1379

Happy to take a Beta uplift request for this (and ESR91), but I don't think we need to track for this specific release given the age of the regression.

Hi, can you confirm that this is fixed in the latest nightly?

Yes, I can confirm it's fixed. Thank you.

(In reply to tgn-ff from comment #14)
Great, thanks for filing.

Comment on attachment 9244351 [details]
Bug 1717553 - Take WeakMap mark color into account when marking delegates r?sfink

Beta/Release Uplift Approval Request

• User impact if declined: Possible memory leak / increased memory use on popular sites.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a complex area of code. However the patch reverts to the original behaviour before the regressing bug landed.
• String changes made/needed:

Comment on attachment 9244351 [details]
Bug 1717553 - Take WeakMap mark color into account when marking delegates r?sfink

Approved for 94.0b4.

https://hg.mozilla.org/releases/mozilla-beta/rev/d771826487ce

Please nominate this for ESR91 approval.

Comment on attachment 9244351 [details]
Bug 1717553 - Take WeakMap mark color into account when marking delegates r?sfink

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes a memory leak on populate sites including youtube.
• User impact if declined: Increased memory use.
• Fix Landed on Version: 95
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This restores the original weakmap behaviour that was changed in FF90. The fix has been on central and beta for a month without causing problems.
• String or UUID changes made by this patch:

Comment on attachment 9244351 [details]
Bug 1717553 - Take WeakMap mark color into account when marking delegates r?sfink

Approved for 91.4esr.

https://hg.mozilla.org/releases/mozilla-esr91/rev/aa9b5e2a7c66