2079806 - Pages holding `cookieStore` leak as ghost windows if cookies change while in `bfcache`

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Willem Kaufmann]

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

Steps to reproduce

I personally encountered this bug in my daily work by having many long-lived github.com tabs open. I began to encounter keyboard stutters and stalls when interacting with these tabs, so I investigated further. A Firefox Profiler recording taken while typing showed the github.com content process's main thread stalling for up to ~2.4 s, almost entirely in cycle collection. about:memory then showed 45 ghost windows across the github.com content processes, 21 of them in that one process, which no longer had any live page. The cycle collector log showed each ghost window held by references the cycle collector could not account for, and scanning the process memory identified those as CookieChangeEvent objects.

A simple reproducer is as follows:

1. Create a directory with these two files:

   page.html

   <!doctype html><script>window.store = cookieStore;</script>
   

   other.html

   <!doctype html>other
   

   and serve it locally:

   python3 -m http.server 8000
   

2. Open http://localhost:8000/page.html in a new tab (tab A).

3. In tab A, navigate to http://127.0.0.1:8000/other.html. This is a different site, so page.html goes into the bfcache.

4. Open http://localhost:8000/other.html in a second tab (tab B) and run this in its console:

   for (let i = 0; i < 50; i++) document.cookie = "c" + i + "=" + Math.random();
   

5. Close tab A, which evicts page.html from the bfcache.

6. Keep tab B open so the localhost content process stays alive. Wait about a minute, then open about:memory, click Minimize memory usage, then Measure.

Reproduced on Firefox 146.0 (Fedora 42) in a fresh profile with no add-ons, with default Fission and bfcache settings.

Actual results

page.html is never freed. about:memory lists it under window-objects/top(none)/detached (later under ghost-windows) in the localhost content process, and it survives Minimize memory usage. It stays alive until that content process exits.

page.html is freed normally if either:

• it never touches cookieStore (no script in step 1), or
• no cookies change while it is in the bfcache (skip step 4).

Impact in real use

A signed-in github.com session accumulated 45 ghost windows (~1.7 GB attributed) across its content processes over three days. One webIsolated=https://github.com process held 21 ghost windows and no live page (2.9 GB resident). Every cycle collection there walked ~9 million objects and freed almost none, causing ~2 s main-thread stalls when typing in any github.com tab sharing that process.

What holds the windows

In that process's CC log, each ghost nsGlobalWindowInner was a CC root with more references than the graph explains. Scanning the process memory for pointers to those windows showed that the unexplained references were all CookieChangeEvent objects. The counts matched exactly for every window: 4 to 455 per window, 1,647 in total.

Expected results

After tab A is closed, page.html is freed and no longer appears under detached or ghost windows in about:memory, the same as when the page does not use cookieStore. For the real-use case, I would expect closed GitHub tabs to be freed, so that content process memory doesn't grow over days and typing doesn't stall.

Comment 1

[Willem Kaufmann]

Attachment: Bug 2079806 - Shut down `CookieStore` when its global is torn down. r?#cookie-reviewers

While a window is in the bfcache, CookieStoreNotifier queues cookie
change events in mDelayedDOMEvents, and each event holds a strong
reference to the window. When the bfcache entry was evicted, nothing
released the queue. The notifier is not cycle collected, and
CookieStore::Shutdown() only ran from unlink and the destructor, which
a page holding cookieStore never reached. As a result, the window leaked
as a ghost window for the lifetime of its content process.

Shut the CookieStore down in DisconnectFromOwner() and clear the
queued events when the notifier is disentangled to prevent the leaks.

Comment 2

[Willem Kaufmann]

Seems to affect Firefox builds on every platform from version 140 onwards.

Comment 3

[Manuel Bucher [:manuel]]

Thanks for the report! This looks like very throughout, including a very good looking patch, will potentially get to review (or someone else in cookies-reviewers). Could you determine the regression bug (with mozregression) since you've already been able to pinpoint this to the 140 version?

Comment 4

[Willem Kaufmann]

