Bug 1245578 - Make the cookie manager shutdown-safe;r?jduell draft
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Fri, 05 Feb 2016 13:52:12 +0100
changeset 329153 d752c8f70fc458671f189f4c864edd88414feb9e
parent 328969 5e024441510f6d2b460e570d0e6d2dee0dc89723
child 513914 e3e83c53a4108afaae706c92cf2ee7aa4bad4d0a
push id10475
push userdteller@mozilla.com
push dateFri, 05 Feb 2016 13:10:53 +0000
reviewersjduell
bugs1245578
milestone47.0a1
Bug 1245578 - Make the cookie manager shutdown-safe;r?jduell The Cookie Service was written in times immemorial, when shutdown was a simple thing. These days, this causes the Cookie Service to sometimes shutdown before some clients of the nsICookieManager can finish their work. This interferes with privacy cleanups and is suspected to cause shutdown hangs/crashes. This patch replaces the shutdown during notification profile-before-change with the following algorithm: 1. Add a shutdown blocker for profileBeforeChange; 2. Upon notification that profileBeforeChange is in progress, inform clients that have setup shutdown blockers upon the cookie service; 3. Once all clients have lifted their shutdown blocker, lift the profileBeforeChange shutdown blocker.
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
netwerk/cookie/nsICookieManager.idl
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -22,16 +22,18 @@
 #include "nsIURL.h"
 #include "nsIChannel.h"
 #include "nsIFile.h"
 #include "nsIObserverService.h"
 #include "nsILineInputStream.h"
 #include "nsIEffectiveTLDService.h"
 #include "nsIIDNService.h"
 #include "mozIThirdPartyUtil.h"
+#include "nsIAsyncShutdown.h"
+#include "nsIWritablePropertyBag2.h"
 
 #include "nsTArray.h"
 #include "nsCOMArray.h"
 #include "nsIMutableArray.h"
 #include "nsArrayEnumerator.h"
 #include "nsEnumeratorUtils.h"
 #include "nsAutoPtr.h"
 #include "nsReadableUtils.h"
@@ -682,16 +684,219 @@ nsCookieService::GetSingleton()
 nsCookieService::AppClearDataObserverInit()
 {
   nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
   nsCOMPtr<nsIObserver> obs = new AppClearDataObserver();
   observerService->AddObserver(obs, TOPIC_CLEAR_ORIGIN_DATA,
                                /* holdsWeak= */ false);
 }
 
