Bug 938186 - HTTP cache v2: introduce DISALLOW_SYNC_CALLBACK for opening cache entries, r=michal
authorHonza Bambas <honzab.moz@firemni.cz>
Mon, 09 Jun 2014 20:59:08 +0200
changeset 209711 400d92d915f469f43fb2fa8a0d8e2d28979807e1
parent 209710 fa494104700e246c5b16e08aa05ce8c232b7a682
child 209712 4b7010671a27100ebe826c55565dbaf7af86de67
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal
bugs938186
milestone32.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 938186 - HTTP cache v2: introduce DISALLOW_SYNC_CALLBACK for opening cache entries, r=michal
netwerk/cache2/CacheEntry.cpp
netwerk/cache2/CacheEntry.h
netwerk/cache2/nsICacheStorage.idl
netwerk/test/unit/test_cache2-01f-basic-async.js
netwerk/test/unit/test_cache2-24-force-async.js
netwerk/test/unit/xpcshell.ini
--- a/netwerk/cache2/CacheEntry.cpp
+++ b/netwerk/cache2/CacheEntry.cpp
@@ -71,24 +71,25 @@ CacheEntryHandle::~CacheEntryHandle()
 
   MOZ_COUNT_DTOR(CacheEntryHandle);
 }
 
 // CacheEntry::Callback
 
 CacheEntry::Callback::Callback(CacheEntry* aEntry,
                                nsICacheEntryOpenCallback *aCallback,
-                               bool aReadOnly, bool aCheckOnAnyThread)
+                               bool aReadOnly, bool aCheckOnAnyThread, bool aForceAsync)
 : mEntry(aEntry)
 , mCallback(aCallback)
 , mTargetThread(do_GetCurrentThread())
 , mReadOnly(aReadOnly)
 , mCheckOnAnyThread(aCheckOnAnyThread)
 , mRecheckAfterWrite(false)
 , mNotWanted(false)
+, mForceAsync(aForceAsync)
 {
   MOZ_COUNT_CTOR(CacheEntry::Callback);
 
   // The counter may go from zero to non-null only under the service lock
   // but here we expect it to be already positive.
   MOZ_ASSERT(mEntry->HandlesCount());
   mEntry->AddHandleRef();
 }
@@ -96,16 +97,17 @@ CacheEntry::Callback::Callback(CacheEntr
 CacheEntry::Callback::Callback(CacheEntry::Callback const &aThat)
 : mEntry(aThat.mEntry)
 , mCallback(aThat.mCallback)
 , mTargetThread(aThat.mTargetThread)
 , mReadOnly(aThat.mReadOnly)
 , mCheckOnAnyThread(aThat.mCheckOnAnyThread)
 , mRecheckAfterWrite(aThat.mRecheckAfterWrite)
 , mNotWanted(aThat.mNotWanted)
+, mForceAsync(aThat.mForceAsync)
 {
   MOZ_COUNT_CTOR(CacheEntry::Callback);
 
   // The counter may go from zero to non-null only under the service lock
   // but here we expect it to be already positive.
   MOZ_ASSERT(mEntry->HandlesCount());
   mEntry->AddHandleRef();
 }
@@ -126,18 +128,29 @@ void CacheEntry::Callback::ExchangeEntry
   // The counter may go from zero to non-null only under the service lock
   // but here we expect it to be already positive.
   MOZ_ASSERT(aEntry->HandlesCount());
   aEntry->AddHandleRef();
   mEntry->ReleaseHandleRef();
   mEntry = aEntry;
 }
 
