Closed Bug 1596187 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla74
Fission Milestone M5
Tracking Status
firefox74 --- fixed

People

(Reporter: mconley, Assigned: jdai)

References

Details

Attachments

(2 files, 1 obsolete file)

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.

Summary: JSWindowActorChild::willDestroy is not called if actor destruction is due to a tab closure → JSWindowActorChild::willDestroy is not called if actor destruction is due to a tab being closed

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
Flags: needinfo?(jdai)
Blocks: 1596169
Blocks: 1538979

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

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

See Also: → 1594131

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

Fission Milestone: --- → ?
Blocks: 1595154

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

Flags: needinfo?(jdai)

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

Flags: needinfo?(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

Flags: needinfo?(jdai)

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

Flags: needinfo?(mconley)
No longer blocks: 1595154

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.

Flags: needinfo?(mconley)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jmathies)
Assignee: nobody → jdai
Status: NEW → ASSIGNED
Fission Milestone: ? → M5
Priority: -- → P2
Flags: needinfo?(jmathies)

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

Flags: needinfo?(nika)

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

Flags: needinfo?(nika)
See Also: → 1609183
Attachment #9108500 - Attachment description: Bug 1596187 - Add a test that checks of JSWindowActorChild::willDestroy is called when closing tabs. → Bug 1596187 - Add a test that checks of JSWindowActorChild::willDestroy is called when closing tabs,
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
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
Attachment #9122392 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
See Also: → 1613069
See Also: → 1621726
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: