Bugzilla

`AppShutdownConfirmed` aka `"quit-application-granted"` is indeed a bit special. When it runs, we have basically a 100% running browser and are informed that we should begin our shutdown sequence. It is the point of no return.

[`mozilla::AppShutdown::OnShutdownConfirmed()`](https://searchfox.org/mozilla-central/rev/4044c34031d035fadb588143297ba5421419d44b/toolkit/components/startup/nsAppStartup.cpp#485) is called only once we had `ferocity == eForceQuit`. But as you pointed out, we already notified `"quit-application-granted"` observers well above, triggering all kinds of reactions.

We even have some special case inside [`AppShutdown::AdvanceShutdownPhaseInternal`](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/xpcom/base/AppShutdown.cpp#377-387) to avoid side effects from processing pending events before notifying the phase (again). I wonder if most of those side effects come exactly from the earlier notification.

I think we could try to just remove [the first notification](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#439-441) as it [happens below again](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#490-492) at the presumably better moment in time. This matches also better being "the point of no return" as we have some logic that [appears to check if we can really close](https://searchfox.org/mozilla-central/rev/4044c34031d035fadb588143297ba5421419d44b/toolkit/components/startup/nsAppStartup.cpp#472-474) but that seems to not have any other effect than returning an error **after** having initiated the shutdown, anyways, as we will always have set `ferocity = eForceQuit;` before if we get there.

Please note that the modified test of course would still show the behavior you discovered as you manually call `notifyObservers`. If you want the correct shutdown behavior, you need to use [`advanceShutdownPhase`](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/public/nsIAppStartup.idl#172-183). In fact I wonder if mocking `AppShutdown` in that test in general may have some unexpected side effects, too.

`AppShutdownConfirmed` aka `"quit-application-granted"` is indeed a bit special. When it runs, we have basically a 100% running browser and are informed that we should begin our shutdown sequence. It is the point of no return.

[`mozilla::AppShutdown::OnShutdownConfirmed()`](https://searchfox.org/mozilla-central/rev/4044c34031d035fadb588143297ba5421419d44b/toolkit/components/startup/nsAppStartup.cpp#485) is called only once we had `ferocity == eForceQuit`. But as you pointed out, we already notified `"quit-application-granted"` observers well above, triggering all kinds of reactions.

We even have some special case inside [`AppShutdown::AdvanceShutdownPhaseInternal`](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/xpcom/base/AppShutdown.cpp#377-387) to avoid side effects from processing pending events before notifying the phase (again). I wonder if most of those side effects come exactly from the earlier notification.

I think we could try to just remove [the first notification](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#439-441) as it [happens below again](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#490-492) at the presumably better moment in time. This matches also better being "the point of no return" as we have some logic that [appears to check if we can really close](https://searchfox.org/mozilla-central/rev/4044c34031d035fadb588143297ba5421419d44b/toolkit/components/startup/nsAppStartup.cpp#472-474) but that seems to not have any other effect than returning an error **after** having initiated the shutdown, anyways, as we will always have set `ferocity = eForceQuit;` before if we get there.

Please note that the modified test of course would still show the behavior you discovered as you manually call `notifyObservers`. If you want the correct shutdown behavior, you need to use [`advanceShutdownPhase`](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/public/nsIAppStartup.idl#172-183). In fact I wonder if mocking `AppShutdown` in that test in general may have some unexpected side effects, too.

Edit (after chatting with :smaug): It seems we should overhaul the order here a bit. We should definitely be in the shutdown phase before/while sending the notification. And call `CloseAllWindows` only afterwards. We can then still check if the closing was successful, but if not, that probably just merits a `MOZ_CRASH`, as [unload handlers](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#444-448) on child processes would not be able to create new processes anymore (once the `IsInOrBeyond`check works correctly) and having unload handlers for anything running in the parent process should be considered being an error, probably.

`AppShutdownConfirmed` aka `"quit-application-granted"` is indeed a bit special. When it runs, we have basically a 100% running browser and are informed that we should begin our shutdown sequence. It is the point of no return.

[`mozilla::AppShutdown::OnShutdownConfirmed()`](https://searchfox.org/mozilla-central/rev/4044c34031d035fadb588143297ba5421419d44b/toolkit/components/startup/nsAppStartup.cpp#485) is called only once we had `ferocity == eForceQuit`. But as you pointed out, we already notified `"quit-application-granted"` observers well above, triggering all kinds of reactions.

We even have some special case inside [`AppShutdown::AdvanceShutdownPhaseInternal`](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/xpcom/base/AppShutdown.cpp#377-387) to avoid side effects from processing pending events before notifying the phase (again). I wonder if most of those side effects come exactly from the earlier notification.

~~I think we could try to just remove [the first notification](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#439-441) as it [happens below again](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#490-492) at the presumably better moment in time. This matches also better being "the point of no return" as we have some logic that [appears to check if we can really close](https://searchfox.org/mozilla-central/rev/4044c34031d035fadb588143297ba5421419d44b/toolkit/components/startup/nsAppStartup.cpp#472-474) but that seems to not have any other effect than returning an error **after** having initiated the shutdown, anyways, as we will always have set `ferocity = eForceQuit;` before if we get there.~~

Edit: This is nonsense, obviously above we fire "quit-application-granted" and then `AppShutdown` fires "quit-application".

Please note that the modified test of course would still show the behavior you discovered as you manually call `notifyObservers`. If you want the correct shutdown behavior, you need to use [`advanceShutdownPhase`](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/public/nsIAppStartup.idl#172-183). In fact I wonder if mocking `AppShutdown` in that test in general may have some unexpected side effects, too.

Edit (after chatting with :smaug): It seems we should overhaul the order here a bit. We should definitely be in the shutdown phase before/while sending the notification. And call `CloseAllWindows` only afterwards. We can then still check if the closing was successful, but if not, that probably just merits a `MOZ_CRASH`, as [unload handlers](https://searchfox.org/mozilla-central/rev/503938c13ef2dd174705dc0f6d0683ae43074ccc/toolkit/components/startup/nsAppStartup.cpp#444-448) on child processes would not be able to create new processes anymore (once the `IsInOrBeyond`check works correctly) and having unload handlers for anything running in the parent process should be considered being an error, probably.