Bug 1994241. a=pascalc

Original Revision: https://phabricator.services.mozilla.com/D268585

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

gfx/ipc/GPUProcessManager.cpp

@@ -80,7 +80,13 @@ void GPUProcessManager::Initialize() {
   sSingleton = new GPUProcessManager();
 }
 
-void GPUProcessManager::Shutdown() { sSingleton = nullptr; }
+void GPUProcessManager::Shutdown() {
+  if (!sSingleton) {
+    return;
+  }
+  sSingleton->ShutdownInternal();
+  sSingleton = nullptr;
+}
 
 GPUProcessManager::GPUProcessManager()
     : mTaskFactory(this),
@@ -114,67 +120,83 @@ GPUProcessManager::~GPUProcessManager() {
   MOZ_ASSERT(!mProcess && !mGPUChild);
 
   // We should have already removed observers.
-  MOZ_ASSERT(!mObserver);
+  MOZ_DIAGNOSTIC_ASSERT(!mObserver);
+  MOZ_DIAGNOSTIC_ASSERT(!mBatteryObserver);
 }
 
 NS_IMPL_ISUPPORTS(GPUProcessManager::Observer, nsIObserver);
 
-GPUProcessManager::Observer::Observer(GPUProcessManager* aManager)
+GPUProcessManager::Observer::Observer() {
-    : mManager(aManager) {}
+  nsContentUtils::RegisterShutdownObserver(this);
+  Preferences::AddStrongObserver(this, "");
+  if (nsCOMPtr<nsIObserverService> obsServ = services::GetObserverService()) {
+    obsServ->AddObserver(this, "application-foreground", false);
+    obsServ->AddObserver(this, "application-background", false);
+    obsServ->AddObserver(this, "screen-information-changed", false);
+    obsServ->AddObserver(this, "xpcom-will-shutdown", false);
+  }
+}
+
+void GPUProcessManager::Observer::Shutdown() {
+  nsContentUtils::UnregisterShutdownObserver(this);
+  Preferences::RemoveObserver(this, "");
+  if (nsCOMPtr<nsIObserverService> obsServ = services::GetObserverService()) {
+    obsServ->RemoveObserver(this, "application-foreground");
+    obsServ->RemoveObserver(this, "application-background");
+    obsServ->RemoveObserver(this, "screen-information-changed");
+    obsServ->RemoveObserver(this, "xpcom-will-shutdown");
+  }
+}
 
 NS_IMETHODIMP
 GPUProcessManager::Observer::Observe(nsISupports* aSubject, const char* aTopic,
                                      const char16_t* aData) {
-  if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
+  if (auto* gpm = GPUProcessManager::Get()) {
-    mManager->OnXPCOMShutdown();
+    gpm->NotifyObserve(aTopic, aData);
-  } else if (!strcmp(aTopic, "nsPref:changed")) {
-    mManager->OnPreferenceChange(aData);
-  } else if (!strcmp(aTopic, "application-foreground")) {
-    mManager->mAppInForeground = true;
-    if (!mManager->mProcess && gfxConfig::IsEnabled(Feature::GPU_PROCESS)) {
-      Unused << mManager->LaunchGPUProcess();
-    }
-  } else if (!strcmp(aTopic, "application-background")) {
-    mManager->mAppInForeground = false;
-  } else if (!strcmp(aTopic, "screen-information-changed")) {
-    mManager->ScreenInformationChanged();
   }
   return NS_OK;
 }
 
