Bug 1726813: Ensure AppShutdown remains in sync with shutdown notifications in the parent process. r=xpcom-reviewers,nika,dom-worker-reviewers,asuth

The goal here is to ensure we can always rely on `AppShutdown::GetShutdownPhase` to be in sync with the "real" application status, mainly this was needed for xpcshell tests to not break if we add assertions on our shutdown state on some global singletons.

We keep the existing observer notification topics but force them (on the parent process) to be issued through the new `advanceShutdownPhase` function of the startup service using the `ShutdownPhase` enum. This way we can synchronize `AppShutdown`'s internal status accordingly.

Some further notes:

  # The `MOZ_ASSERT(AppShutdown::IsNoOrLegalShutdownTopic(aTopic));` in `NotifyObservers` helped a lot to identify missing cases. I think we should keep it in order to stay safe.
  # Introducing the `cenum IDLShutdownPhase` helps to keep the knowledge about the mapping from shutdown phases to observer topics exclusively inside AppShutdown.cpp. Still callers must know what they do in order to choose a proper phase, of course.
  # However we must be aware that `AppShutdown` this way can be kept in sync with the shutdown notifications only in the parent process and that `GetCurrentShutdownPhase` might not give the correct result in child processes. We might want to file a follow up bug that adds some asserts to avoid improper use of `AppShutdown` functions in child processes (but I do not want to make this patch bigger as needed to solve the blocking dependency for bug 1697972).
  # The socket process is one example of a child process that "overloads" shutdown topics. I was wondering if it is the right call to use the very same topic names here to request shutdown to the socket process or if it should have its own topics. Those topics triggered the assert and thus I had to disable it for child processes, for now.
  # This goes together with the more general approach to define process type specific shutdown phases (and hence mappings to topics) as drafted very roughly in bug 1697745.
  # This patch seemed to trigger a known intermittent more often, thus the change here in `ServiceWorkerManager`.

Differential Revision: https://phabricator.services.mozilla.com/D124350

dom/serviceworkers/ServiceWorkerManager.cpp

@@ -24,6 +24,7 @@
 
 #include "jsapi.h"
 
