Closed Bug 1416296 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: smaug)

References

Details

Attachments

(1 file)

Reduced a fair bit.
(deleted), text/html
Details
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: → https://bugzilla.mozilla.org/show_bug.cgi?id=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. (deleted) —
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: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: