Bug 547031 - Improve async error handling in cookies. Part 5: Fix observers to deal with reentrancy. r=sdwilsh, a=betaN+
authorDan Witte <dwitte@mozilla.com>
Fri, 12 Nov 2010 09:32:35 -0800
changeset 57409 8d5d684341711147d795ff967de9ebab128774a8
parent 57408 3e8a89a8be5651029c7ff5175654690cc137022e
child 57410 97c3302f124c8ea7e3519ca31d988de1a5c7dee3
push idunknown
push userunknown
push dateunknown
reviewerssdwilsh, betaN
bugs547031
milestone2.0b8pre
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 547031 - Improve async error handling in cookies. Part 5: Fix observers to deal with reentrancy. r=sdwilsh, a=betaN+
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
netwerk/cookie/nsICookieService.idl
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -1386,23 +1386,23 @@ nsCookieService::NotifyRejected(nsIURI *
 void
 nsCookieService::NotifyChanged(nsISupports     *aSubject,
                                const PRUnichar *aData)
 {
   if (mObserverService)
     mObserverService->NotifyObservers(aSubject, "cookie-changed", aData);
 }
 
-void
-nsCookieService::NotifyPurged(nsICookie2* aCookie)
+already_AddRefed<nsIArray>
+nsCookieService::CreatePurgeList(nsICookie2* aCookie)
 {
   nsCOMPtr<nsIMutableArray> removedList =
     do_CreateInstance(NS_ARRAY_CONTRACTID);
   removedList->AppendElement(aCookie, PR_FALSE);
-  NotifyChanged(removedList, NS_LITERAL_STRING("batch-deleted").get());
+  return removedList.forget();
 }
 
 /******************************************************************************
  * nsCookieService:
  * pref observer impl
  ******************************************************************************/
 
 void
@@ -1545,24 +1545,24 @@ nsCookieService::Remove(const nsACString
   nsresult rv = NormalizeHost(host);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCAutoString baseDomain;
   rv = GetBaseDomainFromHost(host, baseDomain);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsListIter matchIter;
+  nsRefPtr<nsCookie> cookie;
   if (FindCookie(baseDomain,
                  host,
                  PromiseFlatCString(aName),
                  PromiseFlatCString(aPath),
                  matchIter)) {
-    nsRefPtr<nsCookie> cookie = matchIter.Cookie();
+    cookie = matchIter.Cookie();
     RemoveCookieFromList(matchIter);
-    NotifyChanged(cookie, NS_LITERAL_STRING("deleted").get());
   }
 
   // check if we need to add the host to the permissions blacklist.
   if (aBlocked && mPermissionService) {
     // strip off the domain dot, if necessary
     if (!host.IsEmpty() && host.First() == '.')
       host.Cut(0, 1);
 
@@ -1570,16 +1570,21 @@ nsCookieService::Remove(const nsACString
 
     nsCOMPtr<nsIURI> uri;
     NS_NewURI(getter_AddRefs(uri), host);
 
     if (uri)
       mPermissionService->SetAccess(uri, nsICookiePermission::ACCESS_DENY);
   }
 
+  if (cookie) {
+    // Everything's done. Notify observers.
+    NotifyChanged(cookie, NS_LITERAL_STRING("deleted").get());
+  }
+
   return NS_OK;
 }
 
 /******************************************************************************
  * nsCookieService impl:
  * private file I/O functions
  ******************************************************************************/
 
@@ -1701,20 +1706,20 @@ nsCookieService::AsyncReadComplete()
 
   mDefaultDBState->stmtReadDomain = nsnull;
   mDefaultDBState->pendingRead = nsnull;
   mDefaultDBState->readListener = nsnull;
   mDefaultDBState->syncConn = nsnull;
   mDefaultDBState->hostArray.Clear();
   mDefaultDBState->readSet.Clear();
 
-  mObserverService->NotifyObservers(nsnull, "cookie-db-read", nsnull);
-
   COOKIE_LOGSTRING(PR_LOG_DEBUG, ("Read(): %ld cookies read",
                                   mDefaultDBState->cookieCount));
+
+  mObserverService->NotifyObservers(nsnull, "cookie-db-read", nsnull);
 }
 
 void
 nsCookieService::CancelAsyncRead(PRBool aPurgeReadSet)
 {
   // We may be in the private browsing DB state, with a pending read on the
   // default DB state. (This would occur if we started up in private browsing
   // mode.) As long as we do all our operations on the default state, we're OK.
@@ -1860,18 +1865,16 @@ nsCookieService::EnsureReadComplete()
     nsCookie* newCookie = GetCookieFromRow(stmt);
     AddCookieToList(baseDomain, newCookie, mDefaultDBState, NULL, PR_FALSE);
     ++readCount;
   }
 
   mDefaultDBState->syncConn = nsnull;
   mDefaultDBState->readSet.Clear();
 
-  mObserverService->NotifyObservers(nsnull, "cookie-db-read", nsnull);
-
   COOKIE_LOGSTRING(PR_LOG_DEBUG,
     ("EnsureReadComplete(): %ld cookies read", readCount));
 }
 
 NS_IMETHODIMP
 nsCookieService::ImportCookies(nsIFile *aCookieFile)
 {
   // Make sure we're in the default DB state. We don't want people importing
@@ -1884,16 +1887,19 @@ nsCookieService::ImportCookies(nsIFile *
   nsresult rv;
   nsCOMPtr<nsIInputStream> fileInputStream;
   rv = NS_NewLocalFileInputStream(getter_AddRefs(fileInputStream), aCookieFile);
   if (NS_FAILED(rv)) return rv;
 
   nsCOMPtr<nsILineInputStream> lineInputStream = do_QueryInterface(fileInputStream, &rv);
   if (NS_FAILED(rv)) return rv;
 
+  // First, ensure we've read in everything from the database, if we have one.
+  EnsureReadComplete();
+
   static const char kTrue[] = "TRUE";
 
   nsCAutoString buffer, baseDomain;
   PRBool isMore = PR_TRUE;
   PRInt32 hostIndex, isDomainIndex, pathIndex, secureIndex, expiresIndex, nameIndex, cookieIndex;
   nsASingleFragmentCString::char_iterator iter;
   PRInt32 numInts;
   PRInt64 expires;
@@ -1924,19 +1930,16 @@ nsCookieService::ImportCookies(nsIFile *
    * in a comment, so they don't expose HttpOnly cookies to JS.
    *
    * The format for HttpOnly cookies is
    *
    * #HttpOnly_host \t isDomain \t path \t secure \t expires \t name \t cookie
    *
    */
 
-  // First, ensure we've read in everything from the database, if we have one.
-  EnsureReadComplete();
-
   // We will likely be adding a bunch of cookies to the DB, so we use async
   // batching with storage to make this super fast.
   nsCOMPtr<mozIStorageBindingParamsArray> paramsArray;
   if (originalCookieCount == 0 && mDefaultDBState->dbConn) {
     mDefaultDBState->stmtInsert->NewBindingParamsArray(getter_AddRefs(paramsArray));
   }
 
   while (isMore && NS_SUCCEEDED(lineInputStream->ReadLine(buffer, &isMore))) {
@@ -2379,36 +2382,38 @@ nsCookieService::AddInternal(const nsCSt
     return;
   }
 
   nsListIter matchIter;
   PRBool foundCookie = FindCookie(aBaseDomain, aCookie->Host(),
     aCookie->Name(), aCookie->Path(), matchIter);
 
   nsRefPtr<nsCookie> oldCookie;
+  nsCOMPtr<nsIArray> purgedList;
   if (foundCookie) {
     oldCookie = matchIter.Cookie();
 
     // Check if the old cookie is stale (i.e. has already expired). If so, we
     // need to be careful about the semantics of removing it and adding the new
     // cookie: we want the behavior wrt adding the new cookie to be the same as
     // if it didn't exist, but we still want to fire a removal notification.
     if (oldCookie->Expiry() <= currentTime) {
       if (aCookie->Expiry() <= currentTime) {
         // The new cookie has expired and the old one is stale. Nothing to do.
         COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
           "cookie has already expired");
         return;
       }
 
-      // Remove the stale cookie and notify.
+      // Remove the stale cookie. We save notification for later, once all list
+      // modifications are complete.
       RemoveCookieFromList(matchIter);
       COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
         "stale cookie was purged");
-      NotifyPurged(oldCookie);
+      purgedList = CreatePurgeList(oldCookie);
 
       // We've done all we need to wrt removing and notifying the stale cookie.
       // From here on out, we pretend pretend it didn't exist, so that we
       // preserve expected notification semantics when adding the new cookie.
       foundCookie = PR_FALSE;
 
     } else {
       // If the old cookie is httponly, make sure we're not coming from script.
@@ -2447,40 +2452,46 @@ nsCookieService::AddInternal(const nsCSt
     if (entry && entry->GetCookies().Length() >= mMaxCookiesPerHost) {
       nsListIter iter;
       FindStaleCookie(entry, currentTime, iter);
       oldCookie = iter.Cookie();
 
       // remove the oldest cookie from the domain
       RemoveCookieFromList(iter);
       COOKIE_LOGEVICTED(oldCookie, "Too many cookies for this domain");
-      NotifyPurged(oldCookie);
+      purgedList = CreatePurgeList(oldCookie);
 
     } else if (mDBState->cookieCount >= ADD_TEN_PERCENT(mMaxNumberOfCookies)) {
       PRInt64 maxAge = aCurrentTimeInUsec - mDBState->cookieOldestTime;
       PRInt64 purgeAge = ADD_TEN_PERCENT(mCookiePurgeAge);
       if (maxAge >= purgeAge) {
         // we're over both size and age limits by 10%; time to purge the table!
         // do this by:
         // 1) removing expired cookies;
         // 2) evicting the balance of old cookies until we reach the size limit.
         // note that the cookieOldestTime indicator can be pessimistic - if it's
         // older than the actual oldest cookie, we'll just purge more eagerly.
-        PurgeCookies(aCurrentTimeInUsec);
+        purgedList = PurgeCookies(aCurrentTimeInUsec);
       }
     }
   }
 
   // Add the cookie to the db. We do not supply a params array for batching
   // because this might result in removals and additions being out of order.
   AddCookieToList(aBaseDomain, aCookie, mDBState, NULL);
+  COOKIE_LOGSUCCESS(SET_COOKIE, aHostURI, aCookieHeader, aCookie, foundCookie);
+
+  // Now that list mutations are complete, notify observers. We do it here
+  // because observers may themselves attempt to mutate the list.
+  if (purgedList) {
+    NotifyChanged(purgedList, NS_LITERAL_STRING("batch-deleted").get());
+  }
+
   NotifyChanged(aCookie, foundCookie ? NS_LITERAL_STRING("changed").get()
                                      : NS_LITERAL_STRING("added").get());
-
-  COOKIE_LOGSUCCESS(SET_COOKIE, aHostURI, aCookieHeader, aCookie, foundCookie);
 }
 
 /******************************************************************************
  * nsCookieService impl:
  * private cookie header parsing functions
  ******************************************************************************/
 
 // The following comment block elucidates the function of ParseAttributes.
@@ -3153,43 +3164,41 @@ purgeCookiesCallback(nsCookieEntry *aEnt
 
       ++i;
     }
   }
   return PL_DHASH_NEXT;
 }
 
 // purges expired and old cookies in a batch operation.
-void
+already_AddRefed<nsIArray>
 nsCookieService::PurgeCookies(PRInt64 aCurrentTimeInUsec)
 {
   NS_ASSERTION(mDBState->hostTable.Count() > 0, "table is empty");
+  EnsureReadComplete();
+
 #ifdef PR_LOGGING
   PRUint32 initialCookieCount = mDBState->cookieCount;
   COOKIE_LOGSTRING(PR_LOG_DEBUG,
     ("PurgeCookies(): beginning purge with %ld cookies and %lld oldest age",
      mDBState->cookieCount, aCurrentTimeInUsec - mDBState->cookieOldestTime));
 #endif
 
   nsAutoTArray<nsListIter, kMaxNumberOfCookies> purgeList;
 
   nsCOMPtr<nsIMutableArray> removedList = do_CreateInstance(NS_ARRAY_CONTRACTID);
-  if (!removedList)
-    return;
 
   // Create a params array to batch the removals. This is OK here because
   // all the removals are in order, and there are no interleaved additions.
   mozIStorageAsyncStatement *stmt = mDBState->stmtDelete;
   nsCOMPtr<mozIStorageBindingParamsArray> paramsArray;
   if (mDBState->dbConn) {
     stmt->NewBindingParamsArray(getter_AddRefs(paramsArray));
   }
 
-  EnsureReadComplete();
-
   nsPurgeData data(aCurrentTimeInUsec / PR_USEC_PER_SEC,
     aCurrentTimeInUsec - mCookiePurgeAge, purgeList, removedList, paramsArray);
   mDBState->hostTable.EnumerateEntries(purgeCookiesCallback, &data);
 
 #ifdef PR_LOGGING
   PRUint32 postExpiryCookieCount = mDBState->cookieCount;
 #endif
 
@@ -3227,28 +3236,27 @@ nsCookieService::PurgeCookies(PRInt64 aC
       nsresult rv = stmt->BindParameters(paramsArray);
       NS_ASSERT_SUCCESS(rv);
       nsCOMPtr<mozIStoragePendingStatement> handle;
       rv = stmt->ExecuteAsync(mDBState->removeListener, getter_AddRefs(handle));
       NS_ASSERT_SUCCESS(rv);
     }
   }
 
-  // take all the cookies in the removed list, and notify about them in one batch
-  NotifyChanged(removedList, NS_LITERAL_STRING("batch-deleted").get());
-
   // reset the oldest time indicator
   mDBState->cookieOldestTime = data.oldestTime;
 
   COOKIE_LOGSTRING(PR_LOG_DEBUG,
     ("PurgeCookies(): %ld expired; %ld purged; %ld remain; %lld oldest age",
      initialCookieCount - postExpiryCookieCount,
      postExpiryCookieCount - mDBState->cookieCount,
      mDBState->cookieCount,
      aCurrentTimeInUsec - mDBState->cookieOldestTime));
+
+  return removedList.forget();
 }
 
 // find whether a given cookie has been previously set. this is provided by the
 // nsICookieManager2 interface.
 NS_IMETHODIMP
 nsCookieService::CookieExists(nsICookie2 *aCookie,
                               PRBool     *aFoundCookie)
 {
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -63,16 +63,17 @@
 
 class nsICookiePermission;
 class nsIEffectiveTLDService;
 class nsIIDNService;
 class nsIPrefBranch;
 class nsIObserverService;
 class nsIURI;
 class nsIChannel;
+class nsIArray;
 class mozIStorageService;
 class mozIThirdPartyUtil;
 class ReadCookieDBListener;
 
 struct nsCookieAttributes;
 struct nsListIter;
 struct nsEnumerationData;
 
@@ -262,22 +263,23 @@ class nsCookieService : public nsICookie
     static PRBool                 GetTokenValue(nsASingleFragmentCString::const_char_iterator &aIter, nsASingleFragmentCString::const_char_iterator &aEndIter, nsDependentCSubstring &aTokenString, nsDependentCSubstring &aTokenValue, PRBool &aEqualsFound);
     static PRBool                 ParseAttributes(nsDependentCString &aCookieHeader, nsCookieAttributes &aCookie);
     bool                          RequireThirdPartyCheck();
     CookieStatus                  CheckPrefs(nsIURI *aHostURI, bool aIsForeign, const nsCString &aBaseDomain, PRBool aRequireHostMatch, const char *aCookieHeader);
     PRBool                        CheckDomain(nsCookieAttributes &aCookie, nsIURI *aHostURI, const nsCString &aBaseDomain, PRBool aRequireHostMatch);
     static PRBool                 CheckPath(nsCookieAttributes &aCookie, nsIURI *aHostURI);
     static PRBool                 GetExpiry(nsCookieAttributes &aCookie, PRInt64 aServerTime, PRInt64 aCurrentTime);
     void                          RemoveAllFromMemory();
-    void                          PurgeCookies(PRInt64 aCurrentTimeInUsec);
+    already_AddRefed<nsIArray>    PurgeCookies(PRInt64 aCurrentTimeInUsec);
     PRBool                        FindCookie(const nsCString& aBaseDomain, const nsAFlatCString &aHost, const nsAFlatCString &aName, const nsAFlatCString &aPath, nsListIter &aIter);
     static void                   FindStaleCookie(nsCookieEntry *aEntry, PRInt64 aCurrentTime, nsListIter &aIter);
     void                          NotifyRejected(nsIURI *aHostURI);
     void                          NotifyChanged(nsISupports *aSubject, const PRUnichar *aData);
     void                          NotifyPurged(nsICookie2* aCookie);
+    already_AddRefed<nsIArray>    CreatePurgeList(nsICookie2* aCookie);
 
   protected:
     // cached members.
     nsCOMPtr<nsIObserverService>     mObserverService;
     nsCOMPtr<nsICookiePermission>    mPermissionService;
     nsCOMPtr<mozIThirdPartyUtil>     mThirdPartyUtil;
     nsCOMPtr<nsIEffectiveTLDService> mTLDService;
     nsCOMPtr<nsIIDNService>          mIDNService;
--- a/netwerk/cookie/nsICookieService.idl
+++ b/netwerk/cookie/nsICookieService.idl
@@ -43,18 +43,27 @@ interface nsIChannel;
 
 /**
  * nsICookieService
  *
  * Provides methods for setting and getting cookies in the context of a
  * page load.  See nsICookieManager for methods to manipulate the cookie
  * database directly.  This separation of interface is mainly historical.
  *
- * This service broadcasts the following notifications when the cookie
- * list is changed, or a cookie is rejected:
+ * This service broadcasts the notifications detailed below when the cookie
+ * list is changed, or a cookie is rejected.
+ *
+ * NOTE: observers of these notifications *must* not attempt to change profile
+ *       or switch into or out of private browsing mode from within the
+ *       observer. Doing so will cause undefined behavior. Mutating the cookie
+ *       list (e.g. by calling methods on nsICookieService and friends) is
+ *       allowed, but beware that there may be pending notifications you haven't
+ *       seen yet -- for instance, a "batch-deleted" notification will likely be
+ *       immediately followed by "added". You may check the state of the cookie
+ *       list to determine if this is the case.
  *
  * topic  : "cookie-changed"
  *          broadcast whenever the cookie list changes in some way. see
  *          explanation of data strings below.
  * subject: see below.
  * data   : "deleted"
  *          a cookie was deleted. the subject is an nsICookie2 representing
  *          the deleted cookie.