Bug 723362 - Make an asynchronous variant of nsCacheEntryDescriptor::Doom, r=hurley

netwerk/cache/nsCacheEntryDescriptor.cpp

@@ -12,10 +12,63 @@
 #include "nsReadableUtils.h"
 #include "nsIOutputStream.h"
 #include "nsCRT.h"
+#include "nsThreadUtils.h"
 
 #define kMinDecompressReadBufLen 1024
 #define kMinCompressWriteBufLen  1024
 
+
+/******************************************************************************
+ * nsAsyncDoomEvent
+ *****************************************************************************/
+
+class nsAsyncDoomEvent : public nsRunnable {
+public:
+    nsAsyncDoomEvent(nsCacheEntryDescriptor *descriptor,
+                     nsICacheListener *listener)
+    {
+        mDescriptor = descriptor;
+        mListener = listener;
+        mThread = do_GetCurrentThread();
+        // We addref the listener here and release it in nsNotifyDoomListener
+        // on the callers thread. If posting of nsNotifyDoomListener event fails
+        // we leak the listener which is better than releasing it on a wrong
+        // thread.
+        NS_IF_ADDREF(mListener);
+    }
+
+    NS_IMETHOD Run()
+    {
+        nsresult status = NS_OK;
+
+        {
+            nsCacheServiceAutoLock lock(LOCK_TELEM(NSASYNCDOOMEVENT_RUN));
+
+            if (mDescriptor->mCacheEntry) {
+                status = nsCacheService::gService->DoomEntry_Internal(
+                             mDescriptor->mCacheEntry, true);
+            } else if (!mDescriptor->mDoomedOnClose) {
+                status = NS_ERROR_NOT_AVAILABLE;
+            }
+        }
+
+        if (mListener) {
+            mThread->Dispatch(new nsNotifyDoomListener(mListener, status),
+                              NS_DISPATCH_NORMAL);
+            // posted event will release the reference on the correct thread
+            mListener = nullptr;
+        }
+
+        return NS_OK;
+    }
+
+private:
+    nsCOMPtr<nsCacheEntryDescriptor> mDescriptor;
+    nsICacheListener                *mListener;
+    nsCOMPtr<nsIThread>              mThread;
+};
+
+
 NS_IMPL_THREADSAFE_ISUPPORTS2(nsCacheEntryDescriptor,
                               nsICacheEntryDescriptor,
                               nsICacheEntryInfo)
@@ -24,7 +77,10 @@ nsCacheEntryDescriptor::nsCacheEntryDescriptor(nsCacheEntry * entry,
                                                nsCacheAccessMode accessGranted)
     : mCacheEntry(entry),
       mAccessGranted(accessGranted),
-      mOutput(nullptr)
+      mOutput(nullptr),
+      mLock("nsCacheEntryDescriptor.mLock"),
+      mAsyncDoomPending(false),
+      mDoomedOnClose(false)
 {
     PR_INIT_CLIST(this);
     NS_ADDREF(nsCacheService::GlobalInstance());  // ensure it lives for the lifetime of the descriptor
@@ -439,6 +495,34 @@ nsCacheEntryDescriptor::DoomAndFailPendingRequests(nsresult status)
 }
 
 
