1762840 - Let AsyncShutdown.getPhase use IDLShutdownPhase instead of topics and check if we are not beyond the requested phase

Status: NEW

(Toolkit :: Async Tooling, task, P3)

Description

[Jens Stutte [:jstutte]]

Currently getPhase is mapped to shutdown phases via observer topics. We now have ShutdownPhase definitions for all relevant phases. Furthermore we have now mapping functions in AppShutdown we might want to expose to JS as well.

In addition we should verify if we are already isInOrBeyondShutdownPhase for a given phase before:

• creating it
• adding new shutdown blockers to it

It is expected that this will create some fallout, but this fallout will be real bugs in our shutdown handling.

Comment 1

[:Gijs (he/him)]

Hey Jens, are there folks intending to work on this? This is showing up in my triage queue and I'm unsure how to prioritize it...

Comment 2

[Jens Stutte [:jstutte]]

Well, I was thinking to start to look into it soonish, but P3 is probably ok for now. How much work this is depends pretty much on the fallout it generates, but to arrive at the point to see it should not be such a big thing, hopefully.

But longterm it is IMHO a quite important design improvement of the AsyncShutdown that should be tackled one day - mostly for the additional checks.

Comment 3

[Jens Stutte [:jstutte]]

(actually seeing much fallout might raise the priority as it would mean there are real problems elsewhere)

Comment 4

[Jens Stutte [:jstutte]]

I started to look into this and noticed that AsyncShutdown currently defines standard phases for the following notifications:

"profile-change-teardown"
"profile-before-change"
"profile-before-change-telemetry"
"quit-application-granted"
"web-workers-shutdown"
"xpcom-will-shutdown"

which stands against this mapping in AppShutdown:

    "quit-application",                 // AppShutdownConfirmed
    "profile-change-net-teardown",      // AppShutdownNetTeardown
    "profile-change-teardown",          // AppShutdownTeardown
    "profile-before-change",            // AppShutdown
    "profile-before-change-qm",         // AppShutdownQM
    "profile-before-change-telemetry",  // AppShutdownTelemetry
    "xpcom-will-shutdown",              // XPCOMWillShutdown
    "xpcom-shutdown",                   // XPCOMShutdown
    "xpcom-shutdown-threads",           // XPCOMShutdownThreads

such that "quit-application-granted" and "web-workers-shutdown" are not mapped to any shutdown phase. Furthermore the design currently allows to register blockers to any notification topic, which seemed to be a design choice looking at this comment:

// Ideally, phases should be registered from the component that decides
// when they start/stop. For compatibility with existing startup/shutdown
// mechanisms, we register a few phases here.

but in practice seemed not to find many uses except for AsyncShutdown tests.

Now "web-workers-shutdown" seems to be triggered by "xpcom-shutdown" but wants probably to be completed before finishing the shutdown. I think this could be handled by making those blockers block "xpcom-will-shutdown" and then doing the RuntimeService shutdown as part of "xpcom-shutdown" as before.

Remains quitApplicationGranted where we would need to understand, if those blockers can just move over to "quit-application" or if we need to add this to the AppShutdown phases. Honestly I am not sure if there is any conceptual difference between the two, but I am quite sure that unifying them might create some surprises.

Comment 5

[Jens Stutte [:jstutte]]

Attachment: WIP: Bug 1762840: Add getShutdownPhaseObserverTopic to AppStartup.

Comment 6

[Jens Stutte [:jstutte]]

Attachment: WIP: Bug 1762840: Use shutdown phases instead of observer topics in AsyncShutdown where possible.

Posting as WIP for now, this is just a sketch that helped me to arrive at the conclusions and open questions from comment 4. It might well be the wrong approach here.

Depends on D143342

Comment 7

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #4)

Remains quitApplicationGranted where we would need to understand, if those blockers can just move over to "quit-application" or if we need to add this to the AppShutdown phases. Honestly I am not sure if there is any conceptual difference between the two, but I am quite sure that unifying them might create some surprises.

Actually quitApplicationGranted is the first sign of shutdown and we can receive this at any time (for example on top of nested event loops on the main thread and such, see SpinEventLoopUntilOrQuit). So we need it and should keep it, but we should probably not make it being blocked by too much shutdown processing and use "quit-application"/ AppShutdownConfirmed instead.