From 22461fd77e61d52419ffd27c4e2d32292440c861 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Mon, 9 Jun 2014 20:59:08 +0200 Subject: [PATCH] Bug 938186 - HTTP cache v2: introduce DISALLOW_SYNC_CALLBACK for opening cache entries, r=michal --- netwerk/cache2/CacheEntry.cpp | 47 +++++++++++++++++-- netwerk/cache2/CacheEntry.h | 13 +++-- netwerk/cache2/nsICacheStorage.idl | 8 ++++ .../test/unit/test_cache2-01f-basic-async.js | 28 +++++++++++ .../test/unit/test_cache2-24-force-async.js | 42 +++++++++++++++++ netwerk/test/unit/xpcshell.ini | 2 + 6 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 netwerk/test/unit/test_cache2-01f-basic-async.js create mode 100644 netwerk/test/unit/test_cache2-24-force-async.js diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp index 6af10d3a2e2..086f34bf5a5 100644 --- a/netwerk/cache2/CacheEntry.cpp +++ b/netwerk/cache2/CacheEntry.cpp @@ -76,7 +76,7 @@ CacheEntryHandle::~CacheEntryHandle() CacheEntry::Callback::Callback(CacheEntry* aEntry, nsICacheEntryOpenCallback *aCallback, - bool aReadOnly, bool aCheckOnAnyThread) + bool aReadOnly, bool aCheckOnAnyThread, bool aForceAsync) : mEntry(aEntry) , mCallback(aCallback) , mTargetThread(do_GetCurrentThread()) @@ -84,6 +84,7 @@ CacheEntry::Callback::Callback(CacheEntry* aEntry, , mCheckOnAnyThread(aCheckOnAnyThread) , mRecheckAfterWrite(false) , mNotWanted(false) +, mForceAsync(aForceAsync) { MOZ_COUNT_CTOR(CacheEntry::Callback); @@ -101,6 +102,7 @@ CacheEntry::Callback::Callback(CacheEntry::Callback const &aThat) , mCheckOnAnyThread(aThat.mCheckOnAnyThread) , mRecheckAfterWrite(aThat.mRecheckAfterWrite) , mNotWanted(aThat.mNotWanted) +, mForceAsync(aThat.mForceAsync) { MOZ_COUNT_CTOR(CacheEntry::Callback); @@ -131,8 +133,19 @@ void CacheEntry::Callback::ExchangeEntry(CacheEntry* aEntry) 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); @@ -170,6 +183,7 @@ CacheEntry::CacheEntry(const nsACString& aStorageID, , mIsDoomed(false) , mSecurityInfoLoaded(false) , mPreventCallbacks(false) +, mDispatchingCallbacks(false) , mHasData(false) , mState(NOTLOADED) , mRegistration(NEVERREGISTERED) @@ -268,11 +282,12 @@ void CacheEntry::AsyncOpen(nsICacheEntryOpenCallback* aCallback, uint32_t aFlags 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); @@ -509,9 +524,14 @@ void CacheEntry::RememberCallback(Callback & aCallback, bool aBypassIfBusy) 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(); } @@ -539,6 +559,11 @@ bool CacheEntry::InvokeCallbacks(bool aReadOnly) 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; @@ -556,10 +581,17 @@ bool CacheEntry::InvokeCallbacks(bool aReadOnly) if (NS_SUCCEEDED(rv) && !onCheckThread) { // Redispatch to the target thread nsRefPtr > 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; } @@ -589,6 +621,11 @@ bool CacheEntry::InvokeCallback(Callback & aCallback) 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 diff --git a/netwerk/cache2/CacheEntry.h b/netwerk/cache2/CacheEntry.h index e4ec3d72dbf..a1fb32b1825 100644 --- a/netwerk/cache2/CacheEntry.h +++ b/netwerk/cache2/CacheEntry.h @@ -135,7 +135,7 @@ private: public: Callback(CacheEntry* aEntry, nsICacheEntryOpenCallback *aCallback, - bool aReadOnly, bool aCheckOnAnyThread); + bool aReadOnly, bool aCheckOnAnyThread, bool aForceAsync); Callback(Callback const &aThat); ~Callback(); @@ -153,8 +153,9 @@ private: 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; }; @@ -211,7 +212,7 @@ private: void OnLoaded(); void RememberCallback(Callback & aCallback, bool aBypassIfBusy); - void InvokeCallbacksLock(); + void InvokeDispatchedCallbacks(); void InvokeCallbacks(); bool InvokeCallbacks(bool aReadOnly); bool InvokeCallback(Callback & aCallback); @@ -277,8 +278,12 @@ private: // 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 diff --git a/netwerk/cache2/nsICacheStorage.idl b/netwerk/cache2/nsICacheStorage.idl index ca6f948b085..d465329d52c 100644 --- a/netwerk/cache2/nsICacheStorage.idl +++ b/netwerk/cache2/nsICacheStorage.idl @@ -50,6 +50,13 @@ interface nsICacheStorage : nsISupports */ 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. @@ -69,6 +76,7 @@ interface nsICacheStorage : nsISupports * 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. diff --git a/netwerk/test/unit/test_cache2-01f-basic-async.js b/netwerk/test/unit/test_cache2-01f-basic-async.js new file mode 100644 index 00000000000..5cda7b5b650 --- /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(); +} diff --git a/netwerk/test/unit/test_cache2-24-force-async.js b/netwerk/test/unit/test_cache2-24-force-async.js new file mode 100644 index 00000000000..5cd4f153aa0 --- /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(); +} diff --git a/netwerk/test/unit/xpcshell.ini b/netwerk/test/unit/xpcshell.ini index 33e637bcf9e..9f50f94147b 100644 --- a/netwerk/test/unit/xpcshell.ini +++ b/netwerk/test/unit/xpcshell.ini @@ -25,6 +25,7 @@ support-files = [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] @@ -57,6 +58,7 @@ skip-if = os == "android" [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"