mozregression result:
Last good revision: a51b9d3e7251d81217a4d7b3622a1658e9a23dfc (2024-09-15)
First bad revision: 2990b799422bb25a82bba52fac2d55a77d652dfa (2024-09-16)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a51b9d3e7251d81217a4d7b3622a1658e9a23dfc&tochange=2990b799422bb25a82bba52fac2d55a77d652dfa

Integration builds for that range have expired, so it can't narrow further, but the pushlog contains bug 1475599, which introduced the Cookie Store API, which checks out.

I set regressed by to bug 1475599.

Comment 5

[Manuel Bucher [:manuel]]

Thanks, that does look very likely. Since you've noticed this on github, you might have found the cause for bug 2055234. (but that is just me matching things, could also be unrelateded. I'm putting into see_also for now nonetheless)

Comment 6

[Willem Kaufmann]

Sweet! Thank you for the prompt investigation & responses :)

Comment 7

[BugBot [:suhaib / :marco]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/16713f82e6e1
https://hg.mozilla.org/integration/autoland/rev/77a4e5e8157b
Shut down `CookieStore` when its global is torn down. r=cookie-reviewers,baku

Comment 9

[Henri Sivonen (:hsivonen)]

[Tracking Requested - why for this release]: Reportedly, this has bad effects on GitHub, so let's track this to ensure uplifts.

Comment 10

[rperta]

https://hg.mozilla.org/mozilla-central/rev/77a4e5e8157b

Comment 11

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected, along with ESR.

:willem.kaufmann, please make an uplift decision for beta and ESR:

• For beta, nominate the patch for beta uplift approval if the fix should be included in this release, or set status-firefox159 to wontfix if the fix can wait for the next release.
• For ESR, nominate the patch for the appropriate ESR uplift approval if the fix should be included in ESR, or set the ESR status flag(s) to wontfix if an uplift isn't needed. Each affected ESR release needs a decision.

See Requesting an Uplift for documentation on how to request an uplift.

For more information, please visit BugBot documentation.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I'm guessing you'll need to help with comment 11, baku.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: Bug 2079806 - Shut down `CookieStore` when its global is torn down.

While a window is in the bfcache, CookieStoreNotifier queues cookie
change events in mDelayedDOMEvents, and each event holds a strong
reference to the window. When the bfcache entry was evicted, nothing
released the queue. The notifier is not cycle collected, and
CookieStore::Shutdown() only ran from unlink and the destructor, which
a page holding cookieStore never reached. As a result, the window leaked
as a ghost window for the lifetime of its content process.

Shut the CookieStore down in DisconnectFromOwner() and clear the
queued events when the notifier is disentangled to prevent the leaks.

Original Revision: https://phabricator.services.mozilla.com/D332068

Comment 14

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Memory leak: windows kept alive because of the CookieStore API
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: The patch introduces a standard approach to free resources monitoring the lifetime of the parent object.
• String changes made/needed?: n/a
• Is Android affected?: yes

Comment 15

[Andrea Marchesini [:baku]]

Attachment: Bug 2079806 - Shut down `CookieStore` when its global is torn down.

While a window is in the bfcache, CookieStoreNotifier queues cookie
change events in mDelayedDOMEvents, and each event holds a strong
reference to the window. When the bfcache entry was evicted, nothing
released the queue. The notifier is not cycle collected, and
CookieStore::Shutdown() only ran from unlink and the destructor, which
a page holding cookieStore never reached. As a result, the window leaked
as a ghost window for the lifetime of its content process.

Shut the CookieStore down in DisconnectFromOwner() and clear the
queued events when the notifier is disentangled to prevent the leaks.

Original Revision: https://phabricator.services.mozilla.com/D332068

Comment 16

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: Memory leak: windows kept alive because of the CookieStore API
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: The patch introduces a standard approach to free resources monitoring the lifetime of the parent object.
• String changes made/needed?: n/a
• Is Android affected?: yes

Comment 17

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/9014d5cebc01
https://hg.mozilla.org/releases/mozilla-beta/rev/d0c7728c2d32

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/9520fe9958aa

Comment 19

[Manuel Bucher [:manuel]]

(uplift requests done by baku)