Closed Bug 1416296 Opened 2 years ago Closed 2 years ago

Leak of the world when the crashtest from bug 1415021 has dom.webcomponents.enabled=true.

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: smaug)

References

Details

Attachments

(1 file)

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a4c7a72f31

Should get reverted.

Given the messages from the log:

    WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.  FIX THIS!
    ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /builds/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp, line 1026

This is likely not a layout bug, but a DOM bug. Andrew, any chance you can find anybody to own this?
Flags: needinfo?(overholt)
(Adding dependency on bug 1415021, since that's where we landed the crashtest that's causing trouble here.)

Direct link to the testcase: https://bug1415021.bmoattachments.org/attachment.cgi?id=8925763

The crashtest does a bunch of different things in sequence (because it's fuzzer-generated), but AFAIK the key part for webcomponents usage is:

> try { o16 = document.createElement('area') } catch(e) { }
> try { o14 = document.createElement('body') } catch(e) { }
> try { o17 = o14.createShadowRoot() } catch(e) { }
> try { try { o17.appendChild(o16) } catch(e) { document.documentElement.appendChild(o16) } } catch(e) { }

We create a "body" element, and then call createShadowRoot to it, and then try to append an 'area' to that shadow-root. (And then try to append the area to our <html> element if that fails -- but it probably doesn't fail, I'd guess.)

I'll bet the rest of the testcase is unnecessary for the purposes of triggering this leak, and this ^^ is the key piece that triggers problematic leaky behavior.
Depends on: 1415021
(In reply to Daniel Holbert [:dholbert] from comment #2)
> > try { o16 = document.createElement('area') } catch(e) { }
> > try { o14 = document.createElement('body') } catch(e) { }
> > try { o17 = o14.createShadowRoot() } catch(e) { }
> > try { try { o17.appendChild(o16) } catch(e) { document.documentElement.appendChild(o16) } } catch(e) { }
> 
> We create a "body" element, and then call createShadowRoot to it, and then
> try to append an 'area' to that shadow-root. (And then try to append the
> area to our <html> element if that fails -- but it probably doesn't fail,
> I'd guess.)
> 
> I'll bet the rest of the testcase is unnecessary for the purposes of
> triggering this leak, and this ^^ is the key piece that triggers problematic
> leaky behavior.

Well, there may be other bit which may or may not make a difference, which is the body getting inserted into the document:

> try { document.documentElement.appendChild(o14) } catch(e) { }

But yeah, I agree that the leak is probably reducible :)
createShadowRoot isn't what we're implementing. Can we change the test?
Flags: needinfo?(overholt)
Priority: -- → P2
(In reply to Andrew Overholt [:overholt] from comment #4)
> createShadowRoot isn't what we're implementing. Can we change the test?

I mean, sure, it should work the same way with attachShadow({ mode: "open" }), so not a big deal in that regard.
Yes, it's reproducible with attachShadow `attachShadow({ mode: "open" })` too.
And I found that if I replace the following line:

> try { o15.content.append(document.documentElement, "") } catch(e) { }

with this:

> try { o15.content.append(document.documentElement) } catch(e) { }

the leak is gone.

The difference is: if you append more than one node at a time, a DocumentFragment is created to handle this, the nodes to append are moved to this DocumentFragment [1]. However, the above line will fail later due to "HierarchyRequestError".

I tried to append to another element, which also causes "HierarchyRequestError", e.g.

> try { o1.append(document.documentElement, "") } catch(e) { }

and the leak is gone too.
So, I have no idea so far :(


[1] https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/dom/base/nsINode.cpp#1722-1732
(In reply to Jessica Jong [:jessica] from comment #6)
> I tried to append to another element, which also causes
> "HierarchyRequestError", e.g.
> 
> > try { o1.append(document.documentElement, "") } catch(e) { }
> 
> and the leak is gone too.
> So, I have no idea so far :(

So, that's a pretty neat observation, I think, actually.

o15 is a template element, and that has a document fragment that doesn't look cycle collected:

  https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/html/HTMLTemplateElement.h#42

That probably should be cycle collected and the leak should go away, I would think...

But if this leaks it is because we insert incorrectly in the first place, and there are a couple of checks that look particularly fishy. In particular, we allow any document fragment to get inserted into an element[1], even if they contain an element with a shadow root. Looks to me that we can't really do that early-out, since we check for shadow roots and template elements in [2].

[1]: https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/nsINode.cpp#2075
[2]: https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/nsINode.cpp#1978
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> So, that's a pretty neat observation, I think, actually.
> 
> o15 is a template element, and that has a document fragment that doesn't
> look cycle collected:
> 
>  
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/dom/html/HTMLTemplateElement.h#42
> 
> That probably should be cycle collected and the leak should go away, I would
> think...

I think the HTMLTemplateContent does cycle collect its `content` here:

https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/dom/html/HTMLTemplateElement.cpp#46-57

or is anything wrong here?

> 
> But if this leaks it is because we insert incorrectly in the first place,
> and there are a couple of checks that look particularly fishy. In
> particular, we allow any document fragment to get inserted into an
> element[1], even if they contain an element with a shadow root. Looks to me
> that we can't really do that early-out, since we check for shadow roots and
> template elements in [2].
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/nsINode.cpp#2075
> [2]:
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/dom/base/nsINode.cpp#1978

Actually, when doing:

> try { o15.content.append(document.documentElement, "") } catch(e) { }
or
> try { o1.append(document.documentElement, "") } catch(e) { }

the elements are not really appended to either o15.content or o1, since the code early returns here:

https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/dom/base/nsINode.cpp#2217

so I'm not sure why their outcome differs.

In summary, the 2 conditions causing the leak are:

- o15.content.append(document.documentElement, "")
- o17 = o14.attachShadow({ mode: "open" })

It looks like the shadow host is causing the elements not to be released. So, I looked at the part where we attach shadow, and found that if we remove the `!GetShadowRoot()` check and force to use `OwnerDoc()` instead of `document` in `Element::UnbindFromTree`:

https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/dom/base/Element.cpp#2067-2079

the leak is gone!

But I still can't explain why `o1.append(document.documentElement, "")` has no such problem...
(In reply to Jessica Jong [:jessica] from comment #8)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> > So, that's a pretty neat observation, I think, actually.
> > 
> > o15 is a template element, and that has a document fragment that doesn't
> > look cycle collected:
> > 
> >  
> > https://searchfox.org/mozilla-central/rev/
> > bab833ebeef6b2202e71f81d225b968283521fd6/dom/html/HTMLTemplateElement.h#42
> > 
> > That probably should be cycle collected and the leak should go away, I would
> > think...
> 
> I think the HTMLTemplateContent does cycle collect its `content` here:
> 
> https://searchfox.org/mozilla-central/rev/
> a662f122c37704456457a526af90db4e3c0fd10e/dom/html/HTMLTemplateElement.cpp#46-
> 57
> 
> or is anything wrong here?

Err, your 110% right, I'll never learn to look at the right place for the cycle collection macros.

> > try { o15.content.append(document.documentElement, "") } catch(e) { }
> or
> > try { o1.append(document.documentElement, "") } catch(e) { }
> 
> the elements are not really appended to either o15.content or o1, since the
> code early returns here:
> 
> https://searchfox.org/mozilla-central/rev/
> a662f122c37704456457a526af90db4e3c0fd10e/dom/base/nsINode.cpp#2217

Hmm... So do we really reject appending the fragment, I guess... I'm about to debug it a bit more closely, but if I'm not wrong:

 * aNewChild is a document fragment containing document.documentElement and an empty textnode.
 * aParent is the template's doc fragment.

So we go through the ContentIsHostIncludingDescendantOf, and it fails...

> It looks like the shadow host is causing the elements not to be released.
> So, I looked at the part where we attach shadow, and found that if we remove
> the `!GetShadowRoot()` check and force to use `OwnerDoc()` instead of
> `document` in `Element::UnbindFromTree`:
> 
> https://searchfox.org/mozilla-central/rev/
> a662f122c37704456457a526af90db4e3c0fd10e/dom/base/Element.cpp#2067-2079
> 
> the leak is gone!
> 
> But I still can't explain why `o1.append(document.documentElement, "")` has
> no such problem...

Yeah... I'm investigating a bit more now. But we really don't want to do that. This is the kind of thing why using XBL for shadow dom gets annoying :(

See also bug 1217531, which may be a duplicate of this bug actually.
See Also: → 1217531
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> 
> Hmm... So do we really reject appending the fragment, I guess... I'm about
> to debug it a bit more closely, but if I'm not wrong:
> 
>  * aNewChild is a document fragment containing document.documentElement and
> an empty textnode.
>  * aParent is the template's doc fragment.
> 
> So we go through the ContentIsHostIncludingDescendantOf, and it fails...

Exactly, and that's the same for `o15.content.append` or `o1.append`.

> 
> > It looks like the shadow host is causing the elements not to be released.
> > So, I looked at the part where we attach shadow, and found that if we remove
> > the `!GetShadowRoot()` check and force to use `OwnerDoc()` instead of
> > `document` in `Element::UnbindFromTree`:
> > 
> > https://searchfox.org/mozilla-central/rev/
> > a662f122c37704456457a526af90db4e3c0fd10e/dom/base/Element.cpp#2067-2079
> > 
> > the leak is gone!
> > 
> > But I still can't explain why `o1.append(document.documentElement, "")` has
> > no such problem...
> 
> Yeah... I'm investigating a bit more now. But we really don't want to do
> that. This is the kind of thing why using XBL for shadow dom gets annoying :(
> 
> See also bug 1217531, which may be a duplicate of this bug actually.

Yeah, looks like the same problem...
So is there a minimal testcase for the leak?
Attached file Reduced a fair bit.
Here's a naive reduced test-case from the crashtest.
Assignee: nobody → emilio
err... didn't plan to take it, at least not just yet.
Assignee: emilio → nobody
Attachment #8928623 - Attachment mime type: text/plain → text/html
Assignee: nobody → bugs
Depends on: 1425759
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53072cbdc184
Enable shadow DOM in the crashtest now that it doesn't leak the XBL stuff. r=me
https://hg.mozilla.org/mozilla-central/rev/53072cbdc184
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.