+#include "mozilla/AppShutdown.h"
 #include "mozilla/BasePrincipal.h"
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/ErrorNames.h"
@@ -1374,23 +1375,24 @@ ServiceWorkerManager::GetOrCreateJobQueue(const nsACString& aKey,
 
 /* static */
 already_AddRefed<ServiceWorkerManager> ServiceWorkerManager::GetInstance() {
-  // Note: We don't simply check gInstance for null-ness here, since otherwise
+  if (!gInstance) {
-  // this can resurrect the ServiceWorkerManager pretty late during shutdown.
-  static bool firstTime = true;
-  if (firstTime) {
     RefPtr<ServiceWorkerRegistrar> swr;
 
-    // Don't create the ServiceWorkerManager until the ServiceWorkerRegistrar is
+    // XXX: Substitute this with an assertion. See comment in Init.
-    // initialized.
     if (XRE_IsParentProcess()) {
+      // Don't (re-)create the ServiceWorkerManager if we are already shutting
+      // down.
+      if (AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed)) {
+        return nullptr;
+      }
+      // Don't create the ServiceWorkerManager until the ServiceWorkerRegistrar
+      // is initialized.
       swr = ServiceWorkerRegistrar::Get();
       if (!swr) {
         return nullptr;
       }
     }
 
-    firstTime = false;
-
     MOZ_ASSERT(NS_IsMainThread());
 
     gInstance = new ServiceWorkerManager();

netwerk/test/unit/test_cookies_upgrade_10.js

@@ -46,7 +46,9 @@ add_task(async function() {
 
     // Pretend that we're about to shut down, to tell the cookie manager
     // to clean up its connection with its database.
-    Services.obs.notifyObservers(null, "profile-before-change");
+    Services.startup.advanceShutdownPhase(
+      Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+    );
 
     // check for upgraded schema.
     Assert.equal(12, getDBVersion(destFile));

security/manager/ssl/tests/unit/test_sss_sanitizeOnShutdown.js

@@ -64,8 +64,12 @@ add_task(async function run_test() {
   Services.prefs.setBoolPref(Sanitizer.PREF_SANITIZE_ON_SHUTDOWN, true);
 
   // Simulate shutdown.
-  Services.obs.notifyObservers(null, "profile-change-teardown");
+  Services.startup.advanceShutdownPhase(
-  Services.obs.notifyObservers(null, "profile-before-change");
+    Services.startup.SHUTDOWN_PHASE_APPSHUTDOWNTEARDOWN
+  );
+  Services.startup.advanceShutdownPhase(
+    Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+  );
 
   equal(getStateFileContents(), "", "state file should be empty");
 });

testing/xpcshell/head.js

@@ -695,10 +695,18 @@ function _execute_test() {
   if (_profileInitialized) {
     // Since we have a profile, we will notify profile shutdown topics at
     // the end of the current test, to ensure correct cleanup on shutdown.
-    _Services.obs.notifyObservers(null, "profile-change-net-teardown");
+    _Services.startup.advanceShutdownPhase(
-    _Services.obs.notifyObservers(null, "profile-change-teardown");
+      _Services.startup.SHUTDOWN_PHASE_APPSHUTDOWNNETTEARDOWN
-    _Services.obs.notifyObservers(null, "profile-before-change");
+    );
-    _Services.obs.notifyObservers(null, "profile-before-change-qm");
+    _Services.startup.advanceShutdownPhase(
+      _Services.startup.SHUTDOWN_PHASE_APPSHUTDOWNTEARDOWN
+    );
+    _Services.startup.advanceShutdownPhase(
+      _Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+    );
+    _Services.startup.advanceShutdownPhase(
+      _Services.startup.SHUTDOWN_PHASE_APPSHUTDOWNQM
+    );
 
     _profileInitialized = false;
   }

toolkit/components/startup/nsAppStartup.cpp

@@ -503,6 +503,53 @@ nsAppStartup::Quit(uint32_t aMode, int aExitCode, bool* aUserAllowedQuit) {
   return rv;
 }
 
+// Ensure ShutdownPhase.h and nsIAppStartup.idl are in sync.
+static_assert(int(nsIAppStartup::SHUTDOWN_PHASE_NOTINSHUTDOWN) ==
+                      int(mozilla::ShutdownPhase::NotInShutdown) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_APPSHUTDOWNCONFIRMED) ==
+                      int(mozilla::ShutdownPhase::AppShutdownConfirmed) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_APPSHUTDOWNNETTEARDOWN) ==
+                      int(mozilla::ShutdownPhase::AppShutdownNetTeardown) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_APPSHUTDOWNTEARDOWN) ==
+                      int(mozilla::ShutdownPhase::AppShutdownTeardown) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_APPSHUTDOWN) ==
+                      int(mozilla::ShutdownPhase::AppShutdown) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_APPSHUTDOWNQM) ==
+                      int(mozilla::ShutdownPhase::AppShutdownQM) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_APPSHUTDOWNRELEMETRY) ==
+                      int(mozilla::ShutdownPhase::AppShutdownTelemetry) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_XPCOMWILLSHUTDOWN) ==
+                      int(mozilla::ShutdownPhase::XPCOMWillShutdown) &&
+                  int(nsIAppStartup::SHUTDOWN_PHASE_XPCOMSHUTDOWN) ==
+                      int(mozilla::ShutdownPhase::XPCOMShutdown),
+              "IDLShutdownPhase values are as expected");
+
+// Helper for safe conversion to native shutdown phases.
+Result<ShutdownPhase, nsresult> IDLShutdownPhaseToNative(
+    nsAppStartup::IDLShutdownPhase aPhase) {
+  if (uint8_t(aPhase) <= nsIAppStartup::SHUTDOWN_PHASE_XPCOMSHUTDOWN) {
+    return ShutdownPhase(aPhase);
+  }
+  return Err(NS_ERROR_ILLEGAL_VALUE);
+}
+
+NS_IMETHODIMP
+nsAppStartup::AdvanceShutdownPhase(IDLShutdownPhase aPhase) {
+  ShutdownPhase nativePhase;
+  MOZ_TRY_VAR(nativePhase, IDLShutdownPhaseToNative(aPhase));
+  AppShutdown::AdvanceShutdownPhase(nativePhase);
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+nsAppStartup::IsInOrBeyondShutdownPhase(IDLShutdownPhase aPhase,
+                                        bool* aIsInOrBeyond) {
+  ShutdownPhase nativePhase;
+  MOZ_TRY_VAR(nativePhase, IDLShutdownPhaseToNative(aPhase));
+  *aIsInOrBeyond = AppShutdown::IsInOrBeyond(nativePhase);
+  return NS_OK;
+}
+
 void nsAppStartup::CloseAllWindows() {
   nsCOMPtr<nsIWindowMediator> mediator(
       do_GetService(NS_WINDOWMEDIATOR_CONTRACTID));

toolkit/components/startup/public/nsIAppStartup.idl

@@ -151,6 +151,47 @@ interface nsIAppStartup : nsISupports
      */
     bool quit(in uint32_t aMode, [optional] in int32_t aExitCode);
 
+    /**
+    * These values must match the xpcom/base/ShutdownPhase.h values.
+    * We do not expose late XPCOM shutdown phases here, everything
+    * after SHUTDOWN_PHASE_XPCOMSHUTDOWN is expected to be irrelevant 
+    * for JS.
+    */
+    cenum IDLShutdownPhase : 8 {
+        SHUTDOWN_PHASE_NOTINSHUTDOWN = 0,
+        SHUTDOWN_PHASE_APPSHUTDOWNCONFIRMED,
+        SHUTDOWN_PHASE_APPSHUTDOWNNETTEARDOWN,
+        SHUTDOWN_PHASE_APPSHUTDOWNTEARDOWN,
+        SHUTDOWN_PHASE_APPSHUTDOWN,
+        SHUTDOWN_PHASE_APPSHUTDOWNQM,
+        SHUTDOWN_PHASE_APPSHUTDOWNRELEMETRY,
+        SHUTDOWN_PHASE_XPCOMWILLSHUTDOWN,
+        SHUTDOWN_PHASE_XPCOMSHUTDOWN
+    };
+
+    /**
+     * Wrapper for shutdown notifications that informs the terminator before
+     * we notify other observers. Calls MaybeFastShutdown.
+     * This function is supposed to be used only from some (xpcshell) tests
+     * explicitely dealing with shutdown.
+     * 
+     * @param aPhase
+     *        The shutdown phase we want to advance to. Please note, that
+     *        we cannot go back to earlier phases or abort shutdown once
+     *        it started.
+     */
+    void advanceShutdownPhase(in nsIAppStartup_IDLShutdownPhase aPhase);
+
+    /**
+     * Check if we entered or passed a specific shutdown phase.
+     * 
+     * @param aPhase
+     *        The shutdown phase we want to check. 
+     *
+     * @return true if we are in or beyond the given phase.
+     */
+    bool isInOrBeyondShutdownPhase(in nsIAppStartup_IDLShutdownPhase aPhase);
+
     /**
      * True if the application is in the process of shutting down.
      */

toolkit/crashreporter/test/unit/test_crash_AsyncShutdown.js

@@ -61,7 +61,9 @@ function setup_osfile_crash_noerror() {
   );
   OS.File.getCurrentDirectory();
 
-  Services.obs.notifyObservers(null, "profile-before-change");
+  Services.startup.advanceShutdownPhase(
+    Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+  );
   dump("Waiting for crash\n");
 }
 
@@ -97,7 +99,9 @@ function setup_osfile_crash_exn() {
   );
   OS.File.read("I do not exist");
 
-  Services.obs.notifyObservers(null, "profile-before-change");
+  Services.startup.advanceShutdownPhase(
+    Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+  );
   dump("Waiting for crash\n");
 }
 

toolkit/mozapps/downloads/tests/unit/head_downloads.js

@@ -1,5 +1,7 @@
 var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 
 registerCleanupFunction(function() {
-  Services.obs.notifyObservers(null, "quit-application");
+  Services.startup.advanceShutdownPhase(
+    Services.startup.SHUTDOWN_PHASE_APPSHUTDOWNCONFIRMED
+  );
 });

toolkit/mozapps/extensions/test/xpcshell/test_shutdown_early.js

@@ -41,7 +41,9 @@ add_task(async function test_shutdown_immediately_after_startup() {
   );
 
   info("Immediate exit at startup, without quit-application-granted");
-  Services.obs.notifyObservers(null, "profile-before-change");
+  Services.startup.advanceShutdownPhase(
+    Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+  );
   let shutdownPromise = MockAsyncShutdown.profileBeforeChange.trigger();
   equal(shutdownCount, 1, "AddonManager.beforeShutdown has started");
 

tools/fuzzing/interface/harness/FuzzerTestHarness.h

@@ -28,6 +28,7 @@
 #include "nsIObserverService.h"
 #include "nsIServiceManager.h"
 #include "nsXULAppAPI.h"
+#include "mozilla/AppShutdown.h"
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
@@ -78,21 +79,16 @@ class ScopedXPCOM final : public nsIDirectoryServiceProvider2 {
   ~ScopedXPCOM() {
     // If we created a profile directory, we need to remove it.
     if (mProfD) {
-      nsCOMPtr<nsIObserverService> os =
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-          do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
+          mozilla::ShutdownPhase::AppShutdownNetTeardown);
-      MOZ_ASSERT(os);
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-      if (os) {
+          mozilla::ShutdownPhase::AppShutdownTeardown);
-        MOZ_ALWAYS_SUCCEEDS(os->NotifyObservers(
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-            nullptr, "profile-change-net-teardown", nullptr));
+          mozilla::ShutdownPhase::AppShutdown);
-        MOZ_ALWAYS_SUCCEEDS(
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-            os->NotifyObservers(nullptr, "profile-change-teardown", nullptr));
+          mozilla::ShutdownPhase::AppShutdownQM);
-        MOZ_ALWAYS_SUCCEEDS(
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-            os->NotifyObservers(nullptr, "profile-before-change", nullptr));
+          mozilla::ShutdownPhase::AppShutdownTelemetry);
-        MOZ_ALWAYS_SUCCEEDS(
-            os->NotifyObservers(nullptr, "profile-before-change-qm", nullptr));
-        MOZ_ALWAYS_SUCCEEDS(os->NotifyObservers(
-            nullptr, "profile-before-change-telemetry", nullptr));
-      }
 
       if (NS_FAILED(mProfD->Remove(true))) {
         NS_WARNING("Problem removing profile directory");

xpcom/base/AppShutdown.cpp

@@ -110,6 +110,10 @@ ShutdownPhase AppShutdown::GetCurrentShutdownPhase() {
   return sCurrentShutdownPhase;
 }
 
+bool AppShutdown::IsInOrBeyond(ShutdownPhase aPhase) {
+  return (sCurrentShutdownPhase >= aPhase);
+}
+
 int AppShutdown::GetExitCode() { return sExitCode; }
 
 void AppShutdown::SaveEnvVarsForPotentialRestart() {
@@ -304,11 +308,33 @@ bool AppShutdown::IsRestarting() {
   return sShutdownMode == AppShutdownMode::Restart;
 }
 
+#ifdef DEBUG
+static bool sNotifyingShutdownObservers = false;
+
+bool AppShutdown::IsNoOrLegalShutdownTopic(const char* aTopic) {
+  if (!XRE_IsParentProcess()) {
+    // Until we know what to do with AppShutdown for child processes,
+    // we ignore them for now. See bug 1697745.
+    return true;
+  }
+  ShutdownPhase phase = GetShutdownPhaseFromTopic(aTopic);
+  return phase == ShutdownPhase::NotInShutdown ||
+         (sNotifyingShutdownObservers && phase == sCurrentShutdownPhase);
+}
+#endif
+
 void AdvanceShutdownPhaseInternal(
     ShutdownPhase aPhase, bool doNotify, const char16_t* aNotificationData,
     const nsCOMPtr<nsISupports>& aNotificationSubject) {
-  MOZ_ASSERT(aPhase >= sCurrentShutdownPhase);
+  AssertIsOnMainThread();
-  if (sCurrentShutdownPhase >= aPhase) return;
+
+  // We ensure that we can move only forward. We cannot
+  // MOZ_ASSERT here as there are some tests that fire
+  // notifications out of shutdown order.
+  // See for example test_sss_sanitizeOnShutdown.js
+  if (sCurrentShutdownPhase >= aPhase) {
+    return;
+  }
   sCurrentShutdownPhase = aPhase;
 
 #ifndef ANDROID
@@ -327,6 +353,10 @@ void AdvanceShutdownPhaseInternal(
       nsCOMPtr<nsIObserverService> obsService =
           mozilla::services::GetObserverService();
       if (obsService) {
+#ifdef DEBUG
+        sNotifyingShutdownObservers = true;
+        auto reset = MakeScopeExit([] { sNotifyingShutdownObservers = false; });
+#endif
         obsService->NotifyObservers(aNotificationSubject, aTopic,
                                     aNotificationData);
       }
@@ -350,4 +380,13 @@ void AppShutdown::AdvanceShutdownPhase(
                                aNotificationSubject);
 }
 
+ShutdownPhase AppShutdown::GetShutdownPhaseFromTopic(const char* aTopic) {
+  for (size_t i = 0; i < ArrayLength(sPhaseObserverKeys); ++i) {
+    if (sPhaseObserverKeys[i] && !strcmp(sPhaseObserverKeys[i], aTopic)) {
+      return static_cast<ShutdownPhase>(i);
+    }
+  }
+  return ShutdownPhase::NotInShutdown;
+}
+
 }  // namespace mozilla

xpcom/base/AppShutdown.h

@@ -21,6 +21,7 @@ class AppShutdown {
  public:
   static bool IsShuttingDown();
   static ShutdownPhase GetCurrentShutdownPhase();
+  static bool IsInOrBeyond(ShutdownPhase aPhase);
 
   /**
    * Returns the current exit code that the process will be terminated with.
@@ -89,9 +90,23 @@ class AppShutdown {
   static void MaybeFastShutdown(ShutdownPhase aPhase);
 
   /**
-   * Map shutdown phases to observer keys
+   * Map shutdown phase to observer key
    */
   static const char* GetObserverKey(ShutdownPhase aPhase);
+
+  /**
+   * Map observer topic key to shutdown phase
+   */
+  static ShutdownPhase GetShutdownPhaseFromTopic(const char* aTopic);
+
+#ifdef DEBUG
+  /**
+   * Check, if we are allowed to send a shutdown notification.
+   * Shutdown specific topics are only allowed during calls to
+   * AdvanceShutdownPhase itself.
+   */
+  static bool IsNoOrLegalShutdownTopic(const char* aTopic);
+#endif
 };
 
 }  // namespace mozilla

xpcom/ds/nsIObserverService.idl

@@ -54,6 +54,8 @@ interface nsIObserverService : nsISupports
      * notifyObservers
      *
      * Notifies all registered listeners of the given topic.
+     * Must not be used with shutdown topics (will assert
+     * on the parent process).
      *
      * @param aSubject : Notification specific interface pointer.
      * @param aTopic   : The notification topic or subject.

xpcom/ds/nsObserverService.cpp

@@ -17,6 +17,7 @@
 #include "nsThreadUtils.h"
 #include "nsEnumeratorUtils.h"
 #include "xpcpublic.h"
+#include "mozilla/AppShutdown.h"
 #include "mozilla/net/NeckoCommon.h"
 #include "mozilla/ProfilerLabels.h"
 #include "mozilla/ProfilerMarkers.h"
@@ -276,6 +277,8 @@ NS_IMETHODIMP nsObserverService::NotifyObservers(nsISupports* aSubject,
     return NS_ERROR_INVALID_ARG;
   }
 
+  MOZ_ASSERT(AppShutdown::IsNoOrLegalShutdownTopic(aTopic));
+
   mozilla::TimeStamp start = TimeStamp::Now();
 
   AUTO_PROFILER_MARKER_TEXT("NotifyObservers", OTHER, MarkerStack::Capture(),

xpcom/tests/TestHarness.h

@@ -31,6 +31,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
+#include "mozilla/AppShutdown.h"
 
 static uint32_t gFailCount = 0;
 
@@ -93,19 +94,16 @@ class ScopedXPCOM : public nsIDirectoryServiceProvider2 {
   ~ScopedXPCOM() {
     // If we created a profile directory, we need to remove it.
     if (mProfD) {
-      nsCOMPtr<nsIObserverService> os =
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-          do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
+          mozilla::ShutdownPhase::AppShutdownNetTeardown);
-      MOZ_RELEASE_ASSERT(os);
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-      MOZ_ALWAYS_SUCCEEDS(
+          mozilla::ShutdownPhase::AppShutdownTeardown);
-          os->NotifyObservers(nullptr, "profile-change-net-teardown", nullptr));
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-      MOZ_ALWAYS_SUCCEEDS(
+          mozilla::ShutdownPhase::AppShutdown);
-          os->NotifyObservers(nullptr, "profile-change-teardown", nullptr));
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-      MOZ_ALWAYS_SUCCEEDS(
+          mozilla::ShutdownPhase::AppShutdownQM);
-          os->NotifyObservers(nullptr, "profile-before-change", nullptr));
+      mozilla::AppShutdown::AdvanceShutdownPhase(
-      MOZ_ALWAYS_SUCCEEDS(
+          mozilla::ShutdownPhase::AppShutdownTelemetry);
-          os->NotifyObservers(nullptr, "profile-before-change-qm", nullptr));
-      MOZ_ALWAYS_SUCCEEDS(os->NotifyObservers(
-          nullptr, "profile-before-change-telemetry", nullptr));
 
       if (NS_FAILED(mProfD->Remove(true))) {
         NS_WARNING("Problem removing profile directory");