+/**
+ * An AsyncShutdown blocker in charge of shutting down the cookie
+ * service.
+ */
+class CookieServiceShutdown final:
+    public nsIAsyncShutdownBlocker,
+    public nsIAsyncShutdownCompletionCallback
+{
+public:
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSIASYNCSHUTDOWNBLOCKER
+  NS_DECL_NSIASYNCSHUTDOWNCOMPLETIONCALLBACK
+
+  explicit CookieServiceShutdown(nsCookieService* aService);
+
+  already_AddRefed<nsIAsyncShutdownClient> GetClient();
+
+private:
+  nsCOMPtr<nsIAsyncShutdownBarrier> mBarrier;
+  nsCOMPtr<nsIAsyncShutdownClient> mParentClient;
+
+  // The owning service.
+  // The cycle is broken in method Done().
+  RefPtr<nsCookieService> mService;
+
+  // The current state, used both internally and for
+  // forensics/debugging purposes.
+  enum State {
+    NOT_STARTED,
+
+    // Execution of `BlockShutdown` in progress
+    // a. `BlockShutdown` is starting.
+    RECEIVED_BLOCK_SHUTDOWN,
+    // b. `BlockShutdown` is complete, waiting for clients.
+    CALLED_WAIT_CLIENTS,
+
+    // Execution of `Done` in progress
+    // a. `Done` is starting.
+    RECEIVED_DONE,
+    // b. Execution of `Done` is complete, connection is closed in the
+    // background.
+
+    DONE,
+  };
+  State mState;
+
+  // As tests may resurrect a dead service, we use a counter to
+  // give the instances of `Shutdown` unique names.
+  uint16_t mCounter;
+  static uint16_t sCounter;
+
+  ~CookieServiceShutdown() {}
+};
+
+uint16_t
+CookieServiceShutdown::sCounter = 1;
+
+NS_IMPL_ISUPPORTS(
+  CookieServiceShutdown
+, nsIAsyncShutdownBlocker
+, nsIAsyncShutdownCompletionCallback
+)
+
+CookieServiceShutdown::CookieServiceShutdown(nsCookieService* service)
+  : mService(service)
+  , mState(NOT_STARTED)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  nsCOMPtr<nsIAsyncShutdownService> asyncShutdownSvc = services::GetAsyncShutdown();
+  MOZ_ASSERT(asyncShutdownSvc);
+
+  if (asyncShutdownSvc) {
+    DebugOnly<nsresult> rv = asyncShutdownSvc->MakeBarrier(
+      NS_LITERAL_STRING("nsCookieService shutdown"),
+      getter_AddRefs(mBarrier)
+    );
+    MOZ_ASSERT(NS_SUCCEEDED(rv));
+  }
+}
+
+already_AddRefed<nsIAsyncShutdownClient>
+CookieServiceShutdown::GetClient()
+{
+  nsCOMPtr<nsIAsyncShutdownClient> client;
+  if (mBarrier) {
+    DebugOnly<nsresult> rv = mBarrier->GetClient(getter_AddRefs(client));
+    MOZ_ASSERT(NS_SUCCEEDED(rv));
+  }
+  return client.forget();
+}
+
+// nsIAsyncShutdownBlocker::GetName
+NS_IMETHODIMP
+CookieServiceShutdown::GetName(nsAString& aName)
+{
+  if (mCounter > 0) {
+    // During tests, we can end up with the Database singleton being resurrected.
+    // Make sure that each instance of Shutdown has a unique name.
+    nsPrintfCString name("nsCookieService Shutdown: Blocking profile-before-change (%x)", this);
+    aName = NS_ConvertUTF8toUTF16(name);
+  } else {
+    aName = NS_LITERAL_STRING("nsCookieService Shutdown: Blocking profile-before-change");
+  }
+  return NS_OK;
+}
+
+// nsIAsyncShutdownBlocker::GetState
+NS_IMETHODIMP
+CookieServiceShutdown::GetState(nsIPropertyBag** aState)
+{
+  nsresult rv;
+  nsCOMPtr<nsIWritablePropertyBag2> bag =
+    do_CreateInstance("@mozilla.org/hash-property-bag;1", &rv);
+  if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+
+  // Put `mState` in field `progress`
+  RefPtr<nsVariant> progress = new nsVariant();
+
+  rv = progress->SetAsUint8(mState);
+  if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+
+  rv = bag->SetPropertyAsInterface(NS_LITERAL_STRING("progress"), progress);
+  if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+
+  // Put `mBarrier`'s state in field `barrier`, if possible
+  if (!mBarrier) {
+    return NS_OK;
+  }
+  nsCOMPtr<nsIPropertyBag> barrierState;
+  rv = mBarrier->GetState(getter_AddRefs(barrierState));
+  if (NS_FAILED(rv)) {
+    return NS_OK;
+  }
+
+  RefPtr<nsVariant> barrier = new nsVariant();
+
+  rv = barrier->SetAsInterface(NS_GET_IID(nsIPropertyBag), barrierState);
+  if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+
+  rv = bag->SetPropertyAsInterface(NS_LITERAL_STRING("Barrier"), barrier);
+  if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+
+  return NS_OK;
+}
+
+// nsIAsyncShutdownBlocker::BlockShutdown
+//
+// Step 1 in shutdown, called during profile-before-change.
+// As a `nsIAsyncShutdownBarrier`, we now need to wait until all clients
+// of `this` barrier have completed their own shutdown.
+//
+// See `Done()` for step 2.
+NS_IMETHODIMP
+CookieServiceShutdown::BlockShutdown(nsIAsyncShutdownClient* aParentClient)
+{
+  mParentClient = aParentClient;
+  mState = RECEIVED_BLOCK_SHUTDOWN;
+
+  if (NS_WARN_IF(!mBarrier)) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
+  // Wait until all clients have removed their blockers, then proceed
+  // with own shutdown.
+  DebugOnly<nsresult> rv = mBarrier->Wait(this);
+  MOZ_ASSERT(NS_SUCCEEDED(rv));
+
+  mState = CALLED_WAIT_CLIENTS;
+  return NS_OK;
+}
+
+
+// nsIAsyncShutdownCompletionCallback::Done
+//
+// Step 2 and last step in shutdown, called once all clients have
+// removed their blockers.  We may now close the database and remove
+// the blocker.
+NS_IMETHODIMP
+CookieServiceShutdown::Done()
+{
+  mState = RECEIVED_DONE;
+
+  // At this stage, any use of this database is forbidden. Note,
+  // however, that the database could be resurrected.  This can happen
+  // in particular during tests.
+  mService->CloseDBStates();
+
+  nsresult rv;
+  if (mParentClient) {
+    // mParentClient may be nullptr in tests
+    rv = mParentClient->RemoveBlocker(this);
+    if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+  }
+
+  // Cleanup.
+  nsCOMPtr<nsIAsyncShutdownBarrier> barrier = mBarrier.forget();
+  nsCOMPtr<nsIAsyncShutdownClient> parentClient = mParentClient.forget();
+  RefPtr<nsCookieService> service = mService.forget();
+
+  mState = DONE;
+  return NS_OK;
+}
+
 /******************************************************************************
  * nsCookieService impl:
  * public methods
  ******************************************************************************/
 
 NS_IMPL_ISUPPORTS(nsCookieService,
                   nsICookieService,
                   nsICookieManager,
@@ -702,17 +907,38 @@ NS_IMPL_ISUPPORTS(nsCookieService,
 
 nsCookieService::nsCookieService()
  : mDBState(nullptr)
  , mCookieBehavior(nsICookieService::BEHAVIOR_ACCEPT)
  , mThirdPartySession(false)
  , mMaxNumberOfCookies(kMaxNumberOfCookies)
  , mMaxCookiesPerHost(kMaxCookiesPerHost)
  , mCookiePurgeAge(kCookiePurgeAge)
+ , mShutdown(new CookieServiceShutdown(this))
 {
+  // Prepare async shutdown
+  nsCOMPtr<nsIAsyncShutdownClient> shutdownPhase;
+  nsCOMPtr<nsIAsyncShutdownService> asyncShutdownSvc = services::GetAsyncShutdown();
+  MOZ_ASSERT(asyncShutdownSvc);
+
+  if (asyncShutdownSvc) {
+    DebugOnly<nsresult> rv = asyncShutdownSvc->
+      GetProfileBeforeChange(getter_AddRefs(shutdownPhase));
+    MOZ_ASSERT(NS_SUCCEEDED(rv));
+    MOZ_ASSERT(shutdownPhase);
+  }
+
+  if (shutdownPhase) {
+    DebugOnly<nsresult> rv = shutdownPhase->AddBlocker(
+      static_cast<nsIAsyncShutdownBlocker*>(mShutdown.get()),
+      NS_LITERAL_STRING(__FILE__),
+      __LINE__,
+      NS_LITERAL_STRING(""));
+    MOZ_ASSERT(NS_SUCCEEDED(rv));
+  }
 }
 
 nsresult
 nsCookieService::Init()
 {
   nsresult rv;
   mTLDService = do_GetService(NS_EFFECTIVETLDSERVICE_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -739,17 +965,16 @@ nsCookieService::Init()
 
   // Init our default, and possibly private DBStates.
   InitDBStates();
 
   RegisterWeakMemoryReporter(this);
 
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   NS_ENSURE_STATE(os);
-  os->AddObserver(this, "profile-before-change", true);
   os->AddObserver(this, "profile-do-change", true);
   os->AddObserver(this, "last-pb-context-exited", true);
 
   mPermissionService = do_GetService(NS_COOKIEPERMISSION_CONTRACTID);
   if (!mPermissionService) {
     NS_WARNING("nsICookiePermission implementation not available - some features won't work!");
     COOKIE_LOGSTRING(LogLevel::Warning, ("Init(): nsICookiePermission implementation not available"));
   }
@@ -1822,38 +2047,28 @@ nsCookieService::RebuildCorruptDB(DBStat
   NS_ASSERT_SUCCESS(rv);
   nsCOMPtr<mozIStoragePendingStatement> handle;
   rv = stmt->ExecuteAsync(aDBState->insertListener, getter_AddRefs(handle));
   NS_ASSERT_SUCCESS(rv);
 }
 
 nsCookieService::~nsCookieService()
 {
-  CloseDBStates();
-
   UnregisterWeakMemoryReporter(this);
 
   gCookieService = nullptr;
 }
 
 NS_IMETHODIMP
 nsCookieService::Observe(nsISupports     *aSubject,
                          const char      *aTopic,
                          const char16_t *aData)
 {
   // check the topic
-  if (!strcmp(aTopic, "profile-before-change")) {
-    // The profile is about to change,
-    // or is going away because the application is shutting down.
-
-    // Close the default DB connection and null out our DBStates before
-    // changing.
-    CloseDBStates();
-
-  } else if (!strcmp(aTopic, "profile-do-change")) {
+  if (!strcmp(aTopic, "profile-do-change")) {
     NS_ASSERTION(!mDefaultDBState, "shouldn't have a default DBState");
     NS_ASSERTION(!mPrivateDBState, "shouldn't have a private DBState");
 
     // the profile has already changed; init the db from the new location.
     // if we are in the private browsing state, however, we do not want to read
     // data into it - we should instead put it into the default state, so it's
     // ready for us if and when we switch back to it.
     InitDBStates();
@@ -4637,8 +4852,16 @@ NS_IMETHODIMP
 nsCookieService::CollectReports(nsIHandleReportCallback* aHandleReport,
                                 nsISupports* aData, bool aAnonymize)
 {
   return MOZ_COLLECT_REPORT(
     "explicit/cookie-service", KIND_HEAP, UNITS_BYTES,
     SizeOfIncludingThis(CookieServiceMallocSizeOf),
     "Memory used by the cookie service.");
 }
+
+NS_IMETHODIMP
+nsCookieService::GetShutdownClient(nsIAsyncShutdownClient** aClient)
+{
+  nsCOMPtr<nsIAsyncShutdownClient> client = mShutdown->GetClient();
+  NS_IF_ADDREF(*aClient = client);
+  return NS_OK;
+}
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -6,16 +6,17 @@
 #ifndef nsCookieService_h__
 #define nsCookieService_h__
 
 #include "nsICookieService.h"
 #include "nsICookieManager.h"
 #include "nsICookieManager2.h"
 #include "nsIObserver.h"
 #include "nsWeakReference.h"
+#include "nsIAsyncShutdown.h"
 
 #include "nsCookie.h"
 #include "nsString.h"
 #include "nsAutoPtr.h"
 #include "nsHashKeys.h"
 #include "nsIMemoryReporter.h"
 #include "nsTHashtable.h"
 #include "mozIStorageStatement.h"
@@ -351,14 +352,17 @@ class nsCookieService final : public nsI
     uint16_t                      mMaxNumberOfCookies;
     uint16_t                      mMaxCookiesPerHost;
     int64_t                       mCookiePurgeAge;
 
     // friends!
     friend class DBListenerErrorHandler;
     friend class ReadCookieDBListener;
     friend class CloseCookieDBListener;
+    friend class CookieServiceShutdown;
 
     static nsCookieService*       GetSingleton();
     friend class mozilla::net::CookieServiceParent;
+
+    RefPtr<class CookieServiceShutdown> mShutdown;
 };
 
 #endif // nsCookieService_h__
--- a/netwerk/cookie/nsICookieManager.idl
+++ b/netwerk/cookie/nsICookieManager.idl
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /* 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 "nsISupports.idl"
 
 interface nsISimpleEnumerator;
+interface nsIAsyncShutdownClient;
 
 /** 
  * An optional interface for accessing or removing the cookies
  * that are in the cookie list
  */
 
 [scriptable, uuid(AAAB6710-0F2C-11d5-A53B-0010A401EB10)]
 interface nsICookieManager : nsISupports
@@ -41,9 +42,15 @@ interface nsICookieManager : nsISupports
    * @param aPath The path for which the cookie was set
    * @param aBlocked Indicates if cookies from this host should be permanently blocked
    *
    */
   void remove(in AUTF8String aHost,
               in ACString    aName,
               in AUTF8String aPath,
               in boolean     aBlocked);
+
+  /**
+   * Hook for clients who need to perform actions during/by the end of
+   * the shutdown of the manager.
+   */
+  readonly attribute nsIAsyncShutdownClient shutdownClient;
 };