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

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Attachment: leak.html

Opening the attachment and then quitting the browser shows that we leak the document.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah it wouldn't surprise me if that's where the second unknown reference comes from.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Andrew McCreight [:mccr8]]

I should go ahead and finish up bug 990160 I guess. ;)

Comment 7

[William Chen [:wchen]]

After applying the patch in bug 990160 and tracing/unlinking mStyleSheetList and mRuleProcessor in nsXBLPrototypeResources I did not get the leak anymore.

Comment 8

[William Chen [:wchen]]

Attachment: 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.

Comment 9

[Andrew McCreight [:mccr8]]

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

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: 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...

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: binding manager ownership diagram

Comment 12

[William Chen [:wchen]]

Attachment: Traverse and unlink nsXBLPrototypeResources members.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Want to review this mccr8?

Comment 14

[Andrew McCreight [:mccr8]]

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?

Comment 15

[William Chen [:wchen]]

(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

Comment 16

[William Chen [:wchen]]

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

Wrong bug number in the commit message.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Is this something that affects older branches too? In other words, should we consider backporting this?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Andrew McCreight [:mccr8]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

So this is NPOTB/test-only from an uplift standpoint?

Comment 21

[Andrew McCreight [:mccr8]]

No, the patches definitely touch code we run everywhere, I just don't know if we can create a leak without enabling a pref.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 23

[Andrew McCreight [:mccr8]]

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.

Comment 24

[William Chen [:wchen]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

needinfo? on comment 25. If you're OK with trying to backport this to Aurora, please nominate it. Thanks!

Comment 27

[William Chen [: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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

They should also be nominated, thanks!

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

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 30

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Aurora+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a917c02f2145

Comment 33

[Joshua Mitchell (Inactive)]

This issue has been resolved - removing outdated keywords: regression-window-wanted