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

--HG--
extra : rebase_source : acfb0611a028fef6b9387eb5d1d9e285782fbc7c

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)
 {

dom/media/webspeech/synth/nsSynthVoiceRegistry.h

@@ -67,8 +67,6 @@ public:
 
   static void RecvNotifyVoicesChanged();
 
-  static void Shutdown();
-
 private:
   virtual ~nsSynthVoiceRegistry();
 

dom/media/webspeech/synth/speechd/SpeechDispatcherModule.cpp

@@ -37,12 +37,6 @@ static const mozilla::Module::CategoryEntry kCategories[] = {
   { nullptr }
 };
 
-static void
-UnloadSpeechDispatcherModule()
-{
-  SpeechDispatcherService::Shutdown();
-}
-
 static const mozilla::Module kModule = {
   mozilla::Module::kVersion,
   kCIDs,
@@ -50,7 +44,7 @@ static const mozilla::Module kModule = {
   kCategories,
   nullptr,
   nullptr,
-  UnloadSpeechDispatcherModule
+  nullptr,
 };
 
 NSMODULE_DEFN(synthspeechdispatcher) = &kModule;

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

dom/media/webspeech/synth/speechd/SpeechDispatcherService.h

@@ -42,8 +42,6 @@ public:
   static SpeechDispatcherService* GetInstance(bool create = true);
   static already_AddRefed<SpeechDispatcherService> GetInstanceForService();
 
-  static void Shutdown();
-
   static StaticRefPtr<SpeechDispatcherService> sSingleton;
 
 private:

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);

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 @@ private:
 
   // 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>

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 @@ private:
   }
 };
 
-// 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;
     }
   }
 

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

js/xpconnect/src/nsXPConnect.cpp

@@ -111,6 +111,7 @@ nsXPConnect::~nsXPConnect()
 
     delete gPrimaryContext;
 
+    MOZ_ASSERT(gSelf == this);
     gSelf = nullptr;
     gOnceAliveNowDead = true;
 }

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();

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,

modules/libjar/nsJARChannel.h

@@ -26,6 +26,7 @@
 #include "mozilla/Logging.h"
 
 class nsJARInputThunk;
+class nsJARProtocolHandler;
 class nsInputStreamPump;
 
 //-----------------------------------------------------------------------------
@@ -72,6 +73,7 @@ private:
 
     bool                            mOpened;
 
+    RefPtr<nsJARProtocolHandler>    mJarHandler;
     nsCOMPtr<nsIJARURI>             mJarURI;
     nsCOMPtr<nsIURI>                mOriginalURI;
     nsCOMPtr<nsISupports>           mOwner;

modules/libjar/nsJARFactory.cpp

@@ -47,15 +47,6 @@ 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,
@@ -63,7 +54,7 @@ static const mozilla::Module kJARModule = {
     nullptr,
     nullptr,
     nullptr,
-    nsJarShutdown
+    nullptr
 };
 
 NSMODULE_DEFN(nsJarModule) = &kJARModule;

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,7 +25,7 @@ static NS_DEFINE_CID(kZipReaderCacheCID, NS_ZIPREADERCACHE_CID);
 
 //-----------------------------------------------------------------------------
 
-nsJARProtocolHandler *gJarHandler = nullptr;
+StaticRefPtr<nsJARProtocolHandler> gJarHandler;
 
 nsJARProtocolHandler::nsJARProtocolHandler()
 {
@@ -32,10 +33,7 @@ nsJARProtocolHandler::nsJARProtocolHandler()
 }
 
 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);
 }

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 @@ protected:
     nsCOMPtr<nsIMIMEService> mMimeService;
 };
 
-extern nsJARProtocolHandler *gJarHandler;
+extern mozilla::StaticRefPtr<nsJARProtocolHandler> gJarHandler;
 
 #define NS_JARPROTOCOLHANDLER_CID                    \
 { /* 0xc7e410d4-0x85f2-11d3-9f63-006008a6efe9 */     \

netwerk/base/nsIOService.cpp

@@ -78,7 +78,7 @@ namespace net {
 
 #define MAX_RECURSION_COUNT 50
 
-nsIOService* gIOService = nullptr;
+nsIOService* gIOService;
 static bool gHasWarnedUploadChannel2;
 static bool gCaptivePortalEnabled = false;
 static LazyLogModule gIOServiceLog("nsIOService");
@@ -269,7 +269,10 @@ nsIOService::Init()
 
 nsIOService::~nsIOService()
 {
-    gIOService = nullptr;
+    if (gIOService) {
+        MOZ_ASSERT(gIOService == this);
+        gIOService = nullptr;
+    }
 }
 
 nsresult
@@ -355,13 +358,10 @@ already_AddRefed<nsIOService>
 nsIOService::GetInstance() {
     if (!gIOService) {
         RefPtr<nsIOService> ios = new nsIOService();
-        gIOService = ios.get();
-        if (NS_FAILED(ios->Init())) {
-            gIOService = nullptr;
-            return nullptr;
+        if (NS_SUCCEEDED(ios->Init())) {
+            MOZ_ASSERT(gIOService == ios.get());
+            return ios.forget();
         }
-
-        return ios.forget();
     }
     return do_AddRef(gIOService);
 }

netwerk/dns/ChildDNSService.cpp

@@ -11,6 +11,8 @@
 #include "nsIPrefService.h"
 #include "nsIProtocolProxyService.h"
 #include "nsNetCID.h"
+#include "mozilla/ClearOnShutdown.h"
+#include "mozilla/StaticPtr.h"
 #include "mozilla/SystemGroup.h"
 #include "mozilla/net/NeckoChild.h"
 #include "mozilla/net/DNSListenerProxy.h"
@@ -23,7 +25,7 @@ namespace net {
 // ChildDNSService
 //-----------------------------------------------------------------------------
 
-static ChildDNSService *gChildDNSService;
+static StaticRefPtr<ChildDNSService> gChildDNSService;
 static const char kPrefNameDisablePrefetch[] = "network.dns.disablePrefetch";
 
 already_AddRefed<ChildDNSService> ChildDNSService::GetSingleton()
@@ -32,6 +34,7 @@ already_AddRefed<ChildDNSService> ChildDNSService::GetSingleton()
 
   if (!gChildDNSService) {
     gChildDNSService = new ChildDNSService();
+    ClearOnShutdown(&gChildDNSService);
   }
 
   return do_AddRef(gChildDNSService);

netwerk/dns/nsDNSService2.cpp

@@ -34,10 +34,12 @@
 #include "nsINetworkLinkService.h"
 
 #include "mozilla/Attributes.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/net/NeckoCommon.h"
 #include "mozilla/net/ChildDNSService.h"
 #include "mozilla/net/DNSListenerProxy.h"
 #include "mozilla/Services.h"
+#include "mozilla/StaticPtr.h"
 
 using namespace mozilla;
 using namespace mozilla::net;
@@ -496,7 +498,7 @@ NS_IMPL_ISUPPORTS(nsDNSService, nsIDNSService, nsPIDNSService, nsIObserver,
  * nsDNSService impl:
  * singleton instance ctor/dtor methods
  ******************************************************************************/
-static nsDNSService *gDNSService;
+static StaticRefPtr<nsDNSService> gDNSService;
 
 already_AddRefed<nsIDNSService>
 nsDNSService::GetXPCOMSingleton()
@@ -513,18 +515,16 @@ nsDNSService::GetSingleton()
 {
     NS_ASSERTION(!IsNeckoChild(), "not a parent process");
 
-    if (gDNSService) {
-        return do_AddRef(gDNSService);
+    if (!gDNSService) {
+        gDNSService = new nsDNSService();
+        if (NS_SUCCEEDED(gDNSService->Init())) {
+            ClearOnShutdown(&gDNSService);
+        } else {
+            gDNSService = nullptr;
+        }
     }
 
-    auto dns = MakeRefPtr<nsDNSService>();
-    gDNSService = dns.get();
-    if (NS_FAILED(dns->Init())) {
-        gDNSService = nullptr;
-        return nullptr;
-    }
-
-    return dns.forget();
+    return do_AddRef(gDNSService);
 }
 
 NS_IMETHODIMP

storage/VacuumManager.cpp

@@ -320,7 +320,9 @@ VacuumManager::getSingleton()
   }
 
   if (!gVacuumManager) {
-    gVacuumManager = new VacuumManager();
+    auto manager = MakeRefPtr<VacuumManager>();
+    MOZ_ASSERT(gVacuumManager == manager.get());
+    return manager.forget();
   }
   return do_AddRef(gVacuumManager);
 }

storage/mozStorageService.cpp

@@ -219,13 +219,13 @@ Service::getSingleton()
   // main thread.
   NS_ENSURE_TRUE(NS_IsMainThread(), nullptr);
   RefPtr<Service> service = new Service();
-  gService = service.get();
-  if (NS_FAILED(service->initialize())) {
-    gService = nullptr;
-    return nullptr;
+  if (NS_SUCCEEDED(service->initialize())) {
+    // Note: This is cleared in the Service destructor.
+    gService = service.get();
+    return service.forget();
   }
 
-  return service.forget();
+  return nullptr;
 }
 
 int32_t Service::sSynchronousPref;

toolkit/components/downloads/nsDownloadManager.cpp

@@ -35,16 +35,17 @@ nsDownloadManager::GetSingleton()
   }
 
   auto serv = MakeRefPtr<nsDownloadManager>();
+  // Note: This is cleared in the nsDownloadManager constructor.
   gDownloadManagerService = serv.get();
-  if (NS_FAILED(serv->Init())) {
-    gDownloadManagerService = nullptr;
-    return nullptr;
+  if (NS_SUCCEEDED(serv->Init())) {
+    return serv.forget();
   }
-  return serv.forget();
+  return nullptr;
 }
 
 nsDownloadManager::~nsDownloadManager()
 {
+  MOZ_ASSERT(gDownloadManagerService == this);
   gDownloadManagerService = nullptr;
 }
 

toolkit/components/places/History.cpp

@@ -2157,6 +2157,7 @@ History::~History()
 {
   UnregisterWeakMemoryReporter(this);
 
+  MOZ_ASSERT(gService == this);
   gService = nullptr;
 }
 
@@ -2658,9 +2659,10 @@ already_AddRefed<History>
 History::GetSingleton()
 {
   if (!gService) {
-    gService = new History();
-    NS_ENSURE_TRUE(gService, nullptr);
-    gService->InitMemoryReporter();
+    RefPtr<History> svc = new History();
+    MOZ_ASSERT(gService == svc.get());
+    svc->InitMemoryReporter();
+    return svc.forget();
   }
 
   return do_AddRef(gService);

toolkit/components/reputationservice/ApplicationReputation.cpp

@@ -1556,6 +1556,7 @@ already_AddRefed<ApplicationReputationService>
 ApplicationReputationService::GetSingleton()
 {
   if (!gApplicationReputationService) {
+    // Note: This is cleared in the new ApplicationReputationService destructor.
     gApplicationReputationService = new ApplicationReputationService();
   }
   return do_AddRef(gApplicationReputationService);

toolkit/mozapps/extensions/AddonPathService.cpp

@@ -50,6 +50,7 @@ AddonPathService::AddonPathService()
 
 AddonPathService::~AddonPathService()
 {
+  MOZ_ASSERT(sInstance == this);
   sInstance = nullptr;
 }
 

uriloader/prefetch/nsOfflineCacheUpdateService.cpp

@@ -256,6 +256,7 @@ nsOfflineCacheUpdateService::nsOfflineCacheUpdateService()
 
 nsOfflineCacheUpdateService::~nsOfflineCacheUpdateService()
 {
+    MOZ_ASSERT(gOfflineCacheUpdateService == this);
     gOfflineCacheUpdateService = nullptr;
 
     delete mAllowedDomains;
@@ -300,11 +301,9 @@ nsOfflineCacheUpdateService::GetInstance()
 {
     if (!gOfflineCacheUpdateService) {
         auto serv = MakeRefPtr<nsOfflineCacheUpdateService>();
-        gOfflineCacheUpdateService = serv.get();
-        if (NS_FAILED(serv->Init())) {
-            gOfflineCacheUpdateService = nullptr;
-            return nullptr;
-        }
+        if (NS_FAILED(serv->Init()))
+            serv = nullptr;
+        MOZ_ASSERT(gOfflineCacheUpdateService == serv.get());
         return serv.forget();
     }