Shutdown leak on CustomElements with XUL element support

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox63 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
We hit shutdown leak [1] when we add CustomElements support for XUL element (bug 1404420) and migrate <stringbundle> from a XBL binding to a Custom Element (bug 1411707).

It is because Element.UnbindFromTree is invoked during shutdown, like
> * frame #0: 0x000000011225a543 XUL`mozilla::dom::CustomElementReactionsStack::Enqueue(this=0x000000013a73f100, aElement=0x000000013b5f3f70, aReaction=UniquePtr<mozilla::dom::CustomElementReaction, mozilla::DefaultDelete<mozilla::dom::CustomElementReaction> > @ 0x00007fff55ad0bc0) at CustomElementRegistry.cpp:1098
>   frame #1: 0x0000000112256bec XUL`mozilla::dom::CustomElementReactionsStack::EnqueueCallbackReaction(this=0x000000013a73f100, aElement=0x000000013b5f3f70, aCustomElementCallback=<unavailable>) at CustomElementRegistry.cpp:1068
>   frame #2: 0x0000000112256b0c XUL`mozilla::dom::CustomElementRegistry::EnqueueLifecycleCallback(aType=eDisconnected, aCustomElement=0x000000013b5f3f70, aArgs=0x0000000000000000, aAdoptedCallbackArgs=0x0000000000000000, aDefinition=0x0000000000000000) at CustomElementRegistry.cpp:486
>   frame #3: 0x000000011215bec6 XUL`nsContentUtils::EnqueueLifecycleCallback(aType=eDisconnected, aCustomElement=0x000000013b5f3f70, aArgs=0x0000000000000000, aAdoptedCallbackArgs=0x0000000000000000, aDefinition=0x0000000000000000) at nsContentUtils.cpp:10243
>   frame #4: 0x00000001122975bd XUL`mozilla::dom::Element::UnbindFromTree(this=0x000000013b5f3f70, aDeep=true, aNullParent=false) at Element.cpp:1987
>   frame #5: 0x00000001149c5352 XUL`nsXULElement::UnbindFromTree(this=0x000000013b5f3f70, aDeep=true, aNullParent=false) at nsXULElement.cpp:972
>   frame #6: 0x0000000112297663 XUL`mozilla::dom::Element::UnbindFromTree(this=0x000000013ac99430, aDeep=true, aNullParent=true) at Element.cpp:2009
>   frame #7: 0x00000001149c5352 XUL`nsXULElement::UnbindFromTree(this=0x000000013ac99430, aDeep=true, aNullParent=true) at nsXULElement.cpp:972
>   frame #8: 0x000000011228b352 XUL`mozilla::dom::FragmentOrElement::cycleCollection::Unlink(this=0x000000011ef7ee98, p=0x000000013ac99550) at FragmentOrElement.cpp:1518
>   frame #9: 0x00000001149c25fd XUL`nsXULElement::cycleCollection::Unlink(this=0x000000011ef7ee98, p=0x000000013ac99550) at nsXULElement.cpp:412
>   frame #10: 0x000000010fbd9036 XUL`nsCycleCollector::CollectWhite(this=0x000000010a4696e0) at nsCycleCollector.cpp:3401
>   frame #11: 0x000000010fbdaa7f XUL`nsCycleCollector::Collect(this=0x000000010a4696e0, aCCType=ShutdownCC, aBudget=0x00007fff55ad12a8, aManualListener=0x0000000000000000, aPreferShorterSlices=false) at nsCycleCollector.cpp:3769
>   frame #12: 0x000000010fbda781 XUL`nsCycleCollector::ShutdownCollect(this=0x000000010a4696e0) at nsCycleCollector.cpp:3687
>   frame #13: 0x000000010fbdc1be XUL`nsCycleCollector::Shutdown(this=0x000000010a4696e0, aDoCollect=true) at nsCycleCollector.cpp:3990
>   frame #14: 0x000000010fbddf2c XUL`nsCycleCollector_shutdown(aDoCollect=true) at nsCycleCollector.cpp:4373
>   frame #15: 0x000000010fd5ff85 XUL`mozilla::ShutdownXPCOM(aServMgr=0x0000000000000000) at XPCOMInit.cpp:985
>   frame #16: 0x000000010fd5f985 XUL`::NS_ShutdownXPCOM(aServMgr=0x000000010a42a3d0) at XPCOMInit.cpp:822
>   frame #17: 0x000000011830160b XUL`ScopedXPCOMStartup::~ScopedXPCOMStartup(this=0x000000010a4114a0) at nsAppRunner.cpp:1513
>   frame #18: 0x0000000118301655 XUL`ScopedXPCOMStartup::~ScopedXPCOMStartup(this=0x000000010a4114a0) at nsAppRunner.cpp:1494
>   frame #19: 0x0000000118310f4b XUL`mozilla::DefaultDelete<ScopedXPCOMStartup>::operator(this=0x00007fff55ad18e0, aPtr=0x000000010a4114a0)(ScopedXPCOMStartup*) const at UniquePtr.h:528
>   frame #20: 0x0000000118310ecf XUL`mozilla::UniquePtr<ScopedXPCOMStartup, mozilla::DefaultDelete<ScopedXPCOMStartup> >::reset(this=0x00007fff55ad18e0, aPtr=0x0000000000000000) at UniquePtr.h:343
>   frame #21: 0x000000011830d297 XUL`mozilla::UniquePtr<ScopedXPCOMStartup, mozilla::DefaultDelete<ScopedXPCOMStartup> >::operator=(this=0x00007fff55ad18e0, (null)=0x0000000000000000) at UniquePtr.h:313
>   frame #22: 0x000000011830ce1d XUL`XREMain::XRE_main(this=0x00007fff55ad18b8, argc=5, argv=0x00007fff55ad1f30, aConfig=0x00007fff55ad1a78) at nsAppRunner.cpp:4876
>   frame #23: 0x000000011830d52c XUL`XRE_main(argc=5, argv=0x00007fff55ad1f30, aConfig=0x00007fff55ad1a78) at nsAppRunner.cpp:4943

