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

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I don't have access to that bug, but that kind of failure looks more likely to be DOM related.

Comment 2

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

the leaded urls and atoms look css related.

Comment 3

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

leaked urls

Comment 4

[Emilio Cobos Álvarez (: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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

We seem to be leaking the <slot>...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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]}

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Ok, let me try to do something about that...

Comment 9

[Andrew McCreight [:mccr8]]

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 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1445392: Make HTMLSlotElement slot change event stuff not linear.

Also fix refcount churn in nsDOMMutationObserver and such.

This is a preliminar patch which I think is worth landing regardless of whether
the next is the right thing to do.

Review commit: https://reviewboard.mozilla.org/r/242124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242124/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

Ended up not being the cause of the leak, but worth doing as well IMO, or worth
removing nsDocument's traversal otherwise.

Review commit: https://reviewboard.mozilla.org/r/242126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242126/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1445392: Remove pending slots from the docgroup on unlink.

This fixes the leak. However, I suspect it's not the right thing to do, given
the same test-case asserts later with:

  mPendingMicroTaskRunnables.empty()

In CycleCollectedJSContext.cpp.

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
UnbindFromTree, I can do that.

Review commit: https://reviewboard.mozilla.org/r/242128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242128/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973750 [details]
Bug 1445392: Make HTMLSlotElement slot change event stuff not linear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242124/diff/1-2/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242126/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973752 [details]
Bug 1445392: Remove pending slots from the docgroup on unlink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242128/diff/1-2/

Comment 16

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

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?

Comment 17

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

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 {}

Comment 18

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

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.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

Moved to bug 1459698.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

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

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973750 [details]
Bug 1445392: Make HTMLSlotElement slot change event stuff not linear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242124/diff/2-3/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242126/diff/2-3/

Comment 23

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

Comment on attachment 8973751 [details]
Bug 1445392: Avoid posting a slotchange event microtask during shutdown.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/670ddc4fc00c
https://hg.mozilla.org/mozilla-central/rev/370b05966452