Closed Bug 1413418 Opened 7 years ago Closed 6 years ago

Shutdown leak on CustomElements with XUL element support

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox63 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1413418 - Test case introducing lifecycle callbacks on <deck> to trigger leaks
59 bytes, text/x-review-board-request
Details
Bug 1413418 - Give the last chance to consume micro task queue during the final cycle collection;
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review 
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
Priority: -- → P2
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)
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.
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.
Attachment #8925044 - Attachment is obsolete: true
Attachment #8925045 - Attachment is obsolete: true
Blocks: 1421070
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)
(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.
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)
(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)
(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+
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
https://hg.mozilla.org/mozilla-central/rev/c736dc02532d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: