1729477 - Remove nsXULPrototypeCache::mStyleSheetTable.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1729477 - Remove nsXULPrototypeCache::mStyleSheetTable. r=smaug

With a very simple tweak to SheetLoadDataHashKey::KeyEquals we should
get the same kind of caching but in a much simpler way.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1729477 - Tweak SubresourceCacheValidationInfo to account for chrome uris. r=tnikkel

Turns out my patch above causes some failures because chrome:// channels
don't have cache information, so we conservatively assume they always
expire, which causes some interesting timing issues in a single test.
Fun stuff.

Tweak the code so that SubresourceCacheValidationInfo has the
pre-existing data:// URI special-case and also special-cases chrome://
URIs.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1259d2d1f247
Tweak SubresourceCacheValidationInfo to account for chrome uris. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/7457834fb7d0
Remove nsXULPrototypeCache::mStyleSheetTable. r=smaug

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cbf3101ed8a
BrowserTestUtils.openNewWindowWithFlushedXULCacheForMozSupports should now clear the stylesheet cache.

Comment 5

[Sandor Molnar[:smolnar]]

Backed out for causing devtools failures in browser_webconsole_sidebar_object_expand_when_message_pruned

Backout link: https://hg.mozilla.org/integration/autoland/rev/5ebe5fe5ffa2689630a1992482fabb9597537d1a

Push with failures

Failure log

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3fe2894da8e
Tweak SubresourceCacheValidationInfo to account for chrome uris. r=tnikkel

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a3fe2894da8e

Comment 8

[Emilio Cobos Álvarez (:emilio)]

So with this patch the test above also fails:

diff --git a/dom/xul/nsXULPrototypeCache.h b/dom/xul/nsXULPrototypeCache.h
index c1baee3691ce3..b489b3831b9f1 100644
--- a/dom/xul/nsXULPrototypeCache.h
+++ b/dom/xul/nsXULPrototypeCache.h
@@ -64,6 +64,9 @@ class nsXULPrototypeCache : public nsIObserver {
    * returns nullptr.
    */
   mozilla::StyleSheet* GetStyleSheet(nsIURI* aURI);
+  void ClearStyleSheets() {
+    mStyleSheetTable.Clear();
+  }
 
   /**
    * Store a style sheet in the cache. The key, style sheet's URI is obtained
diff --git a/layout/style/SharedStyleSheetCache.cpp b/layout/style/SharedStyleSheetCache.cpp
index ada6550b5a0f2..4b0fdce95f402 100644
--- a/layout/style/SharedStyleSheetCache.cpp
+++ b/layout/style/SharedStyleSheetCache.cpp
@@ -48,6 +48,9 @@ void SharedStyleSheetCache::Clear(nsIPrincipal* aForPrincipal,
 
   // No filter, clear all.
   if (!aForPrincipal && !aBaseDomain) {
+    if (nsXULPrototypeCache* cache = nsXULPrototypeCache::GetInstance()) {
+      cache->ClearStyleSheets();
+    }
     sInstance->mCompleteSheets.Clear();
     return;
   }

So whatever it is, it is a pre-existing problem, which is a bit unfortunate.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Sandor Molnar from comment #5)

Backed out for causing devtools failures in browser_webconsole_sidebar_object_expand_when_message_pruned

Ok, so what happens is that before this patch the load of global.css in that test-case was sync, because even though the previous test called clearSiteData, that didn't clear the XUL prototype cache.

But when the load is async, the SheetLoadData keeps a reference to the <link> element that loads it, and it ends up in Document::mPreloadService, which isn't cycle collected so isn't cleared up until Document::Destroy is called.

In this test-case, the docshell is kept alive by:

0x7f8562c85c80 [FragmentOrElement (xhtml) link (orphan) chrome://browser/content/browser.xhtml]
    --[GetParent(), mExtendedSlots->mContainingShadow]--> 0x7f8562c86970 [FragmentOrElement ([none]) #document-fragment (orphan) chrome://browser/content/browser.xhtml]
    --[mHost]--> 0x7f8562c02b80 [FragmentOrElement (XUL) menupopup id='webconsole-menu' (orphan) chrome://browser/content/browser.xhtml]
    --[mFirstChild]--> 0x7f8562c02c10 [FragmentOrElement (XUL) menuitem id='console-menu-store' (orphan) chrome://browser/content/browser.xhtml]
    --[[via hash] mListenerManager]--> 0x7f8552408500 [EventListenerManager]
    --[mListeners event=oncommand listenerType=3 [i]]--> 0x7f85626afec0 [CallbackObject]
    --[mCallback]--> 0xc4ebbe30868 [JS Object (Function - Menu.prototype._createMenuItems/</<)]
    --[**UNKNOWN SLOT 1**]--> 0xc4ebbe62b30 [JS Object (Call)]
    --[item]--> 0xc4ebbea51c8 [JS Object (Object)]
    --[click]--> 0xa4c96a7c5e0 [JS Object (Function - click)]
    --[baseshape_global]--> 0xefad8edc3f8 [JS Object (Sandbox)]
    --[console]--> 0x1d18acbbad90 [JS Object (Console)]
    --[baseshape_global]--> 0x304a81d8eea0 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0x7f85600abc00 [nsGlobalWindowInner # 35 inner chrome://devtools/content/webconsole/index.html]
    --[mDoc]--> 0x7f85526d8000 [Document normal (xhtml) chrome://devtools/content/webconsole/index.html]
    --[mDocumentL10n]--> 0x7f85552c07c0 [Localization]
    --[mContentSink]--> 0x7f854eecc400 [nsContentSink]
    --[mDocShell]--> 0x7f85600ab800 [nsDocLoader]

    Root 0x7f8562c85c80 is a ref counted object with 2 unknown edge(s).
    known edges:
       0x7f8562c86970 [FragmentOrElement ([none]) #document-fragment (orphan) chrome://browser/content/browser.xhtml] --[mFirstChild]--> 0x7f8562c85c80

So we leak it until shutdown... Which is not great.

The node is an orphan because it's removed here via:

0 Menu.prototype.popup/<(e = "[object MouseEvent]") ["resource://devtools/client/framework/menu.js":131:12]
1 hideContextMenu(hud = "[object Object]") ["chrome://mochitests/content/browser/devtools/client/webconsole/test/browser/head.js":414:8]
    this = [object Object]
2 showSidebarWithContextMenu() ["chrome://mochitests/content/browser/devtools/client/webconsole/test/browser/browser_webconsole_sidebar_object_expand_when_message_pruned.js":87:8]
3 InterpretGeneratorResume(gen = "[object Object]", val = "[object HTMLElement]", kind = ""next"") ["self-hosted":1482:33]
4 AsyncFunctionNext(val = "[object HTMLElement]") ["self-hosted":692:26]
    this = [object Object]

So the hideContextMenu call here.

So I think this just uncovered a very interesting pre-existing bug... Now, as for what to do:

• We could clear out SheetLoadData::mOwningNode after firing the load event. I believe that'd work, so I'm going to try that first, since it's the least invasive change.
• We could I think clear mPreloadService entries for style once the sheet loads, since following preloads should use the sync codepath. But I think that's sketchy as well perhaps, since PreloadKeys don't map 1:1 to SheetLoadDataHashKeys.
• We could hunt down the context menu command listener that is keeping the document via alive, but I think that'd be just a workaround at best, so not gonna pursue that.
• I think the fact that the document keeps alive the docshell like that via mDocumentL10n is also undesired (since Document::mDocumentContainer is a weak pointer). I could be wrong though. In any case not the root cause of the issue either.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

• We could clear out SheetLoadData::mOwningNode after firing the load event. I believe that'd work, so I'm going to try that first, since it's the least invasive change.
• We could I think clear mPreloadService entries for style once the sheet loads, since following preloads should use the sync codepath. But I think that's sketchy as well perhaps, since PreloadKeys don't map 1:1 to SheetLoadDataHashKeys.

Grr, so I was hoping this'd fix it but it doesn't, so there has to be something else specific to the test or something... Back to rr I guess :-)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Ok so that fixed one of the leaks, but there is still another...

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Got it.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1729477 - Prevent SheetLoadData from creating cycles for too long. r=smaug

This fixes a leak that my previous patch uncovers. See comment 9 for the
diagnostic, I had missed mRequestingNode in comment 10.

The observer change is not technically necessary but since observers can
theoretically keep alive random stuff as well, clean them up as well.

In particular, clear mOwningNode after we've fired load / error events
(which is what we need it for), and mRequestingNode once we call
LoadComplete, where it is no longer useful.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ede0ecb9bd84
Prevent SheetLoadData from creating cycles for too long. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d55ac30ba1c5
Remove nsXULPrototypeCache::mStyleSheetTable. r=smaug

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ede0ecb9bd84
https://hg.mozilla.org/mozilla-central/rev/d55ac30ba1c5

Comment 16

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #3)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1259d2d1f247
Tweak SubresourceCacheValidationInfo to account for chrome uris. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/7457834fb7d0
Remove nsXULPrototypeCache::mStyleSheetTable. r=smaug

== Change summary for alert #31303 (as of Tue, 14 Sep 2021 11:57:38 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
50%	welcome fcp	macosx1015-64-shippable-qr	warm webrender	76.42 -> 38.12
42%	welcome SpeedIndex	windows10-64-shippable-qr	warm webrender	163.79 -> 94.83
39%	welcome SpeedIndex	windows10-64-shippable-qr	warm webrender	163.96 -> 100.75
31%	welcome fcp	macosx1014-64-shippable-qr	warm webrender	66.21 -> 45.42
7%	welcome PerceptualSpeedIndex	windows10-64-shippable-qr	warm webrender	403.96 -> 374.08
...	...	...	...	...
5%	welcome SpeedIndex	linux1804-64-shippable-qr	warm webrender	1,220.00 -> 1,155.33

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31303

Comment 17

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Sandor Molnar from comment #5)

Backed out for causing devtools failures in browser_webconsole_sidebar_object_expand_when_message_pruned

Backout link: https://hg.mozilla.org/integration/autoland/rev/5ebe5fe5ffa2689630a1992482fabb9597537d1a

Push with failures

Failure log

== Change summary for alert #31263 (as of Fri, 10 Sep 2021 22:07:37 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	welcome loadtime	windows10-64-shippable-qr	warm webrender	26.17 -> 27.92

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31263

Comment 18

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #6)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3fe2894da8e
Tweak SubresourceCacheValidationInfo to account for chrome uris. r=tnikkel

== Change summary for alert #31306 (as of Tue, 14 Sep 2021 12:25:47 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
42%	welcome SpeedIndex	windows10-64-shippable-qr	warm webrender	164.25 -> 95.00
23%	welcome fcp	windows10-64-shippable-qr	warm webrender	49.10 -> 37.79
8%	welcome PerceptualSpeedIndex	windows10-64-shippable-qr	warm webrender	404.42 -> 373.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31306