Bug 1412726: Clean up XPCOM singleton constructor refcount handling. …

…r=froydnj This is a follow-up to bug 1409249. There are a lot of places where our factory singleton constructors either don't correctly handle their returned references being released by the component manager, or do handle it, but in ways that are not obvious. This patch handles a few places where we can sometimes wind up with dangling singleton pointers, adds some explanatory comments and sanity check assertions, and replaces some uses of manual refcounting with StaticRefPtr and ClearOnShutdown. There are still some places where we may wind up with odd behavior if the first QI for a getService call fails. In those cases, we wind up destroying the first instance of a service that we create, and re-creating a new one later. MozReview-Commit-ID: ANYndvd7aZx
mozilla-spidermonkey · Oct 29, 2017 · ae1072a · ae1072a
1 parent 4899da2
commit ae1072a
Show file tree

Hide file tree

Showing 26 changed files with 83 additions and 118 deletions.
diff --git a/dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp b/dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp
@@ -16,6 +16,7 @@
 #include "AudioChannelService.h"
 
 #include "nsString.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/ContentParent.h"
@@ -171,6 +172,7 @@ nsSynthVoiceRegistry::GetInstance()
 
   if (!gSynthVoiceRegistry) {
     gSynthVoiceRegistry = new nsSynthVoiceRegistry();
+    ClearOnShutdown(&gSynthVoiceRegistry);
     if (XRE_IsParentProcess()) {
       // Start up all speech synth services.
       NS_CreateServicesFromCategory(NS_SPEECH_SYNTH_STARTED, nullptr,
@@ -189,14 +191,6 @@ nsSynthVoiceRegistry::GetInstanceForService()
   return registry.forget();
 }
 
-void
-nsSynthVoiceRegistry::Shutdown()
-{
-  LOG(LogLevel::Debug, ("[%s] nsSynthVoiceRegistry::Shutdown()",
-                        (XRE_IsContentProcess()) ? "Content" : "Default"));
-  gSynthVoiceRegistry = nullptr;
-}
-
 bool
 nsSynthVoiceRegistry::SendInitialVoicesAndState(SpeechSynthesisParent* aParent)
 {

diff --git a/dom/media/webspeech/synth/nsSynthVoiceRegistry.h b/dom/media/webspeech/synth/nsSynthVoiceRegistry.h
@@ -67,8 +67,6 @@ class nsSynthVoiceRegistry final : public nsISynthVoiceRegistry
 
   static void RecvNotifyVoicesChanged();
 
-  static void Shutdown();
-
 private:
   virtual ~nsSynthVoiceRegistry();
 

diff --git a/dom/media/webspeech/synth/speechd/SpeechDispatcherModule.cpp b/dom/media/webspeech/synth/speechd/SpeechDispatcherModule.cpp
@@ -37,20 +37,14 @@ static const mozilla::Module::CategoryEntry kCategories[] = {
   { nullptr }
 };
 
-static void
-UnloadSpeechDispatcherModule()
-{
-  SpeechDispatcherService::Shutdown();
-}
-
 static const mozilla::Module kModule = {
   mozilla::Module::kVersion,
   kCIDs,
   kContracts,
   kCategories,
   nullptr,
   nullptr,
-  UnloadSpeechDispatcherModule
+  nullptr,
 };
 
 NSMODULE_DEFN(synthspeechdispatcher) = &kModule;
diff --git a/dom/media/webspeech/synth/speechd/SpeechDispatcherService.cpp b/dom/media/webspeech/synth/speechd/SpeechDispatcherService.cpp
@@ -8,6 +8,7 @@
 
 #include "mozilla/dom/nsSpeechTask.h"
 #include "mozilla/dom/nsSynthVoiceRegistry.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Preferences.h"
 #include "nsEscape.h"
 #include "nsISupports.h"
@@ -560,6 +561,7 @@ SpeechDispatcherService::GetInstance(bool create)
   if (!sSingleton && create) {
     sSingleton = new SpeechDispatcherService();
     sSingleton->Init();
+    ClearOnShutdown(&sSingleton);
   }
 
   return sSingleton;
@@ -585,15 +587,5 @@ SpeechDispatcherService::EventNotify(uint32_t aMsgId, uint32_t aState)
   }
 }
 
-void
-SpeechDispatcherService::Shutdown()
-{
-  if (!sSingleton) {
-    return;
-  }
-
-  sSingleton = nullptr;
-}
-
 } // namespace dom
 } // namespace mozilla
