Bug 1396676: Return already_AddRefed from cookie service GetSingleton() methods. r?jdm draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 04 Sep 2017 15:05:10 -0700
changeset 658808 fc41b07272a2aac88174456d88079fd29e432477
parent 658798 9598a628b43208cd7a26c68cb888847429ca4e93
child 729754 332128c092717fd262596a39123fcf11ee3874f1
push id77876
push usermaglione.k@gmail.com
push dateMon, 04 Sep 2017 22:07:05 +0000
reviewersjdm
bugs1396676
milestone57.0a1
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