1596187 - JSWindowActorChild::willDestroy is not called if actor destruction is due to a tab being closed

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1596187 - Add a test that checks of JSWindowActorChild::willDestroy is called when closing tabs,

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Comment 3

[John Dai[:jdai]]

Yes, I'll take a look. Keep NI for tracking.

Comment 4

[Tomislav Jovanovic :zombie]

Bug 1594131 has more details about this, could possibly be a dupe (ignore the title).

Comment 5

[Chris Peterson [:cpeterson]]

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

Comment 6

[John Dai[:jdai]]

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 8

[John Dai[:jdai]]

(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 the willDestroy 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

[:Gijs (he/him)]

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) ?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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

[BugBot [:suhaib / :marco]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 12

[John Dai[:jdai]]

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

[Nika Layzell [:nika] (ni? for response)]

: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

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1596187 - Invoke JSWindowActor::WillDestroy even if PBrowser is shutting down,

Comment 15

[Pulsebot]

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

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1596187 - Part 3: Fix for renaming of 'windowGlobalChild' getter on a CLOSED TREE, a=bustage

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81ee4a9a0798
Part 3: Fix for renaming of 'windowGlobalChild' getter on a CLOSED TREE, a=bustage

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6aa3f3aac2d8
https://hg.mozilla.org/mozilla-central/rev/18d06ed7a151
https://hg.mozilla.org/mozilla-central/rev/81ee4a9a0798