Closed Bug 1025738 Opened 8 years ago Closed 8 years ago

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


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
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


(Reporter: heycam, Assigned: wchen)


(Blocks 1 open bug)


(Keywords: memory-leak, Whiteboard: [MemShrink])


(4 files, 1 obsolete file)

Attached file leak.html
Opening the attachment and then quitting the browser shows that we leak the document.
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. ;)
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)
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. :)
Attached image 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...
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+
(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.
Assignee: nobody → wchen
Flags: in-testsuite+

Wrong bug number in the commit message.
Closed: 8 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
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.
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.
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)
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.

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.
This issue has been resolved - removing outdated keywords: regression-window-wanted
You need to log in before you can comment on or make changes to this bug.