diff --git a/dom/media/webspeech/synth/speechd/SpeechDispatcherService.h b/dom/media/webspeech/synth/speechd/SpeechDispatcherService.h
@@ -42,8 +42,6 @@ class SpeechDispatcherService final : public nsIObserver,
   static SpeechDispatcherService* GetInstance(bool create = true);
   static already_AddRefed<SpeechDispatcherService> GetInstanceForService();
 
-  static void Shutdown();
-
   static StaticRefPtr<SpeechDispatcherService> sSingleton;
 
 private:

diff --git a/dom/plugins/base/nsPluginHost.cpp b/dom/plugins/base/nsPluginHost.cpp
@@ -53,6 +53,7 @@
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/ContentParent.h"
 #include "mozilla/dom/FakePluginTagInitBinding.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/LoadInfo.h"
 #include "mozilla/plugins/PluginBridge.h"
 #include "mozilla/plugins/PluginTypes.h"
@@ -150,7 +151,7 @@ LazyLogModule nsPluginLogging::gPluginLog(PLUGIN_LOG_NAME);
 #define DEFAULT_NUMBER_OF_STOPPED_INSTANCES 50
 
 nsIFile *nsPluginHost::sPluginTempDir;
-nsPluginHost *nsPluginHost::sInst;
+StaticRefPtr<nsPluginHost> nsPluginHost::sInst;
 
 /* to cope with short read */
 /* we should probably put this into a global library now that this is the second
@@ -296,7 +297,6 @@ nsPluginHost::~nsPluginHost()
   PLUGIN_LOG(PLUGIN_LOG_ALWAYS,("nsPluginHost::dtor\n"));
 
   UnloadPlugins();
-  sInst = nullptr;
 }
 
 NS_IMPL_ISUPPORTS(nsPluginHost,
@@ -311,13 +311,10 @@ nsPluginHost::GetInst()
 {
   if (!sInst) {
     sInst = new nsPluginHost();
-    if (!sInst)
-      return nullptr;
-    NS_ADDREF(sInst);
+    ClearOnShutdown(&sInst);
   }
 
-  RefPtr<nsPluginHost> inst = sInst;
-  return inst.forget();
+  return do_AddRef(sInst);
 }
 
 bool nsPluginHost::IsRunningPlugin(nsPluginTag * aPluginTag)
@@ -3381,7 +3378,6 @@ NS_IMETHODIMP nsPluginHost::Observe(nsISupports *aSubject,
 {
   if (!strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, aTopic)) {
     UnloadPlugins();
-    sInst->Release();
   }
   if (!strcmp(NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, aTopic)) {
     mPluginsDisabled = Preferences::GetBool("plugin.disable", false);

diff --git a/dom/plugins/base/nsPluginHost.h b/dom/plugins/base/nsPluginHost.h
@@ -7,6 +7,7 @@
 #define nsPluginHost_h_
 
 #include "mozilla/LinkedList.h"
+#include "mozilla/StaticPtr.h"
 
 #include "nsIPluginHost.h"
 #include "nsIObserver.h"
@@ -415,7 +416,7 @@ class nsPluginHost final : public nsIPluginHost,
 
   // We need to hold a global ptr to ourselves because we register for
   // two different CIDs for some reason...
-  static nsPluginHost* sInst;
+  static mozilla::StaticRefPtr<nsPluginHost> sInst;
 };
 
 class PluginDestructionGuard : public mozilla::LinkedListElement<PluginDestructionGuard>

diff --git a/dom/workers/WorkerDebuggerManager.cpp b/dom/workers/WorkerDebuggerManager.cpp
@@ -9,6 +9,7 @@
 #include "nsISimpleEnumerator.h"
 
 #include "mozilla/ClearOnShutdown.h"
+#include "mozilla/StaticPtr.h"
 
 #include "WorkerPrivate.h"
 
@@ -69,8 +70,7 @@ class UnregisterDebuggerMainThreadRunnable final : public mozilla::Runnable
   }
 };
 
-// Does not hold an owning reference.
-static WorkerDebuggerManager* gWorkerDebuggerManager;
+static StaticRefPtr<WorkerDebuggerManager> gWorkerDebuggerManager;
 
 } /* anonymous namespace */
 
