JSWindowActorChild::willDestroy is not called if actor destruction is due to a tab being closed
Categories
(Core :: DOM: Content Processes, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: mconley, Assigned: jdai)
References
Details
Attachments
(2 files, 1 obsolete file)
Bug 1596187 - Add a test that checks of JSWindowActorChild::willDestroy is called when closing tabs,
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
I noticed this while trying to address bug 1596169 - the willDestroy
callback, which we often use to clean up our JSWindowActorChild's, doesn't get called consistently.
It gets called if the WindowGlobalChild goes away due to a navigation. It doesn't appear to be called if the WindowGlobalChild goes away due to the tab being closed.
DidDestroy is, however, called properly.
I'll attach a test case.
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 1•4 years ago
|
||
Reporter | ||
Comment 2•4 years ago
|
||
Hey jdai, do you think you have time to look at this? This is pretty unexpected, and might mean that we're leaking or not cleaning up properly in some cases.
Here are the non-TestActor places where we currently use willDestroy:
browser/actors/NetErrorParent.jsm
browser/actors/PluginChild.jsm
toolkit/actors/AutoCompleteChild.jsm
toolkit/actors/AutoCompleteParent.jsm
toolkit/actors/PictureInPictureChild.jsm
toolkit/actors/UAWidgetsChild.jsm
Assignee | ||
Comment 3•4 years ago
|
||
Yes, I'll take a look. Keep NI for tracking.
Comment 4•4 years ago
|
||
Bug 1594131 has more details about this, could possibly be a dupe (ignore the title).
Comment 5•4 years ago
|
||
Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Assignee | ||
Comment 6•4 years ago
|
||
Per bug 1538979 comment #0, it's expected behavior when closing a tab for instance willDestroy
is not called but didDestroy
is.
Child Side
The willDestroy method should be called immediately before SendDestroy in WindowGlobalChild: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/dom/ipc/WindowGlobalChild.cpp#109
We don't want to call this method if our actor is already closed (e.g. due to TabChild being destroyed), as we won't be able to send messages anymore, so only call it inside of the mentioned conditional :-).
The didDestroy method should be called at the end of ActorDestroy on WindowGlobalChild: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/dom/ipc/WindowGlobalChild.cpp#190
Reporter | ||
Comment 7•4 years ago
|
||
(In reply to John Dai[:jdai] from comment #6)
Hm. So what you're saying is that if the tab is closed, a JSWindowActorChild has no opportunity to send any messages to the parent via the willDestroy
method?
Assignee | ||
Comment 8•4 years ago
•
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos) from comment #7)
(In reply to John Dai[:jdai] from comment #6)
Hm. So what you're saying is that if the tab is closed, a JSWindowActorChild has no opportunity to send any messages to the parent via thewillDestroy
method?
Right, willDestroy
rely on BrowserChild still exist or IsDestroyed flag[1] not set. However, BrowserChild is destroyed[2] before willDestroy
is called. I don't have a better solution right now.
[1] https://searchfox.org/mozilla-central/rev/3483fb259b4edbe4594cfcc3911db97d5441b67d/dom/ipc/WindowGlobalChild.cpp#223
[2] https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/dom/ipc/BrowserChild.cpp#2446
Comment 9•4 years ago
|
||
So what's the way forward here? It's pretty counterintuitive that willDestroy
doesn't get called for closed tabs. I'd sooner generate an eslint error and forbid the method entirely than have it as a footgun. Looking at e.g. the PluginChild implementation I guess we should be running this code in didDestroy
instead?
Is willDestroy
still guaranteed to fire in the parent, even for tab close?
Alternatively, can't we still fire the method but just throw an error from sendAsyncMessage
(and then have an eslint rule that errors for calls to this.sendAsyncMessage
outside of a try...catch
inside willDestroy
implementations) ?
Reporter | ||
Comment 10•4 years ago
|
||
I've been discussing this with jdai, and he's planning on adding a new cleanup
method on JSWindowActor that will be called every time the actor is going to be destroyed, but before the DOM starts going away.
At that point, we can start migrating the things that we need to to it. And if it turns out that we don't need willDestroy
or didDestroy
, we can then deprecate / remove those.
Comment 11•4 years ago
|
||
The priority flag is not set for this bug.
:jimm, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
JSWindowActor lifecycle is managed by WindowGlobalActor. When docshell is being destroyed, it will call nsGlobalWindowInner::FreeInnerObjects
[1] then call WindowGlobalChild::Destroy
[2], then call JSWindowActor’s willDestroy. However, when the browser is being closed, we destroy remote browser at first[3] then destroy docshell[4]. It makes WindowGlobalChild
actor doesn't have a chance to send willDestroy
messages due to BrowserChild’s IsDestroyed flag[5] has been set.
I try to call willDestroy
callback while we’re firing unload
event, however, it makes premature unable to send message for JSWindowActor and it causes other issues. NI Nika for suggestion who is the author for WindowGlobalActor.
[1] https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/base/nsGlobalWindowOuter.cpp#2454
[2] https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/ipc/WindowGlobalChild.cpp#232
[3] https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/base/nsFrameLoader.cpp#1889
[4] https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/base/nsFrameLoader.cpp#1904
[5] https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/dom/ipc/BrowserChild.cpp#2446
Comment 13•4 years ago
|
||
:mconley suggested that it may be possible to move existing users to didDestroy
, as they don't need to send IPC messages. If this is the case we may be able to avoid large changes here, otherwise I have some ideas for how we can get this to be slightly more reliable.
Comment 14•4 years ago
|
||
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6aa3f3aac2d8 Invoke JSWindowActor::WillDestroy even if PBrowser is shutting down, r=jdai https://hg.mozilla.org/integration/autoland/rev/18d06ed7a151 Add a test that checks of JSWindowActorChild::willDestroy is called when closing tabs, r=mconley
Comment 16•4 years ago
|
||
Comment 17•4 years ago
|
||
Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/81ee4a9a0798 Part 3: Fix for renaming of 'windowGlobalChild' getter on a CLOSED TREE, a=bustage
Updated•4 years ago
|
Comment 18•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6aa3f3aac2d8
https://hg.mozilla.org/mozilla-central/rev/18d06ed7a151
https://hg.mozilla.org/mozilla-central/rev/81ee4a9a0798
Description
•