-GPUProcessManager::BatteryObserver::BatteryObserver(GPUProcessManager* aManager)
+void GPUProcessManager::NotifyObserve(const char* aTopic,
-    : mManager(aManager) {
+                                      const char16_t* aData) {
+  if (!strcmp(aTopic, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID)) {
+    StopBatteryObserving();
+  } else if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
+    ShutdownInternal();
+  } else if (!strcmp(aTopic, "nsPref:changed")) {
+    OnPreferenceChange(aData);
+  } else if (!strcmp(aTopic, "application-foreground")) {
+    mAppInForeground = true;
+    if (!mProcess && gfxConfig::IsEnabled(Feature::GPU_PROCESS)) {
+      Unused << LaunchGPUProcess();
+    }
+  } else if (!strcmp(aTopic, "application-background")) {
+    mAppInForeground = false;
+  } else if (!strcmp(aTopic, "screen-information-changed")) {
+    ScreenInformationChanged();
+  }
+}
+
+GPUProcessManager::BatteryObserver::BatteryObserver() {
   hal::RegisterBatteryObserver(this);
 }
 
 void GPUProcessManager::BatteryObserver::Notify(
     const hal::BatteryInformation& aBatteryInfo) {
-  mManager->NotifyBatteryInfo(aBatteryInfo);
+  if (auto* gpm = GPUProcessManager::Get()) {
+    gpm->NotifyBatteryInfo(aBatteryInfo);
+  }
 }
 
