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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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).

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Update stylesheets in nsXULPrototypeCache so that unused CSSStyleSheet objects can be freed. r=bholley

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2944fe5e190

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdbaae3b40f
Update stylesheets in nsXULPrototypeCache so that unused CSSStyleSheet objects can be freed. r=heycam

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove dead code from Loader::CreateSheet() that was added in P1. r=bz

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Actually, I'm just going to back out while I think more.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e6427b7695

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Attribute style sheet memory to last CSSStyleSheet attached to inner to avoid hiding memory in XUL cache. r=bz

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9eeb5a616d4a