@@ -143,10 +143,11 @@ WorkerDebuggerManager::GetOrCreate()
   if (!gWorkerDebuggerManager) {
     // The observer service now owns us until shutdown.
     gWorkerDebuggerManager = new WorkerDebuggerManager();
-    if (NS_FAILED(gWorkerDebuggerManager->Init())) {
+    if (NS_SUCCEEDED(gWorkerDebuggerManager->Init())) {
+      ClearOnShutdown(&gWorkerDebuggerManager);
+    } else {
       NS_WARNING("Failed to initialize worker debugger manager!");
       gWorkerDebuggerManager = nullptr;
-      return nullptr;
     }
   }
 

diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp
@@ -929,7 +929,10 @@ nsPermissionManager::~nsPermissionManager()
   mPermissionKeyPromiseMap.Clear();
 
   RemoveAllFromMemory();
-  gPermissionManager = nullptr;
+  if (gPermissionManager) {
+    MOZ_ASSERT(gPermissionManager == this);
+    gPermissionManager = nullptr;
+  }
 }
 
 // static
@@ -948,11 +951,12 @@ nsPermissionManager::GetXPCOMSingleton()
   // See bug 209571.
   auto permManager = MakeRefPtr<nsPermissionManager>();
   if (NS_SUCCEEDED(permManager->Init())) {
+    // Note: This is cleared in the nsPermissionManager destructor.
     gPermissionManager = permManager.get();
     return permManager.forget();
   }
 
-  return nullptr;;
+  return nullptr;
 }
 
 nsresult

diff --git a/js/xpconnect/src/nsXPConnect.cpp b/js/xpconnect/src/nsXPConnect.cpp
@@ -111,6 +111,7 @@ nsXPConnect::~nsXPConnect()
 
     delete gPrimaryContext;
 
+    MOZ_ASSERT(gSelf == this);
     gSelf = nullptr;
     gOnceAliveNowDead = true;
 }

diff --git a/layout/build/nsLayoutStatics.cpp b/layout/build/nsLayoutStatics.cpp
@@ -88,10 +88,6 @@
 #include "nsMenuBarListener.h"
 #endif
 
-#ifdef MOZ_WEBSPEECH
-#include "nsSynthVoiceRegistry.h"
-#endif
-
 #include "CubebUtils.h"
 #include "Latency.h"
 #include "WebAudioUtils.h"
@@ -406,10 +402,6 @@ nsLayoutStatics::Shutdown()
   AsyncLatencyLogger::ShutdownLogger();
   WebAudioUtils::Shutdown();
 
-#ifdef MOZ_WEBSPEECH
-  nsSynthVoiceRegistry::Shutdown();
-#endif
-
   nsCORSListenerProxy::Shutdown();
 
   PointerEventHandler::ReleaseStatics();

diff --git a/modules/libjar/nsJARChannel.cpp b/modules/libjar/nsJARChannel.cpp
@@ -204,17 +204,13 @@ nsJARChannel::nsJARChannel()
     mBlockRemoteFiles = Preferences::GetBool("network.jar.block-remote-files", false);
 
     // hold an owning reference to the jar handler
-    NS_ADDREF(gJarHandler);
+    mJarHandler = gJarHandler;
 }
 
 nsJARChannel::~nsJARChannel()
 {
     NS_ReleaseOnMainThreadSystemGroup("nsJARChannel::mLoadInfo",
                                       mLoadInfo.forget());
-
-    // release owning reference to the jar handler
-    nsJARProtocolHandler *handler = gJarHandler;
-    NS_RELEASE(handler); // nullptr parameter
 }
 
 NS_IMPL_ISUPPORTS_INHERITED(nsJARChannel,

diff --git a/modules/libjar/nsJARChannel.h b/modules/libjar/nsJARChannel.h
@@ -26,6 +26,7 @@
 #include "mozilla/Logging.h"
 
 class nsJARInputThunk;
+class nsJARProtocolHandler;
 class nsInputStreamPump;
 
 //-----------------------------------------------------------------------------
@@ -72,6 +73,7 @@ class nsJARChannel final : public nsIJARChannel
 
     bool                            mOpened;
 
+    RefPtr<nsJARProtocolHandler>    mJarHandler;
     nsCOMPtr<nsIJARURI>             mJarURI;
     nsCOMPtr<nsIURI>                mOriginalURI;
     nsCOMPtr<nsISupports>           mOwner;

diff --git a/modules/libjar/nsJARFactory.cpp b/modules/libjar/nsJARFactory.cpp
@@ -47,23 +47,14 @@ static const mozilla::Module::ContractIDEntry kJARContracts[] = {
     { nullptr }
 };
 
-// Jar module shutdown hook
-static void nsJarShutdown()
-{
-    // Make sure to not null out gJarHandler here, because we may have
-    // still-live nsJARChannels that will want to release it.
-    nsJARProtocolHandler *handler = gJarHandler;
-    NS_IF_RELEASE(handler);
-}
-
 static const mozilla::Module kJARModule = {
     mozilla::Module::kVersion,
     kJARCIDs,
     kJARContracts,
     nullptr,
     nullptr,
     nullptr,
-    nsJarShutdown
+    nullptr
 };
 
 NSMODULE_DEFN(nsJarModule) = &kJARModule;
diff --git a/modules/libjar/nsJARProtocolHandler.cpp b/modules/libjar/nsJARProtocolHandler.cpp
@@ -3,6 +3,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "mozilla/ClearOnShutdown.h"
 #include "nsAutoPtr.h"
 #include "nsJARProtocolHandler.h"
 #include "nsIIOService.h"
@@ -24,18 +25,15 @@ static NS_DEFINE_CID(kZipReaderCacheCID, NS_ZIPREADERCACHE_CID);
 
 //-----------------------------------------------------------------------------
 
-nsJARProtocolHandler *gJarHandler = nullptr;
+StaticRefPtr<nsJARProtocolHandler> gJarHandler;
 
 nsJARProtocolHandler::nsJARProtocolHandler()
 {
     MOZ_ASSERT(NS_IsMainThread());
 }
 
 nsJARProtocolHandler::~nsJARProtocolHandler()
-{
-    MOZ_ASSERT(gJarHandler == this);
-    gJarHandler = nullptr;
-}
+{}
 
 nsresult
 nsJARProtocolHandler::Init()
@@ -67,15 +65,12 @@ already_AddRefed<nsJARProtocolHandler>
 nsJARProtocolHandler::GetSingleton()
 {
     if (!gJarHandler) {
-        auto jar = MakeRefPtr<nsJARProtocolHandler>();
-        gJarHandler = jar.get();
-        if (NS_FAILED(jar->Init())) {
+        gJarHandler = new nsJARProtocolHandler();
+        if (NS_SUCCEEDED(gJarHandler->Init())) {
+            ClearOnShutdown(&gJarHandler);
+        } else {
             gJarHandler = nullptr;
-            return nullptr;
         }
-        // We release this reference on module shutdown.
-        NS_ADDREF(gJarHandler);
-        return jar.forget();
     }
     return do_AddRef(gJarHandler);
 }

diff --git a/modules/libjar/nsJARProtocolHandler.h b/modules/libjar/nsJARProtocolHandler.h
@@ -6,6 +6,7 @@
 #ifndef nsJARProtocolHandler_h__
 #define nsJARProtocolHandler_h__
 
+#include "mozilla/StaticPtr.h"
 #include "nsIJARProtocolHandler.h"
 #include "nsIProtocolHandler.h"
 #include "nsIJARURI.h"
@@ -39,7 +40,7 @@ class nsJARProtocolHandler final : public nsIJARProtocolHandler
     nsCOMPtr<nsIMIMEService> mMimeService;
 };
 
-extern nsJARProtocolHandler *gJarHandler;
+extern mozilla::StaticRefPtr<nsJARProtocolHandler> gJarHandler;
 
 #define NS_JARPROTOCOLHANDLER_CID                    \
 { /* 0xc7e410d4-0x85f2-11d3-9f63-006008a6efe9 */     \