-void GPUProcessManager::BatteryObserver::ShutDown() {
+void GPUProcessManager::BatteryObserver::Shutdown() {
   hal::UnregisterBatteryObserver(this);
 }
 
-GPUProcessManager::BatteryObserver::~BatteryObserver() {}
-
-void GPUProcessManager::OnXPCOMShutdown() {
-  if (mObserver) {
-    nsContentUtils::UnregisterShutdownObserver(mObserver);
-    Preferences::RemoveObserver(mObserver, "");
-    nsCOMPtr<nsIObserverService> obsServ = services::GetObserverService();
-    if (obsServ) {
-      obsServ->RemoveObserver(mObserver, "application-foreground");
-      obsServ->RemoveObserver(mObserver, "application-background");
-      obsServ->RemoveObserver(mObserver, "screen-information-changed");
-    }
-    mObserver = nullptr;
-  }
-
-  CleanShutdown();
-}
-
 void GPUProcessManager::OnPreferenceChange(const char16_t* aData) {
+  if (!mGPUChild && !IsGPUProcessLaunching()) {
+    return;
+  }
+
   // We know prefs are ASCII here.
   NS_LossyConvertUTF16toASCII strData(aData);
 
@@ -183,10 +205,10 @@ void GPUProcessManager::OnPreferenceChange(const char16_t* aData) {
 
   Preferences::GetPreference(&pref, GeckoProcessType_GPU,
                              /* remoteType */ ""_ns);
-  if (!!mGPUChild) {
+  if (mGPUChild) {
     MOZ_ASSERT(mQueuedPrefs.IsEmpty());
     mGPUChild->SendPreferenceUpdate(pref);
-  } else if (IsGPUProcessLaunching()) {
+  } else {
     mQueuedPrefs.AppendElement(pref);
   }
 }
@@ -245,15 +267,7 @@ bool GPUProcessManager::LaunchGPUProcess() {
   // Start listening for pref changes so we can
   // forward them to the process once it is running.
   if (!mObserver) {
-    mObserver = new Observer(this);
+    mObserver = new Observer();
-    nsContentUtils::RegisterShutdownObserver(mObserver);
-    Preferences::AddStrongObserver(mObserver, "");
-    nsCOMPtr<nsIObserverService> obsServ = services::GetObserverService();
-    if (obsServ) {
-      obsServ->AddObserver(mObserver, "application-foreground", false);
-      obsServ->AddObserver(mObserver, "application-background", false);
-      obsServ->AddObserver(mObserver, "screen-information-changed", false);
-    }
   }
 
   // Start the Vsync I/O thread so can use it as soon as the process launches.
@@ -590,8 +604,8 @@ void GPUProcessManager::OnProcessLaunchComplete(GPUProcessHost* aHost) {
                                           std::move(vsyncChild));
   mGPUChild->SendInitVsyncBridge(std::move(vsyncParent));
 
-  MOZ_ASSERT(!mBatteryObserver);
+  MOZ_DIAGNOSTIC_ASSERT(!mBatteryObserver);
-  mBatteryObserver = new BatteryObserver(this);
+  mBatteryObserver = new BatteryObserver();
 
   // Flush any pref updates that happened during launch and weren't
   // included in the blobs set up in LaunchGPUProcess.
@@ -1107,7 +1121,12 @@ void GPUProcessManager::NotifyRemoteActorDestroyed(
   OnProcessUnexpectedShutdown(mProcess);
 }
 
-void GPUProcessManager::CleanShutdown() {
+void GPUProcessManager::ShutdownInternal() {
+  if (mObserver) {
+    mObserver->Shutdown();
+    mObserver = nullptr;
+  }
+
   DestroyProcess();
   mVsyncIOThread = nullptr;
 }
@@ -1149,15 +1168,19 @@ void GPUProcessManager::DestroyProcess(bool aUnexpectedShutdown) {
     mVsyncBridge->Close();
     mVsyncBridge = nullptr;
   }
-  if (mBatteryObserver) {
+  StopBatteryObserving();
-    mBatteryObserver->ShutDown();
-    mBatteryObserver = nullptr;
-  }
 
   CrashReporter::RecordAnnotationCString(
       CrashReporter::Annotation::GPUProcessStatus, "Destroyed");
 }
 
+void GPUProcessManager::StopBatteryObserving() {
+  if (mBatteryObserver) {
+    mBatteryObserver->Shutdown();
+    mBatteryObserver = nullptr;
+  }
+}
+
 already_AddRefed<CompositorSession> GPUProcessManager::CreateTopLevelCompositor(
     nsBaseWidget* aWidget, WebRenderLayerManager* aLayerManager,
     CSSToLayoutDeviceScale aScale, const CompositorOptions& aOptions,

gfx/ipc/GPUProcessManager.h

@@ -188,6 +188,10 @@ class GPUProcessManager final : public GPUProcessHost::Listener {
   // as well as for tests and diagnostics.
   void KillProcess(bool aGenerateMinidump = false);
 
+  // Invoked when we know we will shutdown (but before shutdown begins), to
+  // avoid races with other shutdown observers.
+  void StopBatteryObserving();
+
   // Causes the GPU process to crash. Used for tests and diagnostics
   void CrashProcess();
 
@@ -222,8 +226,6 @@ class GPUProcessManager final : public GPUProcessHost::Listener {
   RefPtr<PGPUChild::TestTriggerMetricsPromise> TestTriggerMetrics();
 
  private:
-  // Called from our xpcom-shutdown observer.
-  void OnXPCOMShutdown();
   void OnPreferenceChange(const char16_t* aData);
   void ScreenInformationChanged();
 
@@ -286,7 +288,7 @@ class GPUProcessManager final : public GPUProcessHost::Listener {
   bool IsProcessStable(const TimeStamp& aNow);
 
   // Shutdown the GPU process.
-  void CleanShutdown();
+  void ShutdownInternal();
   // Destroy the process and clean up resources.
   // Setting aUnexpectedShutdown = true indicates that this is being called to
   // clean up resources in response to an unexpected shutdown having been
@@ -322,33 +324,32 @@ class GPUProcessManager final : public GPUProcessHost::Listener {
 
   DISALLOW_COPY_AND_ASSIGN(GPUProcessManager);
 
+  void NotifyObserve(const char* aTopic, const char16_t* aData);
   void NotifyBatteryInfo(const hal::BatteryInformation& aBatteryInfo);
 
   class Observer final : public nsIObserver {
    public:
     NS_DECL_ISUPPORTS
     NS_DECL_NSIOBSERVER
-    explicit Observer(GPUProcessManager* aManager);
+
+    Observer();
+    void Shutdown();
 
    protected:
     virtual ~Observer() = default;
-
-    GPUProcessManager* mManager;
   };
   friend class Observer;
 
   class BatteryObserver final : public hal::BatteryObserver {
    public:
     NS_INLINE_DECL_REFCOUNTING(BatteryObserver)
-    explicit BatteryObserver(GPUProcessManager* aManager);
 
+    BatteryObserver();
     void Notify(const hal::BatteryInformation& aBatteryInfo) override;
-    void ShutDown();
+    void Shutdown();
 
    protected:
-    virtual ~BatteryObserver();
+    ~BatteryObserver() override = default;
-
-    GPUProcessManager* mManager;
   };
 
  private: