Bug 1396676: Return already_AddRefed from cookie service GetSingleton() methods. r=jdm
authorKris Maglione <maglione.k@gmail.com>
Mon, 04 Sep 2017 15:05:10 -0700
changeset 379143 a4252412b1ac3b90d0845e7c8af5f8231d4e8bce
parent 379142 1fc9e24649a8971d09ab87824478f6fd58b1c0e8
child 379144 c959327c6b75cd4930a6ea087583c38b805e7524
child 379288 b3e93511d0376bb02821ee56e295e4e5903c3e07
push id32449
push userarchaeopteryx@coole-files.de
push dateWed, 06 Sep 2017 09:33:20 +0000
treeherdermozilla-central@c959327c6b75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
bugs1396676
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
netwerk/cookie/CookieServiceChild.cpp
netwerk/cookie/CookieServiceChild.h
netwerk/cookie/CookieServiceParent.cpp
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
netwerk/protocol/http/HttpChannelChild.cpp
xpcom/base/StaticPtr.h
--- a/netwerk/cookie/CookieServiceChild.cpp
+++ b/netwerk/cookie/CookieServiceChild.cpp
@@ -2,16 +2,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * 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/net/CookieServiceChild.h"
 #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"
 #include "mozilla/SystemGroup.h"
 #include "nsCookie.h"
 #include "nsCookieService.h"
 #include "nsContentUtils.h"
 #include "nsNetCID.h"
@@ -31,26 +32,27 @@ namespace net {
 
 // Pref string constants
 static const char kPrefCookieBehavior[] = "network.cookie.cookieBehavior";
 static const char kPrefThirdPartySession[] =
   "network.cookie.thirdparty.sessionOnly";
 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,
                   nsICookieService,
                   nsIObserver,
                   nsISupportsWeakReference)
 
 CookieServiceChild::CookieServiceChild()
--- a/netwerk/cookie/CookieServiceChild.h
+++ b/netwerk/cookie/CookieServiceChild.h
@@ -35,17 +35,17 @@ public:
   NS_DECL_NSICOOKIESERVICE
   NS_DECL_NSIOBSERVER
 
   typedef nsTArray<RefPtr<nsCookie>> CookiesList;
   typedef nsClassHashtable<nsCookieKey, CookiesList> CookiesMap;
 
   CookieServiceChild();
 
-  static CookieServiceChild* GetSingleton();
+  static already_AddRefed<CookieServiceChild> GetSingleton();
 
   void
   TrackCookieLoad(nsIChannel *aChannel);
 
 protected:
   virtual ~CookieServiceChild();
 
   void SerializeURIs(nsIURI *aHostURI,
--- a/netwerk/cookie/CookieServiceParent.cpp
+++ b/netwerk/cookie/CookieServiceParent.cpp
@@ -65,18 +65,17 @@ namespace net {
 
 CookieServiceParent::CookieServiceParent()
 {
   // Instantiate the cookieservice via the service manager, so it sticks around
   // until shutdown.
   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");
 }
 
 CookieServiceParent::~CookieServiceParent()
 {
 }
 
 void
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set sw=2 ts=8 et tw=80 : */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * 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/Attributes.h"
+#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Likely.h"
 #include "mozilla/Printf.h"
 #include "mozilla/Unused.h"
 
 #include "mozilla/net/CookieServiceChild.h"
 #include "mozilla/net/NeckoCommon.h"
 
@@ -67,17 +68,17 @@ using namespace mozilla::net;
 #define DEFAULT_APP_KEY(baseDomain) \
         nsCookieKey(baseDomain, OriginAttributes())
 
 /******************************************************************************
  * nsCookieService impl:
  * 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
 #define HTTP_ONLY_PREFIX "#HttpOnly_"
 
 #define COOKIES_FILE "cookies.sqlite"
 #define COOKIES_SCHEMA_VERSION 8
 
@@ -633,50 +634,50 @@ DBState::SizeOfIncludingThis(MallocSizeO
   return amount;
 }
 
 /******************************************************************************
  * nsCookieService impl:
  * singleton instance ctor/dtor methods
  ******************************************************************************/
 
-nsICookieService*
+already_AddRefed<nsICookieService>
 nsCookieService::GetXPCOMSingleton()
 {
   if (IsNeckoChild())
     return CookieServiceChild::GetSingleton();
 
   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.
   // We AddRef only once since XPCOM has rules about the ordering of module
   // teardowns - by the time our module destructor is called, it's too late to
   // Release our members (e.g. nsIObserverService and nsIPrefBranch), since GC
   // cycles have already been completed and would result in serious leaks.
   // 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
 nsCookieService::AppClearDataObserverInit()
 {
   nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
   nsCOMPtr<nsIObserver> obs = new AppClearDataObserver();
   observerService->AddObserver(obs, TOPIC_CLEAR_ORIGIN_DATA,
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -210,18 +210,18 @@ class nsCookieService final : public nsI
     NS_DECL_ISUPPORTS
     NS_DECL_NSIOBSERVER
     NS_DECL_NSICOOKIESERVICE
     NS_DECL_NSICOOKIEMANAGER
     NS_DECL_NSICOOKIEMANAGER2
     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
    * been uninstalled.  When an app is uninstalled, we get the cookie service
    * (thus instantiating it, if necessary) and clear all the cookies for that
    * app.
    */
   static void AppClearDataObserverInit();
@@ -325,13 +325,13 @@ class nsCookieService final : public nsI
     uint16_t                      mMaxCookiesPerHost;
     int64_t                       mCookiePurgeAge;
 
     // friends!
     friend class DBListenerErrorHandler;
     friend class ReadCookieDBListener;
     friend class CloseCookieDBListener;
 
-    static nsCookieService*       GetSingleton();
+    static already_AddRefed<nsCookieService> GetSingleton();
     friend class mozilla::net::CookieServiceParent;
 };
 
 #endif // nsCookieService_h__
--- a/netwerk/protocol/http/HttpChannelChild.cpp
+++ b/netwerk/protocol/http/HttpChannelChild.cpp
@@ -198,17 +198,17 @@ HttpChannelChild::HttpChannelChild()
     sSecurityPrefChecked = true;
   }
 #endif
 
   // Ensure that the cookie service is initialized before the first
   // 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()
 {
   LOG(("Destroying HttpChannelChild @%p\n", this));
 
   ReleaseMainThreadOnlyReferences();
 }
--- a/xpcom/base/StaticPtr.h
+++ b/xpcom/base/StaticPtr.h
@@ -269,9 +269,17 @@ RefPtr<T>::RefPtr(const mozilla::StaticR
 
 template<class T> template<class U>
 RefPtr<T>&
 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