If the element (XUL element) is an custom element, we will enqueue a disconnectedCallback to backup queue and dispatch a MicroTask to process the reactions. But since it is dispatched during shutdown, it doesn't have chance to be executed, then we leak.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=139641294&repo=try&lineNumber=3183
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Comment 1

2 years ago
It looks like a generic issue on micro task to me, though I am not sure if there are cases (other than CustomElements) will dispatch micro task during shutdown ...

There is a similar issue on stable state (bug 1200514) and fixed by giving the last chance to consume stable state queue. Does it make sense if we do the same thing on micro task queue?
Flags: needinfo?(bugs)
CustomElements don't use microtasks currently, but the weird thingie added for Promises which happens to be called microtasks.
I wonder if bug 1193394 helps help here.
Flags: needinfo?(bugs)
(Assignee)

Comment 3

2 years ago
But even I switch CustomElements using DispatchMicroTaskRunnable (without applying bug 1193394), I still hit shutdown leak. I will try bug 1193394 to see if it helps and get back to you. Thank you.
(Assignee)

Comment 4

2 years ago
bug 1193394 doesn't help, we still have shutdown leak on dispatching MicroTaskRunnable during shutdown case, try log for your reference, https://treeherder.mozilla.org/logviewer.html#?job_id=141561776&repo=try&lineNumber=3026. :(
ok, I guess we could then try some similar hack as in bug 1200514.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8925044 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8925045 - Attachment is obsolete: true
So after the investigation, is this leak XUL-specific? I'm also seeing a leak from `disconnectedCallback` in an HTML document loaded over resource://…
Flags: needinfo?(echen)
(Assignee)

Comment 9

a year ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #8)
> So after the investigation, is this leak XUL-specific? I'm also seeing a
> leak from `disconnectedCallback` in an HTML document loaded over resource://…

This leak could also happen in HTML document, but I haven't seen the case. Do you have STR?
Flags: needinfo?(echen) → needinfo?(MattN+bmo)
Unfortunately when I tried to make a simplified test case I wasn't successful. If you enable custom elements in the payment request tests (so that it doesn't use the polyfill) at https://dxr.mozilla.org/mozilla-central/rev/474d58c9137360c0fa1c85cdd11e3313b33b7cad/toolkit/components/payments/test/mochitest/mochitest.ini#3 then you will see a leak but I'm not 100% sure if that's from custom elements or something else.
Flags: needinfo?(MattN+bmo)
See Also: → 1460334
Pushed up a patch that triggers the issue by introducing lifecycle callbacks on the <deck> Custom Element: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09d16d4c80ddf8b923222587ba5dfdf8e023f15d
I'm seeing a pretty frequent leak on Win64 debug c3 jobs with the patches that convert findbar to a CE (bug 
1411707): https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf61980716902412adb1b91d98a6443dd6dab54&selectedJob=179537827.

Not sure if it's the same issue we see here - the patch from Comment 13 is a simpler test case.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Pushed up a patch that triggers the issue by introducing lifecycle callbacks
> on the <deck> Custom Element:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09d16d4c80ddf8b923222587ba5dfdf8e023f15d

Just to pick a random test that reproduces the leak with this patch applied: `./mach mochitest toolkit/content/tests/chrome/test_bug263683.xul` does the trick.
Comment hidden (mozreview-request)
I've pushed up an updated test case with additional logging and a simplified mochitest to trigger the leak. I noticed that there is a Custom Element that has its constructor/connectedCallback fire even _after_ the window unload event has happened. I'm not sure if that's directly what is causing the leak, but I wasn't expecting that should ever happen. Is that supposed to be possibe?

