Bug 1409249: Require singleton constructors to return explicit alread…

…y_AddRefed. r=froydnj

Right now, NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR expects singleton
constructors to return already-addrefed raw pointers, and while it accepts
constructors that return already_AddRefed, most existing don't do so.

Meanwhile, the convention elsewhere is that a raw pointer return value is
owned by the callee, and that the caller needs to addref it if it wants to
keep its own reference to it.

The difference in convention makes it easy to leak (I've definitely caused
more than one shutdown leak this way), so it would be better if we required
the singleton getters to return an explicit already_AddRefed, which would
behave the same for all callers.


This also cleans up several singleton constructors that left a dangling
pointer to their singletons when their initialization methods failed, when
they released their references without clearing their global raw pointers.

MozReview-Commit-ID: 9peyG4pRYcr

caps/nsScriptSecurityManager.cpp

@@ -1437,13 +1437,12 @@ nsScriptSecurityManager::InitStatics()
 // Currently this nsGenericFactory constructor is used only from FastLoad
 // (XPCOM object deserialization) code, when "creating" the system principal
 // singleton.
-SystemPrincipal *
+already_AddRefed<SystemPrincipal>
 nsScriptSecurityManager::SystemPrincipalSingletonConstructor()
 {
-    nsIPrincipal *sysprin = nullptr;
     if (gScriptSecMan)
-        NS_ADDREF(sysprin = gScriptSecMan->mSystemPrincipal);
-    return static_cast<SystemPrincipal*>(sysprin);
+        return do_AddRef(gScriptSecMan->mSystemPrincipal.get()).downcast<SystemPrincipal>();
+    return nullptr;
 }
 
 struct IsWhitespace {

caps/nsScriptSecurityManager.h

@@ -53,7 +53,7 @@ class nsScriptSecurityManager final : public nsIScriptSecurityManager,
     // Invoked exactly once, by XPConnect.
     static void InitStatics();
 
-    static SystemPrincipal*
+    static already_AddRefed<SystemPrincipal>
     SystemPrincipalSingletonConstructor();
 
     /**

dom/base/DOMRequest.h

@@ -103,12 +103,10 @@ class DOMRequestService final : public nsIDOMRequestService
   NS_DECL_ISUPPORTS
   NS_DECL_NSIDOMREQUESTSERVICE
 
-  // Returns an owning reference! No one should call this but the factory.
-  static DOMRequestService* FactoryCreate()
+  // No one should call this but the factory.
+  static already_AddRefed<DOMRequestService> FactoryCreate()
   {
-    DOMRequestService* res = new DOMRequestService;
-    NS_ADDREF(res);
-    return res;
+    return MakeAndAddRef<DOMRequestService>();
   }
 };
 

dom/quota/QuotaManagerService.cpp

@@ -225,14 +225,11 @@ QuotaManagerService::Get()
 }
 
 // static
-QuotaManagerService*
+already_AddRefed<QuotaManagerService>
 QuotaManagerService::FactoryCreate()
 {
-  // Returns a raw pointer that carries an owning reference! Lame, but the
-  // singleton factory macros force this.
-  QuotaManagerService* quotaManagerService = GetOrCreate();
-  NS_IF_ADDREF(quotaManagerService);
-  return quotaManagerService;
+  RefPtr<QuotaManagerService> quotaManagerService = GetOrCreate();
+  return quotaManagerService.forget();
 }
 
 void

dom/quota/QuotaManagerService.h

@@ -63,8 +63,8 @@ class QuotaManagerService final
   static QuotaManagerService*
   Get();
 
-  // Returns an owning reference! No one should call this but the factory.
-  static QuotaManagerService*
+  // No one should call this but the factory.
+  static already_AddRefed<QuotaManagerService>
   FactoryCreate();
 
   void

extensions/cookie/nsPermissionManager.cpp

@@ -926,12 +926,11 @@ nsPermissionManager::~nsPermissionManager()
 }
 
 // static
-nsIPermissionManager*
+already_AddRefed<nsIPermissionManager>
 nsPermissionManager::GetXPCOMSingleton()
 {
   if (gPermissionManager) {
-    NS_ADDREF(gPermissionManager);
-    return gPermissionManager;
+    return do_AddRef(gPermissionManager);
   }
 
   // Create a new singleton nsPermissionManager.
@@ -940,15 +939,13 @@ nsPermissionManager::GetXPCOMSingleton()
   // Release our members, since GC cycles have already been completed and
   // would result in serious leaks.
   // See bug 209571.
-  gPermissionManager = new nsPermissionManager();
-  if (gPermissionManager) {
-    NS_ADDREF(gPermissionManager);
-    if (NS_FAILED(gPermissionManager->Init())) {
-      NS_RELEASE(gPermissionManager);
-    }
+  auto permManager = MakeRefPtr<nsPermissionManager>();
+  if (NS_SUCCEEDED(permManager->Init())) {
+    gPermissionManager = permManager.get();
+    return permManager.forget();
   }
 
-  return gPermissionManager;
+  return nullptr;;
 }
 
 nsresult

extensions/cookie/nsPermissionManager.h

@@ -167,7 +167,7 @@ class nsPermissionManager final : public nsIPermissionManager,
   NS_DECL_NSIOBSERVER
 
   nsPermissionManager();
-  static nsIPermissionManager* GetXPCOMSingleton();
+  static already_AddRefed<nsIPermissionManager> GetXPCOMSingleton();
   nsresult Init();
 
   // enums for AddInternal()

js/xpconnect/src/nsXPConnect.cpp

@@ -141,12 +141,10 @@ nsXPConnect::InitStatics()
     gSelf->mRuntime->InitSingletonScopes();
 }
 
-nsXPConnect*
+already_AddRefed<nsXPConnect>
 nsXPConnect::GetSingleton()
 {
-    nsXPConnect* xpc = nsXPConnect::XPConnect();
-    NS_IF_ADDREF(xpc);
-    return xpc;
+    return do_AddRef(nsXPConnect::XPConnect());
 }
 
 // static

js/xpconnect/src/xpcprivate.h

@@ -257,10 +257,7 @@ class nsXPConnect final : public nsIXPConnect
         return gSystemPrincipal;
     }
 
-    // This returns an AddRef'd pointer. It does not do this with an 'out' param
-    // only because this form is required by the generic module macro:
-    // NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
-    static nsXPConnect* GetSingleton();
+    static already_AddRefed<nsXPConnect> GetSingleton();
 
     // Called by module code in dll startup
     static void InitStatics();

modules/libjar/nsJARProtocolHandler.cpp

@@ -63,23 +63,19 @@ NS_IMPL_ISUPPORTS(nsJARProtocolHandler,
                   nsIProtocolHandler,
                   nsISupportsWeakReference)
 
-nsJARProtocolHandler*
+already_AddRefed<nsJARProtocolHandler>
 nsJARProtocolHandler::GetSingleton()
 {
     if (!gJarHandler) {
-        gJarHandler = new nsJARProtocolHandler();
-        if (!gJarHandler)
-            return nullptr;
-
-        NS_ADDREF(gJarHandler);
-        nsresult rv = gJarHandler->Init();
-        if (NS_FAILED(rv)) {
-            NS_RELEASE(gJarHandler);
+        auto jar = MakeRefPtr<nsJARProtocolHandler>();
+        gJarHandler = jar.get();
+        if (NS_FAILED(jar->Init())) {
+            gJarHandler = nullptr;
             return nullptr;
         }
+        return jar.forget();
     }
-    NS_ADDREF(gJarHandler);
-    return gJarHandler;
+    return do_AddRef(gJarHandler);
 }
 
 NS_IMETHODIMP

modules/libjar/nsJARProtocolHandler.h

@@ -25,7 +25,7 @@ class nsJARProtocolHandler final : public nsIJARProtocolHandler
     // nsJARProtocolHandler methods:
     nsJARProtocolHandler();
 
-    static nsJARProtocolHandler *GetSingleton();
+    static already_AddRefed<nsJARProtocolHandler> GetSingleton();
 
     nsresult Init();
 

modules/libpref/Preferences.cpp

@@ -3826,12 +3826,11 @@ class AddPreferencesMemoryReporterRunnable : public Runnable
 
 } // namespace
 
-/* static */ Preferences*
+/* static */ already_AddRefed<Preferences>
 Preferences::GetInstanceForService()
 {
   if (sPreferences) {
-    NS_ADDREF(sPreferences);
-    return sPreferences;
+    return do_AddRef(sPreferences);
   }
 
   if (sShutdown) {
@@ -3868,8 +3867,7 @@ Preferences::GetInstanceForService()
     new AddPreferencesMemoryReporterRunnable();
   NS_DispatchToMainThread(runnable);
 
-  NS_ADDREF(sPreferences);
-  return sPreferences;
+  return do_AddRef(sPreferences);
 }
 
 /* static */ bool

modules/libpref/Preferences.h

@@ -78,7 +78,7 @@ class Preferences final
   static void InitializeUserPrefs();
 
   // Returns the singleton instance which is addreffed.
-  static Preferences* GetInstanceForService();
+  static already_AddRefed<Preferences> GetInstanceForService();
 
   // Finallizes global members.
   static void Shutdown();

netwerk/base/nsIOService.cpp

@@ -351,23 +351,19 @@ nsIOService::InitializeProtocolProxyService()
     return rv;
 }
 
-nsIOService*
+already_AddRefed<nsIOService>
 nsIOService::GetInstance() {
     if (!gIOService) {
-        gIOService = new nsIOService();
-        if (!gIOService)
-            return nullptr;
-        NS_ADDREF(gIOService);
-
-        nsresult rv = gIOService->Init();
-        if (NS_FAILED(rv)) {
-            NS_RELEASE(gIOService);
+        RefPtr<nsIOService> ios = new nsIOService();
+        gIOService = ios.get();
+        if (NS_FAILED(ios->Init())) {
+            gIOService = nullptr;
             return nullptr;
         }
-        return gIOService;
+
+        return ios.forget();
     }
-    NS_ADDREF(gIOService);
-    return gIOService;
+    return do_AddRef(gIOService);
 }
 
 NS_IMPL_ISUPPORTS(nsIOService,

netwerk/base/nsIOService.h

@@ -64,8 +64,7 @@ class nsIOService final : public nsIIOService2
 
     // Gets the singleton instance of the IO Service, creating it as needed
     // Returns nullptr on out of memory or failure to initialize.
-    // Returns an addrefed pointer.
-    static nsIOService* GetInstance();
+    static already_AddRefed<nsIOService> GetInstance();
 
     nsresult Init();
     nsresult NewURI(const char* aSpec, nsIURI* aBaseURI,

netwerk/dns/ChildDNSService.cpp

@@ -26,16 +26,15 @@ namespace net {
 static ChildDNSService *gChildDNSService;
 static const char kPrefNameDisablePrefetch[] = "network.dns.disablePrefetch";
 
-ChildDNSService* ChildDNSService::GetSingleton()
+already_AddRefed<ChildDNSService> ChildDNSService::GetSingleton()
 {
   MOZ_ASSERT(IsNeckoChild());
 
   if (!gChildDNSService) {
     gChildDNSService = new ChildDNSService();
   }
 
-  NS_ADDREF(gChildDNSService);
-  return gChildDNSService;
+  return do_AddRef(gChildDNSService);
 }
 
 NS_IMPL_ISUPPORTS(ChildDNSService,

netwerk/dns/ChildDNSService.h

@@ -32,7 +32,7 @@ class ChildDNSService final
 
   ChildDNSService();
 
-  static ChildDNSService* GetSingleton();
+  static already_AddRefed<ChildDNSService> GetSingleton();
 
   void NotifyRequestDone(DNSRequestChild *aDnsRequest);
 

netwerk/dns/nsDNSService2.cpp

@@ -499,7 +499,7 @@ NS_IMPL_ISUPPORTS(nsDNSService, nsIDNSService, nsPIDNSService, nsIObserver,
  ******************************************************************************/
 static nsDNSService *gDNSService;
 
-nsIDNSService*
+already_AddRefed<nsIDNSService>
 nsDNSService::GetXPCOMSingleton()
 {
     if (IsNeckoChild()) {
@@ -509,25 +509,23 @@ nsDNSService::GetXPCOMSingleton()
     return GetSingleton();
 }
 
-nsDNSService*
+already_AddRefed<nsDNSService>
 nsDNSService::GetSingleton()
 {
     NS_ASSERTION(!IsNeckoChild(), "not a parent process");
 
     if (gDNSService) {
-        NS_ADDREF(gDNSService);
-        return gDNSService;
+        return do_AddRef(gDNSService);
     }
 
-    gDNSService = new nsDNSService();
-    if (gDNSService) {
-        NS_ADDREF(gDNSService);
-        if (NS_FAILED(gDNSService->Init())) {
-              NS_RELEASE(gDNSService);
-        }
+    auto dns = MakeRefPtr<nsDNSService>();
+    gDNSService = dns.get();
+    if (NS_FAILED(dns->Init())) {
+        gDNSService = nullptr;
+        return nullptr;
     }
 
-    return gDNSService;
+    return dns.forget();
 }
 
 NS_IMETHODIMP

netwerk/dns/nsDNSService2.h

@@ -34,7 +34,7 @@ class nsDNSService final : public nsPIDNSService
 
     nsDNSService();
 
-    static nsIDNSService* GetXPCOMSingleton();
+    static already_AddRefed<nsIDNSService> GetXPCOMSingleton();
 
     size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
 
@@ -51,7 +51,7 @@ class nsDNSService final : public nsPIDNSService
 private:
     ~nsDNSService();
 
-    static nsDNSService* GetSingleton();
+    static already_AddRefed<nsDNSService> GetSingleton();
 
     uint16_t GetAFForLookup(const nsACString &host, uint32_t flags);
 

startupcache/StartupCache.cpp

@@ -700,13 +700,12 @@ StartupCacheWrapper* StartupCacheWrapper::gStartupCacheWrapper = nullptr;
 
 NS_IMPL_ISUPPORTS(StartupCacheWrapper, nsIStartupCache)
 
-StartupCacheWrapper* StartupCacheWrapper::GetSingleton()
+already_AddRefed<StartupCacheWrapper> StartupCacheWrapper::GetSingleton()
 {
   if (!gStartupCacheWrapper)
     gStartupCacheWrapper = new StartupCacheWrapper();
 
-  NS_ADDREF(gStartupCacheWrapper);
-  return gStartupCacheWrapper;
+  return do_AddRef(gStartupCacheWrapper);
 }
 
 nsresult

startupcache/StartupCache.h

@@ -214,7 +214,7 @@ class StartupCacheWrapper final
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSISTARTUPCACHE
 
-  static StartupCacheWrapper* GetSingleton();
+  static already_AddRefed<StartupCacheWrapper> GetSingleton();
   static StartupCacheWrapper *gStartupCacheWrapper;
 };