+NS_IMETHODIMP
+nsCacheEntryDescriptor::AsyncDoom(nsICacheListener *listener)
+{
+    bool asyncDoomPending;
+    {
+        mozilla::MutexAutoLock lock(mLock);
+        asyncDoomPending = mAsyncDoomPending;
+        mAsyncDoomPending = true;
+    }
+
+    if (asyncDoomPending) {
+        // AsyncDoom was already called. Notify listener if it is non-null,
+        // otherwise just return success.
+        if (listener) {
+            nsresult rv = NS_DispatchToCurrentThread(
+                new nsNotifyDoomListener(listener, NS_ERROR_NOT_AVAILABLE));
+            if (NS_SUCCEEDED(rv))
+                NS_IF_ADDREF(listener);
+            return rv;
+        }
+        return NS_OK;
+    }
+
+    nsRefPtr<nsIRunnable> event = new nsAsyncDoomEvent(this, listener);
+    return nsCacheService::DispatchToCacheIOThread(event);
+}
+
+
 NS_IMETHODIMP
 nsCacheEntryDescriptor::MarkValid()
 {

netwerk/cache/nsCacheEntryDescriptor.h

@@ -15,6 +15,7 @@
 #include "nsCacheService.h"
 #include "nsIDiskCacheStreamInternal.h"
 #include "zlib.h"
+#include "mozilla/Mutex.h"
 
 /******************************************************************************
 * nsCacheEntryDescriptor
@@ -28,6 +29,8 @@ public:
     NS_DECL_NSICACHEENTRYDESCRIPTOR
     NS_DECL_NSICACHEENTRYINFO
 
+    friend class nsAsyncDoomEvent;
+
     nsCacheEntryDescriptor(nsCacheEntry * entry, nsCacheAccessMode  mode);
     virtual ~nsCacheEntryDescriptor();
     
@@ -40,7 +43,20 @@ public:
      * methods callbacks for nsCacheService
      */
     nsCacheEntry * CacheEntry(void)      { return mCacheEntry; }
-    void           ClearCacheEntry(void) { mCacheEntry = nullptr; }
+    void           ClearCacheEntry(void)
+    {
+      bool asyncDoomPending;
+      {
+        mozilla::MutexAutoLock lock(mLock);
+        asyncDoomPending = mAsyncDoomPending;
+      }
+
+      if (asyncDoomPending && mCacheEntry) {
+        nsCacheService::gService->DoomEntry_Internal(mCacheEntry, true);
+        mDoomedOnClose = true;
+      }
+      mCacheEntry = nullptr;
+    }
 
     nsresult       CloseOutput(void)
     {
@@ -208,6 +224,9 @@ private:
      nsCacheEntry          * mCacheEntry; // we are a child of the entry
      nsCacheAccessMode       mAccessGranted;
      nsIOutputStream       * mOutput;
+     mozilla::Mutex          mLock;
+     bool                    mAsyncDoomPending;
+     bool                    mDoomedOnClose;
 };
 
 

netwerk/cache/nsCacheService.cpp

@@ -993,30 +993,6 @@ private:
     nsCacheRequest *mRequest;
 };
 
-/******************************************************************************
- * nsNotifyDoomListener
- *****************************************************************************/
-
-class nsNotifyDoomListener : public nsRunnable {
-public:
-    nsNotifyDoomListener(nsICacheListener *listener,
-                         nsresult status)
-        : mListener(listener)      // transfers reference
-        , mStatus(status)
-    {}
-
-    NS_IMETHOD Run()
-    {
-        mListener->OnCacheEntryDoomed(mStatus);
-        NS_RELEASE(mListener);
-        return NS_OK;
-    }
-
-private:
-    nsICacheListener *mListener;
-    nsresult          mStatus;
-};
-
 /******************************************************************************
  * nsDoomEvent
  *****************************************************************************/
@@ -2871,27 +2847,35 @@ nsCacheService::ClearDoomList()
 void
 nsCacheService::ClearActiveEntries()
 {
-    mActiveEntries.VisitEntries(DeactivateAndClearEntry, nullptr);
+    nsVoidArray entries;
+
+    // We can't detach descriptors while enumerating hash table since calling
+    // entry->DetachDescriptors() could involve dooming the entry which tries
+    // to remove the entry from the hash table.
+    mActiveEntries.VisitEntries(GetActiveEntries, &entries);
+
+    for (int32_t i = 0 ; i < entries.Count() ; i++) {
+        nsCacheEntry * entry = static_cast<nsCacheEntry *>(entries.ElementAt(i));
+        NS_ASSERTION(entry, "### active entry = nullptr!");
+        // only called from Shutdown() so we don't worry about pending requests
+        gService->ClearPendingRequests(entry);
+        entry->DetachDescriptors();
+        gService->DeactivateEntry(entry);
+    }
+
     mActiveEntries.Shutdown();
 }
 
 
 PLDHashOperator
