Closed Bug 1278688 Opened 4 years ago Closed 4 years ago

nsXULPrototypeCache confuses about:memory style-sheet measurements for chrome windows

Categories

(Core :: CSS Parsing and Computation, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 2 obsolete files)

STR:

1) Launch fresh browser
2) Open about:memory, minimize, measure
3) Note that browser.xul contains about 1.7MB of stylesheets
4) Open new window
5) Close original window
6) Open about:memory, minimize, measure
7) Note that browser.xul now contains close to 0 memory for stylesheets

What is happening here is we only count the inner stylesheet memory against the first CSSStyleSheet object referencing.  The second window has close to 0 memory for its stylesheets because the CSSStyleSheet objects from the first window are still alive.

I dug a bit and it turns out this is due to the nsXULPrototypeCache hanging on to the sheets.  This should be easy to fix by simply updating the cache when we make a clone for a new document.  This will let older sheets clean up when possible.

The net result might save us an outer CSSStyleSheet, but will mostly be memory neutral.  The real benefit here is that the memory used in the nsXULPrototypeCache is less likely to be missed in about:memory since the sheet will always show in a xul window (if there is one with it open).
Ehsan, do you feel comfortable reviewing this?  I tried bz, bholley, and dbaron, but none of them are accepting reviews atm.

The purpose of this patch is to make sure we keep the freshest copy of the stylesheet in the XUL cache.  That way old CSSStyleSheet objects can be free'd when their windows are closed.  This is important for this memory reporting code to surface information properly:

https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#929

Without this patch the mSheets[0] ends up remaining the first loaded XUL stylesheet for that URL until firefox exits, which fails to report the memory if the first XUL window that loaded it was closed.
Attachment #8760936 - Flags: review?(ehsan)
Whiteboard: [MemShrink]
Summary: nsXULPrototypCache confuses about:memory style-sheet measurements for chrome windows → nsXULPrototypeCache confuses about:memory style-sheet measurements for chrome windows
Comment on attachment 8760936 [details] [diff] [review]
Update stylesheets in nsXULPrototypeCache so that unused CSSStyleSheet objects can be freed. r=bholley

Cameron said he could review this.  See comment 1 for the patch description.
Attachment #8760936 - Flags: review?(ehsan) → review?(cam)
Comment on attachment 8760936 [details] [diff] [review]
Update stylesheets in nsXULPrototypeCache so that unused CSSStyleSheet objects can be freed. r=bholley

Review of attachment 8760936 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, thanks.
Attachment #8760936 - Flags: review?(cam) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdbaae3b40f
Update stylesheets in nsXULPrototypeCache so that unused CSSStyleSheet objects can be freed. r=heycam
Boris pointed out the code added in Loader::CreateSheet() was not actually getting executed.  Remove this dead code.  Boris gave me r+ on IRC to land this.
Attachment #8761339 - Flags: review+
Actually, I'm just going to back out while I think more.
This is a much smaller patch that fixed the about:memory issue without touching the XUL cache.  I think my previous patches slightly pessimized the standard case of keeping the first window open and open/closing secondary windows.
Attachment #8760936 - Attachment is obsolete: true
Attachment #8761339 - Attachment is obsolete: true
Comment on attachment 8761349 [details] [diff] [review]
Attribute style sheet memory to last CSSStyleSheet attached to inner to avoid hiding memory in XUL cache. r=bz

r=me
Attachment #8761349 - Flags: review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eeb5a616d4a
Attribute style sheet memory to last CSSStyleSheet attached to inner to avoid hiding memory in XUL cache. r=bz
https://hg.mozilla.org/mozilla-central/rev/9eeb5a616d4a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.