1594131 - Make it a guarantee that a JSWindowActorParent's didDestroy methods will be called

Status: RESOLVED WORKSFORME

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

Description

[Neil Deakin]

I've found that willDestroy and didDestroy are often never called, making these functions mostly useless as one can't rely on them.

I note, for example of the parent side ( https://searchfox.org/mozilla-central/rev/8b7aa8af652f87d39349067a5bc9c0256bf6dedc/dom/ipc/WindowGlobalParent.cpp#230 ) that the caller of these functions checks if various states are set and doesn't call will/didDestroy in many cases, yet the actual call to will/didDestroy doesn't seem to depend on those states.

Either:

1. Fix them to be always be called.
2. Make the documentation clearer as to when these will not be called and suggest alternatives.

Comment 1

[Tomislav Jovanovic :zombie]

Sorry Neil for hijacking your bug, but I think we need (reasonable) strong guarantees here, and they should be achievable.

We probably already have code in m-c that depends on these methods always firing, and even if we document the current behavior, new code is likely to land with similar assumptions.

Andrew believes this should be less of an issue without Fission, but if we're replacing code which depended on MessageManager's "message-manager-close" event reliably being called, this is not just blocking Fission, but possibly introducing bugs in current Firefox (without Fission).

My understanding here might be limited, but I don't see a reason why we couldn't ensure, whatever the reason for the child process going away, that the didDestroy() method is called for each parent actor that was linked to that process.

Neha, can you please prioritize this bug as appropriate?

Comment 2

[Kris Maglione [:kmag]]

This is already guaranteed. The only thing that isn't guaranteed is that willDestroy will be called.

Comment 3

[Tomislav Jovanovic :zombie]

Well both Neil and Andrew (can you please confirm?) found them not reliably called, so something is up.

Additionally, I've found that sending messages during the willDestroy callback sometimes throws, despite the comment in webidl claiming it should work. If that is a also a possibility by design, we may as well also call willDestroy right before didDestroy if we haven't had a chance to call it while it could still send messages.

Otherwise, we need to go through existing code, as willDistroy seems more popular:
https://searchfox.org/mozilla-central/search?q=willDestroy(&path=*.js
https://searchfox.org/mozilla-central/search?q=didDestroy(&path=*.js

Comment 4

[Andrew McCreight [:mccr8]]

I don't remember if it was willDestroy or didDestroy that was causing issues.

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

[BugBot [:suhaib / :marco/ :calixte]]

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 7

[Tomislav Jovanovic :zombie]

Just found this while investigating:

// The 'didDestroy' callback is not always getting called.
// So we can't rely on it here. Instead, we will try to access
// the browsing context to judge wether the actor has
// been destroyed or not.

https://searchfox.org/mozilla-central/rev/492214c0/browser/actors/DOMFullscreenParent.jsm#152-155

Neal, can you please provide more information here?

Comment 8

[Neil Deakin]

My issue I think was just 1596187. I don't know what issue Abdoulaye was having.

Comment 9

[Chris Peterson [:cpeterson]]

Future because the original problem is gone.
kmag says we may change how this code works for Fission.

Comment 10

[Neha Kochar [:neha]]

didDestroy is already guaranteed to be called.
willDestroy is never guaranteed to be called. Do not use this for cleanup. Use didDestroy instead. willDestroy should be used for sending some last-minute messages, but not for cleanup.
We will fix the incorrect documentation in bug 1662771.