Bug 1396676: Return already_AddRefed from cookie service GetSingleton() methods. r=jdm

These methods return an addrefed raw pointer, which makes them easy to use in
ways that cause leaks. If they're to continue returning an addrefed pointer,
they should explicitly return an already_AddRefed.

This also switches to StaticRefPtr with ClearOnShutdown for the cached
pointers for the sake of sanity.

MozReview-Commit-ID: D0lDpU8Hqug

--HG--
extra : rebase_source : 7b199070805fc0472eaf8409932517700ed23d49

netwerk/cookie/CookieServiceChild.cpp

@@ -7,6 +7,7 @@
 #include "mozilla/net/NeckoChannelParams.h"
 #include "mozilla/LoadInfo.h"
 #include "mozilla/BasePrincipal.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/ipc/URIUtils.h"
 #include "mozilla/net/NeckoChild.h"
@@ -36,16 +37,17 @@ static const char kPrefThirdPartySession[] =
 static const char kPrefCookieIPCSync[] = "network.cookie.ipc.sync";
 static const char kCookieLeaveSecurityAlone[] = "network.cookie.leave-secure-alone";
 
-static CookieServiceChild *gCookieService;
+static StaticRefPtr<CookieServiceChild> gCookieService;
 
-CookieServiceChild*
+already_AddRefed<CookieServiceChild>
 CookieServiceChild::GetSingleton()
 {
-  if (!gCookieService)
+  if (!gCookieService) {
     gCookieService = new CookieServiceChild();
+    ClearOnShutdown(&gCookieService);
+  }
 
-  NS_ADDREF(gCookieService);
-  return gCookieService;
+  return do_AddRef(gCookieService);
 }
 
 NS_IMPL_ISUPPORTS(CookieServiceChild,

netwerk/cookie/CookieServiceChild.h

@@ -40,7 +40,7 @@ public:
 
   CookieServiceChild();
 
-  static CookieServiceChild* GetSingleton();
+  static already_AddRefed<CookieServiceChild> GetSingleton();
 
   void
   TrackCookieLoad(nsIChannel *aChannel);

netwerk/cookie/CookieServiceParent.cpp

@@ -70,8 +70,7 @@ CookieServiceParent::CookieServiceParent()
   nsCOMPtr<nsICookieService> cs = do_GetService(NS_COOKIESERVICE_CONTRACTID);
 
   // Get the nsCookieService instance directly, so we can call internal methods.
-  mCookieService =
-    already_AddRefed<nsCookieService>(nsCookieService::GetSingleton());
+  mCookieService = nsCookieService::GetSingleton();
   NS_ASSERTION(mCookieService, "couldn't get nsICookieService");
 }
 

netwerk/cookie/nsCookieService.cpp

@@ -5,6 +5,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/Attributes.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Likely.h"
 #include "mozilla/Printf.h"
@@ -72,7 +73,7 @@ using namespace mozilla::net;
  * useful types & constants
  ******************************************************************************/
 
-static nsCookieService *gCookieService;
+static StaticRefPtr<nsCookieService> gCookieService;
 
 // XXX_hack. See bug 178993.
 // This is a hack to hide HttpOnly cookies from older browsers
@@ -638,7 +639,7 @@ DBState::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
  * singleton instance ctor/dtor methods
  ******************************************************************************/
 
-nsICookieService*
+already_AddRefed<nsICookieService>
 nsCookieService::GetXPCOMSingleton()
 {
   if (IsNeckoChild())
@@ -647,14 +648,13 @@ nsCookieService::GetXPCOMSingleton()
   return GetSingleton();
 }
 
-nsCookieService*
+already_AddRefed<nsCookieService>
 nsCookieService::GetSingleton()
 {
   NS_ASSERTION(!IsNeckoChild(), "not a parent process");
 
   if (gCookieService) {
-    NS_ADDREF(gCookieService);
-    return gCookieService;
+    return do_AddRef(gCookieService);
   }
 
   // Create a new singleton nsCookieService.
@@ -665,13 +665,14 @@ nsCookieService::GetSingleton()
   // See bug 209571.
   gCookieService = new nsCookieService();
   if (gCookieService) {
-    NS_ADDREF(gCookieService);
-    if (NS_FAILED(gCookieService->Init())) {
-      NS_RELEASE(gCookieService);
+    if (NS_SUCCEEDED(gCookieService->Init())) {
+      ClearOnShutdown(&gCookieService);
+    } else {
+      gCookieService = nullptr;
     }
   }
 
-  return gCookieService;
+  return do_AddRef(gCookieService);
 }
 
 /* static */ void

netwerk/cookie/nsCookieService.h

@@ -215,8 +215,8 @@ class nsCookieService final : public nsICookieService
     NS_DECL_NSIMEMORYREPORTER
 
     nsCookieService();
-    static nsICookieService*      GetXPCOMSingleton();
-    nsresult                      Init();
+    static already_AddRefed<nsICookieService> GetXPCOMSingleton();
+    nsresult Init();
 
   /**
    * Start watching the observer service for messages indicating that an app has
@@ -330,7 +330,7 @@ class nsCookieService final : public nsICookieService
     friend class ReadCookieDBListener;
     friend class CloseCookieDBListener;
 
-    static nsCookieService*       GetSingleton();
+    static already_AddRefed<nsCookieService> GetSingleton();
     friend class mozilla::net::CookieServiceParent;
 };
 

netwerk/protocol/http/HttpChannelChild.cpp

@@ -203,7 +203,7 @@ HttpChannelChild::HttpChannelChild()
   // IPC HTTP channel is created.
   // We require that the parent cookie service actor exists while
   // processing HTTP responses.
-  CookieServiceChild::GetSingleton();
+  RefPtr<CookieServiceChild> cookieService = CookieServiceChild::GetSingleton();
 }
 
 HttpChannelChild::~HttpChannelChild()

xpcom/base/StaticPtr.h

@@ -274,4 +274,12 @@ RefPtr<T>::operator=(const mozilla::StaticRefPtr<U>& aOther)
   return operator=(aOther.get());
 }
 
+template <class T>
+inline already_AddRefed<T>
+do_AddRef(const mozilla::StaticRefPtr<T>& aObj)
+{
+  RefPtr<T> ref(aObj);
+  return ref.forget();
+}
+
 #endif