-nsresult CacheEntry::Callback::OnCheckThread(bool *aOnCheckThread) const
+nsresult CacheEntry::Callback::OnCheckThread(bool *aOnCheckThread)
 {
+  if (mForceAsync) {
+    // Drop the flag now.  First time we must claim we are not on the proper thread
+    // what will simply force a post.  But, the post does the check again and that
+    // time we must already tell the true we are on the proper thread otherwise we
+    // just loop indefinitely.  Also, we need to post only once the first
+    // InvokeCallback for this callback.
+    mForceAsync = false;
+    *aOnCheckThread = false;
+    return NS_OK;
+  }
+
   if (!mCheckOnAnyThread) {
     // Check we are on the target
     return mTargetThread->IsOnCurrentThread(aOnCheckThread);
   }
 
   // We can invoke check anywhere
   *aOnCheckThread = true;
   return NS_OK;
@@ -165,16 +178,17 @@ CacheEntry::CacheEntry(const nsACString&
 , mFileStatus(NS_ERROR_NOT_INITIALIZED)
 , mURI(aURI)
 , mEnhanceID(aEnhanceID)
 , mStorageID(aStorageID)
 , mUseDisk(aUseDisk)
 , mIsDoomed(false)
 , mSecurityInfoLoaded(false)
 , mPreventCallbacks(false)
+, mDispatchingCallbacks(false)
 , mHasData(false)
 , mState(NOTLOADED)
 , mRegistration(NEVERREGISTERED)
 , mWriter(nullptr)
 , mPredictedDataSize(0)
 , mReleaseThread(NS_GetCurrentThread())
 {
   MOZ_COUNT_CTOR(CacheEntry);
@@ -263,21 +277,22 @@ void CacheEntry::AsyncOpen(nsICacheEntry
   LOG(("CacheEntry::AsyncOpen [this=%p, state=%s, flags=%d, callback=%p]",
     this, StateString(mState), aFlags, aCallback));
 
   bool readonly = aFlags & nsICacheStorage::OPEN_READONLY;
   bool bypassIfBusy = aFlags & nsICacheStorage::OPEN_BYPASS_IF_BUSY;
   bool truncate = aFlags & nsICacheStorage::OPEN_TRUNCATE;
   bool priority = aFlags & nsICacheStorage::OPEN_PRIORITY;
   bool multithread = aFlags & nsICacheStorage::CHECK_MULTITHREADED;
+  bool async = aFlags & nsICacheStorage::FORCE_ASYNC_CALLBACK;
 
   MOZ_ASSERT(!readonly || !truncate, "Bad flags combination");
   MOZ_ASSERT(!(truncate && mState > LOADING), "Must not call truncate on already loaded entry");
 
-  Callback callback(this, aCallback, readonly, multithread);
+  Callback callback(this, aCallback, readonly, multithread, async);
 
   mozilla::MutexAutoLock lock(mLock);
 
   RememberCallback(callback, bypassIfBusy);
 
   // Load() opens the lock
   if (Load(truncate, priority)) {
     // Loading is in progress...
@@ -504,19 +519,24 @@ void CacheEntry::RememberCallback(Callba
     aCallback.mNotWanted = true;
     InvokeAvailableCallback(aCallback);
     return;
   }
 
   mCallbacks.AppendElement(aCallback);
 }
 
-void CacheEntry::InvokeCallbacksLock()
+void CacheEntry::InvokeDispatchedCallbacks()
 {
+  LOG(("CacheEntry::InvokeDispatchedCallbacks [this=%p]", this));
+
   mozilla::MutexAutoLock lock(mLock);
+
+  MOZ_ASSERT(mDispatchingCallbacks);
+  mDispatchingCallbacks = false;
   InvokeCallbacks();
 }
 
 void CacheEntry::InvokeCallbacks()
 {
   mLock.AssertCurrentThreadOwns();
 
   LOG(("CacheEntry::InvokeCallbacks BEGIN [this=%p]", this));
@@ -534,16 +554,21 @@ bool CacheEntry::InvokeCallbacks(bool aR
 
   uint32_t i = 0;
   while (i < mCallbacks.Length()) {
     if (mPreventCallbacks) {
       LOG(("  callbacks prevented!"));
       return false;
     }
 
+    if (mDispatchingCallbacks) {
+      LOG(("  waiting for re-redispatch!"));
+      return false;
+    }
+
     if (!mIsDoomed && (mState == WRITING || mState == REVALIDATING)) {
       LOG(("  entry is being written/revalidated"));
       return false;
     }
 
     if (mCallbacks[i].mReadOnly != aReadOnly) {
       // Callback is not r/w or r/o, go to another one in line
       ++i;
@@ -551,20 +576,27 @@ bool CacheEntry::InvokeCallbacks(bool aR
     }
 
     bool onCheckThread;
     nsresult rv = mCallbacks[i].OnCheckThread(&onCheckThread);
 
     if (NS_SUCCEEDED(rv) && !onCheckThread) {
       // Redispatch to the target thread
       nsRefPtr<nsRunnableMethod<CacheEntry> > event =
-        NS_NewRunnableMethod(this, &CacheEntry::InvokeCallbacksLock);
+        NS_NewRunnableMethod(this, &CacheEntry::InvokeDispatchedCallbacks);
 
       rv = mCallbacks[i].mTargetThread->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
       if (NS_SUCCEEDED(rv)) {
+        // Setting this flag up prevents invocation of any callbacks until
+        // InvokeDispatchedCallbacks is fired on the target thread as a precaution
+        // of any unexpected call to InvokeCallbacks during climb back on the stack.
+        // E.g. GC could release a write handler that invokes callbacks immediately.
+        // Note: InvokeDispatchedCallbacks acquires the lock before checking/dropping
+        // this flag.
+        mDispatchingCallbacks = true;
         LOG(("  re-dispatching to target thread"));
         return false;
       }
     }
 
     Callback callback = mCallbacks[i];
     mCallbacks.RemoveElementAt(i);
 
@@ -584,16 +616,21 @@ bool CacheEntry::InvokeCallbacks(bool aR
 
 bool CacheEntry::InvokeCallback(Callback & aCallback)
 {
   LOG(("CacheEntry::InvokeCallback [this=%p, state=%s, cb=%p]",
     this, StateString(mState), aCallback.mCallback.get()));
 
   mLock.AssertCurrentThreadOwns();
 
+  if (mDispatchingCallbacks) {
+    LOG(("  waiting for callbacks re-dispatch"));
+    return false;
+  }
+
   // When this entry is doomed we want to notify the callback any time
   if (!mIsDoomed) {
     // When we are here, the entry must be loaded from disk
     MOZ_ASSERT(mState > LOADING);
 
     if (mState == WRITING || mState == REVALIDATING) {
       // Prevent invoking other callbacks since one of them is now writing
       // or revalidating this entry.  No consumers should get this entry
--- a/netwerk/cache2/CacheEntry.h
+++ b/netwerk/cache2/CacheEntry.h
@@ -130,17 +130,17 @@ private:
   // We must monitor when a cache entry whose consumer is responsible
   // for writing it the first time gets released.  We must then invoke
   // waiting callbacks to not break the chain.
   class Callback
   {
   public:
     Callback(CacheEntry* aEntry,
              nsICacheEntryOpenCallback *aCallback,
-             bool aReadOnly, bool aCheckOnAnyThread);
+             bool aReadOnly, bool aCheckOnAnyThread, bool aForceAsync);
     Callback(Callback const &aThat);
     ~Callback();
 
     // Called when this callback record changes it's owning entry,
     // mainly during recreation.
     void ExchangeEntry(CacheEntry* aEntry);
 
     // We are raising reference count here to take into account the pending
@@ -148,18 +148,19 @@ private:
     // it's pointer).
     nsRefPtr<CacheEntry> mEntry;
     nsCOMPtr<nsICacheEntryOpenCallback> mCallback;
     nsCOMPtr<nsIThread> mTargetThread;
     bool mReadOnly : 1;
     bool mCheckOnAnyThread : 1;
     bool mRecheckAfterWrite : 1;
     bool mNotWanted : 1;
+    bool mForceAsync : 1;
 
-    nsresult OnCheckThread(bool *aOnCheckThread) const;
+    nsresult OnCheckThread(bool *aOnCheckThread);
     nsresult OnAvailThread(bool *aOnAvailThread) const;
   };
 
   // Since OnCacheEntryAvailable must be invoked on the main thread
   // we need a runnable for it...
   class AvailableCallbackRunnable : public nsRunnable
   {
   public:
@@ -206,17 +207,17 @@ private:
     nsresult mRv;
   };
 
   // Loads from disk asynchronously
   bool Load(bool aTruncate, bool aPriority);
   void OnLoaded();
 
   void RememberCallback(Callback & aCallback, bool aBypassIfBusy);
-  void InvokeCallbacksLock();
+  void InvokeDispatchedCallbacks();
   void InvokeCallbacks();
   bool InvokeCallbacks(bool aReadOnly);
   bool InvokeCallback(Callback & aCallback);
   void InvokeAvailableCallback(Callback const & aCallback);
 
   nsresult OpenOutputStreamInternal(int64_t offset, nsIOutputStream * *_retval);
 
   // When this entry is new and recreated w/o a callback, we need to wrap it
@@ -272,18 +273,22 @@ private:
   // Set when entry is doomed with AsyncDoom() or DoomAlreadyRemoved().
   // Left as a standalone flag to not bother with locking (there is no need).
   bool mIsDoomed;
 
   // Following flags are all synchronized with the cache entry lock.
 
   // Whether security info has already been looked up in metadata.
   bool mSecurityInfoLoaded : 1;
-  // Prevents any callback invocation
+  // Prevents any callback invocation, used to not loop when we're recreating this entry.
   bool mPreventCallbacks : 1;
+  // Set at true between redispatch of callbacks from InvokeCallbacks and call of
+  // InvokeDispatchedCallbacks on the target thread.  Prevents any callback invocation
+  // during that time.
+  bool mDispatchingCallbacks : 1;
   // true: after load and an existing file, or after output stream has been opened.
   //       note - when opening an input stream, and this flag is false, output stream
   //       is open along ; this makes input streams on new entries behave correctly
   //       when EOF is reached (WOULD_BLOCK is returned).
   // false: after load and a new file, or dropped to back to false when a writer
   //        fails to open an output stream.
   bool mHasData : 1;
 
--- a/netwerk/cache2/nsICacheStorage.idl
+++ b/netwerk/cache2/nsICacheStorage.idl
@@ -46,16 +46,23 @@ interface nsICacheStorage : nsISupports
    * Perform the cache entry check (onCacheEntryCheck invocation) on any thread 
    * for optimal perfomance optimization.  If this flag is not specified it is
    * ensured that onCacheEntryCheck is called on the same thread as respective 
    * asyncOpen has been called.
    */
   const uint32_t CHECK_MULTITHREADED = 1 << 4;
 
   /**
+   * Forces the callback to be invoked always only asynchronously regardless
+   * we have all the information to invoke it directly from inside asyncOpenURI
+   * method.
+   */
+  const uint32_t FORCE_ASYNC_CALLBACK = 1 << 5;
+
+  /**
    * Asynchronously opens a cache entry for the specified URI.
    * Result is fetched asynchronously via the callback.
    *
    * @param aURI
    *    The URI to search in cache or to open for writting.
    * @param aIdExtension
    *    Any string that will extend (distinguish) the entry.  Two entries
    *    with the same aURI but different aIdExtension will be comletely
@@ -64,16 +71,17 @@ interface nsICacheStorage : nsISupports
    * @param aFlags
    *    OPEN_NORMALLY - open cache entry normally for read and write
    *    OPEN_TRUNCATE - delete any existing entry before opening it
    *    OPEN_READONLY - don't create an entry if there is none
    *    OPEN_PRIORITY - give this request a priority over others
    *    OPEN_BYPASS_IF_BUSY - backward compatibility only, LOAD_BYPASS_LOCAL_CACHE_IF_BUSY
    *    CHECK_MULTITHREADED - onCacheEntryCheck may be called on any thread, consumer 
    *                          implementation is thread-safe
+   *    FORCE_ASYNC_CALLBACK - always call the callback asynchronously
    * @param aCallback
    *    The consumer that receives the result.
    *    IMPORTANT: The callback may be called sooner the method returns.
    */
   void asyncOpenURI(in nsIURI aURI, in ACString aIdExtension,
                     in uint32_t aFlags,
                     in nsICacheEntryOpenCallback aCallback);
 
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_cache2-01f-basic-async.js
@@ -0,0 +1,28 @@
+function run_test()
+{
+  do_get_profile();
+
+  // Open for write, write
+  asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY | Ci.nsICacheStorage.FORCE_ASYNC_CALLBACK, null,
+    new OpenCallback(NEW, "a1m", "a1d", function(entry) {
+      // Open for read and check
+      asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY | Ci.nsICacheStorage.FORCE_ASYNC_CALLBACK, null,
+        new OpenCallback(NORMAL, "a1m", "a1d", function(entry) {
+          // Open for rewrite (truncate), write different meta and data
+          asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_TRUNCATE | Ci.nsICacheStorage.FORCE_ASYNC_CALLBACK, null,
+            new OpenCallback(NEW, "a2m", "a2d", function(entry) {
+              // Open for read and check
+              asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY | Ci.nsICacheStorage.FORCE_ASYNC_CALLBACK, null,
+                new OpenCallback(NORMAL, "a2m", "a2d", function(entry) {
+                  finish_cache2_test();
+                })
+              );
+            })
+          );
+        })
+      );
+    })
+  );
+
+  do_test_pending();
+}
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_cache2-24-force-async.js
@@ -0,0 +1,42 @@
+function run_test()
+{
+  do_get_profile();
+
+  // Open for write, write, and wait for finishing it before notification to avoid concurrent write
+  // since we want to get as much as possible the scenario when an entry is left in the pool
+  // and a new consumer comes to open it later.
+  var outOfAsyncOpen0 = false;
+  asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY | Ci.nsICacheStorage.FORCE_ASYNC_CALLBACK, null,
+    new OpenCallback(NEW|WAITFORWRITE, "a1m", "a1d", function(entry) {
+      do_check_true(outOfAsyncOpen0);
+      // Open for read, expect callback happen from inside asyncOpenURI
+      var outOfAsyncOpen1 = false;
+      asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
+        function(entry) {
+          do_check_false(outOfAsyncOpen1);
+          var outOfAsyncOpen2 = false;
+          // Open for read, again should be sync
+          asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
+            function(entry) {
+              do_check_false(outOfAsyncOpen2);
+              var outOfAsyncOpen3 = false;
+              // Open for read, expect callback happen from outside of asyncOpenURI
+              asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY | Ci.nsICacheStorage.FORCE_ASYNC_CALLBACK, null,
+                function(entry) {
+                  do_check_true(outOfAsyncOpen3);
+                  finish_cache2_test();
+                }
+              );
+              outOfAsyncOpen3 = true;
+            }
+          );
+          outOfAsyncOpen2 = true;
+        }
+      );
+      outOfAsyncOpen1 = true;
+    })
+  );
+  outOfAsyncOpen0 = true;
+
+  do_test_pending();
+}
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -20,16 +20,17 @@ support-files =
 
 [test_nsIBufferedOutputStream_writeFrom_block.js]
 [test_cache2-01-basic.js]
 [test_cache2-01a-basic-readonly.js]
 [test_cache2-01b-basic-datasize.js]
 [test_cache2-01c-basic-hasmeta-only.js]
 [test_cache2-01d-basic-not-wanted.js]
 [test_cache2-01e-basic-bypass-if-busy.js]
+[test_cache2-01f-basic-async.js]
 [test_cache2-02-open-non-existing.js]
 [test_cache2-03-oncacheentryavail-throws.js]
 [test_cache2-04-oncacheentryavail-throws2x.js]
 [test_cache2-05-visit.js]
 [test_cache2-06-pb-mode.js]
 # Bug 675039, comment 6: "The difference is that the memory cache is disabled in Armv6 builds."
 skip-if = os == "android"
 [test_cache2-07-visit-memory.js]
@@ -52,16 +53,17 @@ skip-if = os == "android"
 [test_cache2-16-conditional-200.js]
 [test_cache2-17-evict-all.js]
 [test_cache2-18-not-valid.js]
 [test_cache2-19-range-206.js]
 [test_cache2-20-range-200.js]
 [test_cache2-21-anon-storage.js]
 [test_cache2-22-anon-visit.js]
 [test_cache2-23-read-over-chunk.js]
+[test_cache2-24-force-async.js]
 [test_304_responses.js]
 # Bug 675039: test hangs on Android-armv6 
 skip-if = os == "android"
 [test_cacheForOfflineUse_no-store.js]
 [test_307_redirect.js]
 [test_NetUtil.js]
 [test_URIs.js]
 [test_aboutblank.js]