Bug 1154124 - Prevent recursion when calling HTTP cache entry's callbacks. r=michal
authorHonza Bambas <honzab.moz@firemni.cz>
Thu, 14 Jan 2016 21:16:06 +0100
changeset 281176 85bc8c6ea180e68a85f53f44e10300ec01323ccd
parent 281175 e97c3cf176607e044ebd1c9d24de52ae5b796e48
child 281177 075be35157ddf71be64e139a23a23055d84f800f
push id29930
push usercbook@mozilla.com
push dateFri, 22 Jan 2016 11:05:50 +0000
treeherdermozilla-central@7104d650a97d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal
bugs1154124
milestone46.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 1154124 - Prevent recursion when calling HTTP cache entry's callbacks. r=michal
netwerk/cache2/CacheEntry.cpp
--- a/netwerk/cache2/CacheEntry.cpp
+++ b/netwerk/cache2/CacheEntry.cpp
@@ -922,66 +922,64 @@ CacheEntryHandle* CacheEntry::NewWriteHa
 
   return (mWriter = NewHandle());
 }
 
 void CacheEntry::OnHandleClosed(CacheEntryHandle const* aHandle)
 {
   LOG(("CacheEntry::OnHandleClosed [this=%p, state=%s, handle=%p]", this, StateString(mState), aHandle));
 
-  nsCOMPtr<nsIOutputStream> outputStream;
-
-  {
-    mozilla::MutexAutoLock lock(mLock);
-
-    if (mWriter != aHandle) {
-      LOG(("  not the writer"));
-      return;
-    }
+  mozilla::MutexAutoLock lock(mLock);
 
-    if (mOutputStream) {
-      // No one took our internal output stream, so there are no data
-      // and output stream has to be open symultaneously with input stream
-      // on this entry again.
-      mHasData = false;
-    }
-
-    outputStream.swap(mOutputStream);
-    mWriter = nullptr;
+  if (mWriter != aHandle) {
+    LOG(("  not the writer"));
+    return;
+  }
 
-    if (mState == WRITING) {
-      LOG(("  reverting to state EMPTY - write failed"));
-      mState = EMPTY;
-    }
-    else if (mState == REVALIDATING) {
-      LOG(("  reverting to state READY - reval failed"));
-      mState = READY;
-    }
-
-    if (mState == READY && !mHasData) {
-      // We may get to this state when following steps happen:
-      // 1. a new entry is given to a consumer
-      // 2. the consumer calls MetaDataReady(), we transit to READY
-      // 3. abandons the entry w/o opening the output stream, mHasData left false
-      //
-      // In this case any following consumer will get a ready entry (with metadata)
-      // but in state like the entry data write was still happening (was in progress)
-      // and will indefinitely wait for the entry data or even the entry itself when
-      // RECHECK_AFTER_WRITE is returned from onCacheEntryCheck.
-      LOG(("  we are in READY state, pretend we have data regardless it"
-            " has actully been never touched"));
-      mHasData = true;
-    }
-
-    InvokeCallbacks();
+  if (mOutputStream) {
+    LOG(("  abandoning phantom output stream"));
+    // No one took our internal output stream, so there are no data
+    // and output stream has to be open symultaneously with input stream
+    // on this entry again.
+    mHasData = false;
+    // This asynchronously ends up invoking callbacks on this entry
+    // through OnOutputClosed() call.
+    mOutputStream->Close();
+    mOutputStream = nullptr;
+  } else {
+    // We must always redispatch, otherwise there is a risk of stack
+    // overflow.  This code can recurse deeply.  It won't execute sooner
+    // than we release mLock.
+    BackgroundOp(Ops::CALLBACKS, true);
   }
 
-  if (outputStream) {
-    LOG(("  abandoning phantom output stream"));
-    outputStream->Close();
+  mWriter = nullptr;
+
+  if (mState == WRITING) {
+    LOG(("  reverting to state EMPTY - write failed"));
+    mState = EMPTY;
+  }
+  else if (mState == REVALIDATING) {
+    LOG(("  reverting to state READY - reval failed"));
+    mState = READY;
+  }
+
+  if (mState == READY && !mHasData) {
+    // We may get to this state when following steps happen:
+    // 1. a new entry is given to a consumer
+    // 2. the consumer calls MetaDataReady(), we transit to READY
+    // 3. abandons the entry w/o opening the output stream, mHasData left false
+    //
+    // In this case any following consumer will get a ready entry (with metadata)
+    // but in state like the entry data write was still happening (was in progress)
+    // and will indefinitely wait for the entry data or even the entry itself when
+    // RECHECK_AFTER_WRITE is returned from onCacheEntryCheck.
+    LOG(("  we are in READY state, pretend we have data regardless it"
+          " has actully been never touched"));
+    mHasData = true;
   }
 }
 
 void CacheEntry::OnOutputClosed()
 {
   // Called when the file's output stream is closed.  Invoke any callbacks
   // waiting for complete entry.