layout/reftests/webcomponents/basic-slot-6.html : YOU ARE LEAKING THE WORLD

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: nbp, Assigned: emilio)

Tracking

(Blocks 2 bugs)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Running:
  mach test layout/reftests/webcomponents/basic-slot-6.html

When testing with Bug 1437600 patch, leaking a JSRuntime causes a hard failure as not all the data registered by the JSRuntime are being unregistered.  This hard failure checks that we have no stale data remaining at the end of a process.
I don't have access to that bug, but that kind of failure looks more likely to be DOM related.
Component: Layout → DOM
Blocks: 1445619
Priority: -- → P3
the leaded urls and atoms look css related.
Component: DOM → CSS Parsing and Computation
Flags: needinfo?(emilio)
So, all those CSS urls are from nsLayoutStylesheetCache.

That cache is held alive by a leaked nsNodeInfoManager via nsLayoutStatics, at least. So back to DOM for now, though I'm still investigating.
Component: CSS Parsing and Computation → DOM
So, the nsNodeInfoManager is alive because the HTML document is alive. I've verified that the ShadowRoot is dead by the time we get to detect the leak, and that there are two shutdown cycle collections, not more.

Not really sure where to look at next...
We seem to be leaking the <slot>...
Ahá, I found the offending reference:

(rr) p tmp
$28 = (mozilla::dom::HTMLSlotElement *) 0x7f1cb02369d0
(rr) p tmp->OwnerDoc()
$29 = (nsHTMLDocument *) 0x7f1ca3f4d000
(rr) p tmp->OwnerDoc()->GetDocGroup()
$30 = (mozilla::dom::DocGroup *) 0x7f1ca0402b80
(rr) p $30->mSignalSlotList
$31 = nsTArray<RefPtr<mozilla::dom::HTMLSlotElement> > = {[(mozilla::dom::HTMLSlotElement *) 0x7f1cb02369d0]}
Ok, let me try to do something about that...
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Per bug 1341961, we don't run leak checking in XPCShell builds right now. So there's likely to be a lot of unfixed leaks. We may not even do the proper cleanup before shutdown there, so this could be a spurious leak.
Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

https://reviewboard.mozilla.org/r/242126/#review247930

Doesn't look right to traverse only. Or does something else somehow magically unlink?
Attachment #8973751 - Flags: review?(bugs) → review-
Comment on attachment 8973750 [details]
Bug 1445392: Make HTMLSlotElement slot change event stuff not linear.

https://reviewboard.mozilla.org/r/242124/#review247934

::: dom/base/DocGroup.h:118
(Diff revision 2)
>    // DocGroup.
>    bool* GetValidAccessPtr();
>  
>    // Append aSlot to the list of signal slot list, if it's not in it already
>    // list, and queue a mutation observer microtask.
> -  void SignalSlotChange(const mozilla::dom::HTMLSlotElement* aSlot);
> +  void SignalSlotChange(HTMLSlotElement&);

Please don't remove argument name

::: dom/html/HTMLSlotElement.h:85
(Diff revision 2)
> +
> +  // Whether we're in the signal slot list of our unit of related similar-origin
> +  // browsing contexts.
> +  //
> +  // https://dom.spec.whatwg.org/#signal-slot-list
> +  bool mInSignalSlotList { false };

Please use = and not {}
Attachment #8973750 - Flags: review?(bugs) → review+
Comment on attachment 8973752 [details]
Bug 1445392: Remove pending slots from the docgroup on unlink.

https://reviewboard.mozilla.org/r/242128/#review247938

This doesn't feel quite right. Could we for now just check gXPCOMThreadsShutDown before adding slot to docgroup's list?
I need to think how to deal with microtasks during shutdown in general

::: commit-message-06c13:10
(Diff revision 2)
> +
> +  mPendingMicroTaskRunnables.empty()
> +
> +In CycleCollectedJSContext.cpp.
> +
> +That microtask is posted from DocGroup::SignalSlotChange, from the

This stuff of course doesn't belong to a commit message

::: commit-message-06c13:14
(Diff revision 2)
> +
> +That microtask is posted from DocGroup::SignalSlotChange, from the
> +UnbindFromTree that happens on the <div> during unlink, and I suspect we should
> +prevent it, somehow.
> +
> +If this is a good excuse to pass down a "are we unlinking" bit to

It is not, IMO. In general passing around "we're unlinking is rather bad idea." Objects may get unlinked, or they may get deleted without unlinking.
Attachment #8973752 - Flags: review?(bugs)
Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

Moved to bug 1459698.
Attachment #8973751 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #18)
> It is not, IMO. In general passing around "we're unlinking is rather bad
> idea." Objects may get unlinked, or they may get deleted without unlinking.

That's a really fair point, agreed :)
Blocks: 1459704
Attachment #8973752 - Attachment is obsolete: true
Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

https://reviewboard.mozilla.org/r/242126/#review248196
Attachment #8973751 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/670ddc4fc00c
Make HTMLSlotElement slot change event stuff not linear. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/370b05966452
Avoid posting a slotchange event microtask during shutdown. r=smaug
https://hg.mozilla.org/mozilla-central/rev/670ddc4fc00c
https://hg.mozilla.org/mozilla-central/rev/370b05966452
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.