leak when using <style scoped> in Web Components shadow trees

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: heycam, Assigned: wchen)

Tracking

(Blocks: 1 bug, {mlk})

Trunk
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 wontfix, firefox31 wontfix, firefox32 fixed, firefox33 fixed, firefox-esr24 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(4 attachments, 1 obsolete attachment)

Created attachment 8440513 [details]
leak.html

Opening the attachment and then quitting the browser shows that we leak the document.
Blocks: 811542
Whiteboard: [MemShrink]
0F247640 [nsCSSStyleSheet]
    --[mScopeElement]--> 0F2473A0 [FragmentOrElement (xhtml) div (orphan) file:///C:/Users/mozilla/Desktop/leak.html]
    --[GetParent(), mSlots->mContainingShadow]--> 18B7BDC0 [FragmentOrElement ([none]) #document-fragment (orphan) file:///C:/Users/mozilla/Desktop/leak.html]

    Root 0F247640 is a ref counted object with 2 unknown edge(s).
    known edges:
       1B24FA90 [FragmentOrElement (xhtml) style (orphan) file:///C:/Users/mozilla/Desktop/leak.html] --[mStyleSheet]--> 0F247640
nsXBLPrototypeResources::Traverse doesn't traverse the style sheets.  That seems like an obvious problem once there's an edge back from the style sheet to content.
Flags: needinfo?(wchen)
Flags: needinfo?(bzbarsky)
Also look at mRuleProcessor, while you are there.  That's something else I put on my giant diagram of XBL ownership as needing traverse/unlink when I looked at it awhile ago. ;)
Yeah it wouldn't surprise me if that's where the second unknown reference comes from.
Mmm.  Yes, nsXBLPrototypeResources::Traverse needs to traverse its sheets.

CSSRuleProcessor can also hold a reference to an element now, yes.  :(  And it holds strong refs to sheets, for that matter....
Flags: needinfo?(bzbarsky)
I should go ahead and finish up bug 990160 I guess. ;)
(Assignee)

Comment 7

3 years ago
After applying the patch in bug 990160 and tracing/unlinking mStyleSheetList and mRuleProcessor in nsXBLPrototypeResources I did not get the leak anymore.
Flags: needinfo?(wchen)
(Assignee)

Comment 8

3 years ago
Created attachment 8442358 [details] [diff] [review]
Traverse and unlink nsXBLPrototypeResources members.

https://tbpl.mozilla.org/?tree=Try&rev=c9c8b78ec5a9

Here is a patch on top of bug 990160 to fix this leak.
Depends on: 990160
Sorry I didn't fix this before.  I actually wrote basically that exact same patch before, but it was in a patch stack with a bunch of other stuff, and something in there was broken so I didn't get around to landing it. :)
Created attachment 8442374 [details]
XBL ownership diagram

Here's my two XBL ownership diagrams that I wrote out by hand.  They may be a little out of date by now...
Created attachment 8442375 [details]
binding manager ownership diagram
(Assignee)

Comment 12

3 years ago
Created attachment 8443840 [details] [diff] [review]
Traverse and unlink nsXBLPrototypeResources members.
Attachment #8442358 - Attachment is obsolete: true
Attachment #8443840 - Flags: review?(bugs)
Comment on attachment 8443840 [details] [diff] [review]
Traverse and unlink nsXBLPrototypeResources members.

Want to review this mccr8?
Attachment #8443840 - Flags: review?(bugs) → review?(continuation)
Comment on attachment 8443840 [details] [diff] [review]
Traverse and unlink nsXBLPrototypeResources members.

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

::: dom/xbl/nsXBLPrototypeResources.cpp
@@ +124,5 @@
> +
> +  for (int32_t i = 0, count = mStyleSheetList.Length(); i < count; ++i) {
> +    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mStyleSheetList[i]");
> +    cb.NoteXPCOMChild(static_cast<nsIStyleSheet*>(mStyleSheetList[i]));
> +  }

You should be able to replace this whole loop with something like  |CycleCollectionNoteChild(cb, mStyleSheetList, "mStyleSheetList");| and template magic will handle the ambiguity and the looping.

@@ +127,5 @@
> +    cb.NoteXPCOMChild(static_cast<nsIStyleSheet*>(mStyleSheetList[i]));
> +  }
> +
> +  NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mRuleProcessor");
> +  cb.NoteXPCOMChild(mRuleProcessor);

Please do |CycleCollectionNoteChild(cb, mRuleProcessor, "mRuleProcessor");| here

@@ +160,5 @@
>  
>  void
>  nsXBLPrototypeResources::RemoveStyleSheet(nsCSSStyleSheet* aSheet)
>  {
> +  mStyleSheetList.RemoveElement(aSheet);

Why are you removing the assert here?
Attachment #8443840 - Flags: review?(continuation) → review+
(Assignee)

Comment 15

3 years ago
(In reply to Andrew McCreight [:mccr8] from comment #14)
> Why are you removing the assert here?

Unlink may clear mStyleSheetList before ShadowRoot has a chance to remove a style sheet, so it's no longer true that sheets we try to remove are still in the list.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7591c7fdee63
Assignee: nobody → wchen
Flags: in-testsuite+
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/mozilla-central/rev/7591c7fdee63

Wrong bug number in the commit message.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED

Updated

3 years ago
Target Milestone: --- → mozilla33
Blocks: 957109
Is this something that affects older branches too? In other words, should we consider backporting this?
Flags: needinfo?(wchen)
Note that bug 957109 was filed when trunk was Gecko 29. Assuming this is low-risk, it appears to me that we should backport this to Aurora, Beta, and b2g30.
status-b2g-v1.3: --- → unaffected
status-b2g-v1.3T: --- → unaffected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
status-firefox30: --- → wontfix
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → fixed
status-firefox-esr24: --- → unaffected
This code is pref'ed off everywhere, except testing apparently.  Well, not the code actually involved with this patch and the blocking patch, but the full test case involves shadow DOM stuff.
So this is NPOTB/test-only from an uplift standpoint?
No, the patches definitely touch code we run everywhere, I just don't know if we can create a leak without enabling a pref.
Bug 957109 is hitting on multiple release branches, including beta which destined to be our next ESR. I would really like to see this long-running leak fixed across affected branches if at all possible.
Well, we could backport the two patches to avoid sheriff annoyance on ESR, or just disable a bunch of the shadow DOM tests that are leaking.  Let's see what William thinks.
(Assignee)

Comment 24

3 years ago
If we want to use the same patches in this bug we would also need to backport bug 1025725 and bug 990160. These patches do look low risk, but the impact is also fairly low. Web components are pref'ed off for the web and only used in a couple certified apps in B2G 2.0 (and we might not encounter this bug). I don't feel it's necessary to backport these fixes unless some certified app on B2G exhibit the leak.
Flags: needinfo?(wchen)
Sounds like we should consider an Aurora backport for v2.0 then? And then I can go the test disabling route for Beta31 and b2g30.
needinfo? on comment 25. If you're OK with trying to backport this to Aurora, please nominate it. Thanks!
Flags: needinfo?(wchen)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8443840 [details] [diff] [review]
Traverse and unlink nsXBLPrototypeResources members.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> Sounds like we should consider an Aurora backport for v2.0 then? And then I
> can go the test disabling route for Beta31 and b2g30.

That sounds fine to me. This patch depends on bug 990160 and conflicts with refactoring in bug 1025725. Do those bugs also need to be nominated or do they ride for free on this nomination?

Approval Request Comment
[Feature/regressing bug #]: Bug 806506
[User impact if declined]: Potential memory leak in B2G certified apps.
[Describe test coverage new/current, TBPL]: leakcheck currently detects intermittent leaks.
[Risks and why]: Low risk, patch adds more members to the cycle collector.
[String/UUID change made/needed]: None
Attachment #8443840 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wchen)
They should also be nominated, thanks!
If I have read the bug correctly, I don't think this impacts anything other than B2G 2.0 (Aurora). No need for Beta or ESR patches unless you want to clean up the test failures.

Ryan - Is this your understanding?
Comment on attachment 8443840 [details] [diff] [review]
Traverse and unlink nsXBLPrototypeResources members.

Aurora+
Attachment #8443840 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The leaks affect older branches, but the plan is to just disable the flaky tests there rather than backporting the various fixes across these bugs.
https://hg.mozilla.org/releases/mozilla-aurora/rev/a917c02f2145
status-b2g-v1.4: affected → wontfix
status-b2g-v2.0: affected → fixed
status-firefox31: affected → wontfix
status-firefox32: affected → fixed
This issue has been resolved - removing outdated keywords: regression-window-wanted
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.