-nsCacheService::DeactivateAndClearEntry(PLDHashTable *    table,
+nsCacheService::GetActiveEntries(PLDHashTable *    table,
                                  PLDHashEntryHdr * hdr,
                                  uint32_t          number,
                                  void *            arg)
 {
-    nsCacheEntry * entry = ((nsCacheEntryHashTableEntry *)hdr)->cacheEntry;
-    NS_ASSERTION(entry, "### active entry = nullptr!");
-    // only called from Shutdown() so we don't worry about pending requests
-    gService->ClearPendingRequests(entry);
-    entry->DetachDescriptors();
-    
-    entry->MarkInactive();  // so we don't call Remove() while we're enumerating
-    gService->DeactivateEntry(entry);
-    
-    return PL_DHASH_REMOVE; // and continue enumerating
+    static_cast<nsVoidArray *>(arg)->AppendElement(
+        ((nsCacheEntryHashTableEntry *)hdr)->cacheEntry);
+    return PL_DHASH_NEXT;
 }
 
 struct ActiveEntryArgs

netwerk/cache/nsCacheService.h

@@ -12,6 +12,8 @@
 #include "nsCacheSession.h"
 #include "nsCacheDevice.h"
 #include "nsCacheEntry.h"
+#include "nsThreadUtils.h"
+#include "nsICacheListener.h"
 
 #include "prthread.h"
 #include "nsIObserver.h"
@@ -31,6 +33,30 @@ class nsCacheServiceAutoLock;
 class nsITimer;
 
 
+/******************************************************************************
+ * nsNotifyDoomListener
+ *****************************************************************************/
+
+class nsNotifyDoomListener : public nsRunnable {
+public:
+    nsNotifyDoomListener(nsICacheListener *listener,
+                         nsresult status)
+        : mListener(listener)      // transfers reference
+        , mStatus(status)
+    {}
+
+    NS_IMETHOD Run()
+    {
+        mListener->OnCacheEntryDoomed(mStatus);
+        NS_RELEASE(mListener);
+        return NS_OK;
+    }
+
+private:
+    nsICacheListener *mListener;
+    nsresult          mStatus;
+};
+
 /******************************************************************************
  *  nsCacheService
  ******************************************************************************/
@@ -192,6 +218,8 @@ private:
     friend class nsDoomEvent;
     friend class nsDisableOldMaxSmartSizePrefEvent;
     friend class nsDiskCacheMap;
+    friend class nsAsyncDoomEvent;
+    friend class nsCacheEntryDescriptor;
 
     /**
      * Internal Methods
@@ -252,7 +280,7 @@ private:
     void             DoomActiveEntries(DoomCheckFn check);
 
     static
-    PLDHashOperator  DeactivateAndClearEntry(PLDHashTable *    table,
+    PLDHashOperator  GetActiveEntries(PLDHashTable *    table,
                                       PLDHashEntryHdr * hdr,
                                       uint32_t          number,
                                       void *            arg);

netwerk/cache/nsICacheEntryDescriptor.idl

@@ -15,7 +15,7 @@ interface nsIFile;
 interface nsICacheMetaDataVisitor;
 
 
-[scriptable, uuid(49c1a11d-f5d2-4f09-8262-551e64908ada)]
+[scriptable, uuid(90b17d31-46aa-4fb1-a206-473c966cbc18)]
 interface nsICacheEntryDescriptor : nsICacheEntryInfo
 {
     /**
@@ -118,6 +118,13 @@ interface nsICacheEntryDescriptor : nsICacheEntryInfo
     void doom();
     void doomAndFailPendingRequests(in nsresult status);
 
+    /**
+     * Asynchronously doom an entry. Listener will be notified about the status
+     * of the operation. Null may be passed if caller doesn't care about the
+     * result.
+     */
+    void asyncDoom(in nsICacheListener listener);
+
     /**
      * A writer must validate this cache object before any readers are given
      * a descriptor to the object.

netwerk/protocol/ftp/nsFtpConnectionThread.cpp

@@ -1118,7 +1118,7 @@ nsFtpState::S_list() {
 
         // open cache entry for writing, and configure it to receive data.
         if (NS_FAILED(InstallCacheListener())) {
-            mCacheEntry->Doom();
+            mCacheEntry->AsyncDoom(nullptr);
             mCacheEntry = nullptr;
         }
     }
@@ -1181,7 +1181,7 @@ nsFtpState::R_retr() {
         // any cache entry, otherwise we'll have problems reading it later.
         // See bug 122548
         if (mCacheEntry) {
-            (void)mCacheEntry->Doom();
+            (void)mCacheEntry->AsyncDoom(nullptr);
             mCacheEntry = nullptr;
         }
         if (HasPendingCallback())
@@ -2149,7 +2149,7 @@ nsFtpState::CloseWithStatus(nsresult status)
 
     mDataStream = nullptr;
     if (mDoomCache && mCacheEntry)
-        mCacheEntry->Doom();
+        mCacheEntry->AsyncDoom(nullptr);
     mCacheEntry = nullptr;
 
     return nsBaseContentStream::CloseWithStatus(status);

netwerk/protocol/http/nsHttpChannel.cpp

@@ -607,7 +607,7 @@ nsHttpChannel::ContinueHandleAsyncRedirect(nsresult rv)
     // for some reason (the cache entry might be corrupt).
     if (mCacheEntry) {
         if (NS_FAILED(rv))
-            mCacheEntry->Doom();
+            mCacheEntry->AsyncDoom(nullptr);
     }
     CloseCacheEntry(false);
 
@@ -982,7 +982,7 @@ nsHttpChannel::CallOnStartRequest()
 
     // if this channel is for a download, close off access to the cache.
     if (mCacheEntry && mChannelIsForDownload) {
-        mCacheEntry->Doom();
+        mCacheEntry->AsyncDoom(nullptr);
         CloseCacheEntry(false);
     }
 
@@ -1262,7 +1262,7 @@ nsHttpChannel::ProcessResponse()
             LOG(("AsyncProcessRedirection failed [rv=%x]\n", rv));
             // don't cache failed redirect responses.
             if (mCacheEntry)
-                mCacheEntry->Doom();
+                mCacheEntry->AsyncDoom(nullptr);
             if (DoNotRender3xxBody(rv)) {
                 mStatus = rv;
                 DoNotifyListener();
@@ -2181,7 +2181,7 @@ nsHttpChannel::ProcessNotModified()
              "[%s] and [%s]\n",
              lastModifiedCached.get(), lastModified304.get()));
 
-        mCacheEntry->Doom();
+        mCacheEntry->AsyncDoom(nullptr);
         if (mConnectionInfo)
             gHttpHandler->ConnMgr()->
                 PipelineFeedbackInfo(mConnectionInfo,
@@ -2259,7 +2259,7 @@ nsHttpChannel::ProcessFallback(bool *waitingForRedirectCallback)
     // Kill any offline cache entry, and disable offline caching for the
     // fallback.
     if (mOfflineCacheEntry) {
-        mOfflineCacheEntry->Doom();
+        mOfflineCacheEntry->AsyncDoom(nullptr);
         mOfflineCacheEntry = 0;
         mOfflineCacheAccess = 0;
     }
@@ -3570,7 +3570,7 @@ nsHttpChannel::CloseCacheEntry(bool doomOnFailure)
 
     if (doom) {
         LOG(("  dooming cache entry!!"));
-        mCacheEntry->Doom();
+        mCacheEntry->AsyncDoom(nullptr);
     }
 
     mCachedResponseHead = nullptr;
@@ -3591,12 +3591,12 @@ nsHttpChannel::CloseOfflineCacheEntry()
     LOG(("nsHttpChannel::CloseOfflineCacheEntry [this=%p]", this));
 
     if (NS_FAILED(mStatus)) {
-        mOfflineCacheEntry->Doom();
+        mOfflineCacheEntry->AsyncDoom(nullptr);
     }
     else {
         bool succeeded;
         if (NS_SUCCEEDED(GetRequestSucceeded(&succeeded)) && !succeeded)
-            mOfflineCacheEntry->Doom();
+            mOfflineCacheEntry->AsyncDoom(nullptr);
     }
 
     mOfflineCacheEntry = 0;
@@ -4068,7 +4068,7 @@ nsHttpChannel::ContinueProcessRedirectionAfterFallback(nsresult rv)
     if (mCacheEntry && (mCacheAccess & nsICache::ACCESS_WRITE) &&
         NS_SUCCEEDED(mURI->Equals(mRedirectURI, &redirectingBackToSameURI)) &&
         redirectingBackToSameURI)
-            mCacheEntry->Doom();
+            mCacheEntry->AsyncDoom(nullptr);
 
     // move the reference of the old location to the new one if the new
     // one has none.

toolkit/components/telemetry/Histograms.json

@@ -1289,6 +1289,12 @@
     "n_buckets": 50,
     "description": "Time spent waiting on the cache service lock (ms) on the main thread in NSDISKCACHEMAP_REVALIDATION"
   },
+  "CACHE_SERVICE_LOCK_WAIT_MAINTHREAD_NSASYNCDOOMEVENT_RUN": {
+    "kind": "exponential",
+    "high": "10 * 1000",
+    "n_buckets": 50,
+    "description": "Time spent waiting on the cache service lock (ms) on the main thread in NSASYNCDOOMEVENT_RUN"
+  },
   "DNS_LOOKUP_METHOD2": {
     "kind": "enumerated",
     "n_values": 16,