FWIW it looks like the CE instance is inside this XBL anon content: https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/browser/base/content/urlbarBindings.xml#1786. Logging out `this.parentNode.outerHTML` shows: "<xul:vbox xmlns:xul=\"http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul\" anonid=\"one-off-search-buttons\" class=\"search-one-offs\" compact=\"true\" includecurrentengine=\"true\" disabletab=\"true\" flex=\"1\" context=\"_child\"/>"
Flags: needinfo?(bugs)
Why unload event had anything to do with connectCallback?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #18)
> Why unload event had anything to do with connectCallback?

I was just surprised that an element was being constructed/connected after the window had unloaded. Spent some more time looking at this and I think what's happening is:

- window unloads
- document-element-inserted fires which triggers the call to `customElements.define`, which triggers an upgrade of a <deck> inside the unloaded window

It seems like document-element-inserted shouldn't fire if the window has already unloaded.
(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Olli Pettay [:smaug] from comment #18)
> > Why unload event had anything to do with connectCallback?
> 
> I was just surprised that an element was being constructed/connected after
> the window had unloaded. Spent some more time looking at this and I think
> what's happening is:
> 
> - window unloads
> - document-element-inserted fires which triggers the call to
> `customElements.define`, which triggers an upgrade of a <deck> inside the
> unloaded window
> 
> It seems like document-element-inserted shouldn't fire if the window has
> already unloaded.

Actually, I think I'm wrong about the order here. I thought it was happening this way because console.trace() inside the connectedCallback pointed back to the customElements.define, but it doesn't actually seem to be in response to the script loading. I need to look more closely, but I think the order is:

- document-element-inserted fires which triggers the call to `customElements.define`
- window unloads
- new <deck> gets constructed / connected
I guess I'm wondering: is there an expected/valid use-case in the spec for constructing and connecting Custom Elements inside a window after it's been unloaded?
Flags: needinfo?(bugs)
Seeing this with the tabbox Custom Element migration as well (Bug 1469902)
Blocks: 1469902
Edgar, is there any chance you can pick this one up? We are starting to see leaks in various XBL binding conversion patches, some of which are almost ready to land otherwise. This is also potentially affecting Web Payments when running without the CE polyfill (Comment 11).
Flags: needinfo?(echen)
(Assignee)

Comment 24

10 months ago
(In reply to Brian Grinstead [:bgrins] from comment #23)
> Edgar, is there any chance you can pick this one up? We are starting to see
> leaks in various XBL binding conversion patches, some of which are almost
> ready to land otherwise. This is also potentially affecting Web Payments
> when running without the CE polyfill (Comment 11).

Yes, I will. (Intentionally leaving myself needinfoed)
(Assignee)

Comment 26

9 months ago
(In reply to Edgar Chen [:edgar] from comment #25)
> Created attachment 8991907 [details]
> Bug 1413418 - Give the last chance to consume micro task queue during the
> final cycle collection; r?smaug

The leak is from the reaction enqueued in UnbindFromTree during shutdown (comment #0) which have no chance to be executed. This patch gives the last chance to consume micro task queue and I don't see leaks on payment tests (with custom elements enabled) [1] and the <deck> test (attachment #8975976 [details]) anymore.
Flags: needinfo?(echen)
Flags: needinfo?(bugs)
Attachment #8991907 - Attachment description: Bug 1413418 - Give the last chance to consume micro task queue during the final cycle collection; r?smaug → Bug 1413418 - Give the last chance to consume micro task queue during the final cycle collection;
FYI: Looks like this patch is hitting an assertion on Try and in my local test:
> Assertion failure: aForce ? currentDepth == 0 : currentDepth > 0, at /mozilla-central2/xpcom/base/CycleCollectedJSContext.cpp:535
(In reply to Edgar Chen [:edgar] from comment #29)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f247856402500629836c36919f58dd43bb20068f

This fixes the leak I was seeing for the PaymentRequest dialog which uses custom elements from XHTML. Thanks! 🎉
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #30)
> (In reply to Edgar Chen [:edgar] from comment #29)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f247856402500629836c36919f58dd43bb20068f
> 
> This fixes the leak I was seeing for the PaymentRequest dialog which uses
> custom elements from XHTML. Thanks! 🎉

This also appears to fix the leaks in the <tabbox> conversion patch in Bug 1469902.
Comment on attachment 8991907 [details]
Bug 1413418 - Give the last chance to consume micro task queue during the final cycle collection;

Olli Pettay [:smaug] (vacation Jul 15->) has approved the revision.

https://phabricator.services.mozilla.com/D2121
Attachment #8991907 - Flags: review+

Comment 33

9 months ago
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c736dc02532d
Give the last chance to consume micro task queue during the final cycle collection; r=smaug

Comment 34

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c736dc02532d
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.