Bug 1011771 - fix for crash in CacheFile::RemoveChunk() and CacheFile::Unlock(), r=honzab

netwerk/cache2/CacheFile.cpp

@@ -1054,7 +1054,7 @@ CacheFile::GetChunkLocked(uint32_t aIndex, ECallerType aCaller,
     mChunks.Put(aIndex, chunk);
     mCachedChunks.Remove(aIndex);
     chunk->mFile = this;
-    chunk->mRemovingChunk = false;
+    chunk->mActiveChunk = true;
 
     MOZ_ASSERT(chunk->IsReady());
 
@@ -1084,6 +1084,7 @@ CacheFile::GetChunkLocked(uint32_t aIndex, ECallerType aCaller,
 
     chunk = new CacheFileChunk(this, aIndex);
     mChunks.Put(aIndex, chunk);
+    chunk->mActiveChunk = true;
 
     LOG(("CacheFile::GetChunkLocked() - Reading newly created chunk %p from "
          "the disk [this=%p]", chunk.get(), this));
@@ -1114,6 +1115,7 @@ CacheFile::GetChunkLocked(uint32_t aIndex, ECallerType aCaller,
       // this listener is going to write to the chunk
       chunk = new CacheFileChunk(this, aIndex);
       mChunks.Put(aIndex, chunk);
+      chunk->mActiveChunk = true;
 
       LOG(("CacheFile::GetChunkLocked() - Created new empty chunk %p [this=%p]",
            chunk.get(), this));
@@ -1298,7 +1300,7 @@ CacheFile::MustKeepCachedChunk(uint32_t aIndex)
 }
 
 nsresult
-CacheFile::RemoveChunk(CacheFileChunk *aChunk)
+CacheFile::DeactivateChunk(CacheFileChunk *aChunk)
 {
   nsresult rv;
 
@@ -1308,7 +1310,7 @@ CacheFile::RemoveChunk(CacheFileChunk *aChunk)
   {
     CacheFileAutoLock lock(this);
 
-    LOG(("CacheFile::RemoveChunk() [this=%p, chunk=%p, idx=%u]",
+    LOG(("CacheFile::DeactivateChunk() [this=%p, chunk=%p, idx=%u]",
          this, aChunk, aChunk->Index()));
 
     MOZ_ASSERT(mReady);
@@ -1317,8 +1319,8 @@ CacheFile::RemoveChunk(CacheFileChunk *aChunk)
                (!mHandle && !mMemoryOnly && mOpeningFile));
 
     if (aChunk->mRefCnt != 2) {
-      LOG(("CacheFile::RemoveChunk() - Chunk is still used [this=%p, chunk=%p, "
-           "refcnt=%d]", this, aChunk, aChunk->mRefCnt.get()));
+      LOG(("CacheFile::DeactivateChunk() - Chunk is still used [this=%p, "
+           "chunk=%p, refcnt=%d]", this, aChunk, aChunk->mRefCnt.get()));
 
       // somebody got the reference before the lock was acquired
       return NS_OK;
@@ -1340,7 +1342,7 @@ CacheFile::RemoveChunk(CacheFileChunk *aChunk)
 
     if (NS_FAILED(mStatus)) {
       // Don't write any chunk to disk since this entry will be doomed
-      LOG(("CacheFile::RemoveChunk() - Releasing chunk because of status "
+      LOG(("CacheFile::DeactivateChunk() - Releasing chunk because of status "
            "[this=%p, chunk=%p, mStatus=0x%08x]", this, chunk.get(), mStatus));
 
       RemoveChunkInternal(chunk, false);
@@ -1348,14 +1350,14 @@ CacheFile::RemoveChunk(CacheFileChunk *aChunk)
     }
 
     if (chunk->IsDirty() && !mMemoryOnly && !mOpeningFile) {
-      LOG(("CacheFile::RemoveChunk() - Writing dirty chunk to the disk "
+      LOG(("CacheFile::DeactivateChunk() - Writing dirty chunk to the disk "
            "[this=%p]", this));
 
       mDataIsDirty = true;
 
       rv = chunk->Write(mHandle, this);
       if (NS_FAILED(rv)) {
-        LOG(("CacheFile::RemoveChunk() - CacheFileChunk::Write() failed "
+        LOG(("CacheFile::DeactivateChunk() - CacheFileChunk::Write() failed "
              "synchronously. Removing it. [this=%p, chunk=%p, rv=0x%08x]",
              this, chunk.get(), rv));
 
@@ -1365,18 +1367,17 @@ CacheFile::RemoveChunk(CacheFileChunk *aChunk)
         CacheFileIOManager::DoomFile(mHandle, nullptr);
         return rv;
       }
-      else {
-        // Chunk will be removed in OnChunkWritten if it is still unused
 
-        // chunk needs to be released under the lock to be able to rely on
-        // CacheFileChunk::mRefCnt in CacheFile::OnChunkWritten()
-        chunk = nullptr;
-        return NS_OK;
-      }
+      // Chunk will be removed in OnChunkWritten if it is still unused
+
+      // chunk needs to be released under the lock to be able to rely on
+      // CacheFileChunk::mRefCnt in CacheFile::OnChunkWritten()
+      chunk = nullptr;
+      return NS_OK;
     }
 
     bool keepChunk = ShouldCacheChunk(aChunk->Index());
-    LOG(("CacheFile::RemoveChunk() - %s unused chunk [this=%p, chunk=%p]",
+    LOG(("CacheFile::DeactivateChunk() - %s unused chunk [this=%p, chunk=%p]",
          keepChunk ? "Caching" : "Releasing", this, chunk.get()));
 
     RemoveChunkInternal(chunk, keepChunk);
@@ -1391,7 +1392,9 @@ CacheFile::RemoveChunk(CacheFileChunk *aChunk)
 void
 CacheFile::RemoveChunkInternal(CacheFileChunk *aChunk, bool aCacheChunk)
 {
-  aChunk->mRemovingChunk = true;
+  AssertOwnsLock();
+
+  aChunk->mActiveChunk = false;
   ReleaseOutsideLock(static_cast<CacheFileChunkListener *>(
                        aChunk->mFile.forget().take()));
 
@@ -1738,7 +1741,7 @@ CacheFile::WriteAllCachedChunks(const uint32_t& aIdx,
 
   file->mChunks.Put(aIdx, aChunk);
   aChunk->mFile = file;
-  aChunk->mRemovingChunk = false;
+  aChunk->mActiveChunk = true;
 
   MOZ_ASSERT(aChunk->IsReady());
 

netwerk/cache2/CacheFile.h

@@ -140,7 +140,7 @@ private:
   bool     ShouldCacheChunk(uint32_t aIndex);
   bool     MustKeepCachedChunk(uint32_t aIndex);
 
-  nsresult RemoveChunk(CacheFileChunk *aChunk);
+  nsresult DeactivateChunk(CacheFileChunk *aChunk);
   void     RemoveChunkInternal(CacheFileChunk *aChunk, bool aCacheChunk);
 
   int64_t  BytesFromChunk(uint32_t aIndex);

netwerk/cache2/CacheFileChunk.cpp

@@ -45,11 +45,29 @@ protected:
   nsRefPtr<CacheFileChunk>         mChunk;
 };
 
+bool
+CacheFileChunk::DispatchRelease()
+{
+  if (NS_IsMainThread()) {
+    return false;
+  }
+
+  nsRefPtr<nsRunnableMethod<CacheFileChunk, MozExternalRefCountType, false> > event =
+    NS_NewNonOwningRunnableMethod(this, &CacheFileChunk::Release);
+  NS_DispatchToMainThread(event);
+
+  return true;
+}
 
 NS_IMPL_ADDREF(CacheFileChunk)
 NS_IMETHODIMP_(MozExternalRefCountType)
 CacheFileChunk::Release()
 {
+  if (DispatchRelease()) {
+    // Redispatched to the main thread.
+    return mRefCnt - 1;
+  }
+
   NS_PRECONDITION(0 != mRefCnt, "dup release");
   nsrefcnt count = --mRefCnt;
   NS_LOG_RELEASE(this, count, "CacheFileChunk");
@@ -60,8 +78,18 @@ CacheFileChunk::Release()
     return 0;
   }
 
-  if (!mRemovingChunk && count == 1) {
-    mFile->RemoveChunk(this);
+  // We can safely access this chunk after decreasing mRefCnt since we re-post
+  // all calls to Release() happening off the main thread to the main thread.
+  // I.e. no other Release() that would delete the object could be run before
+  // we call CacheFile::DeactivateChunk().
+  //
+  // NOTE: we don't grab the CacheFile's lock, so the chunk might be addrefed
+  // on another thread before CacheFile::DeactivateChunk() grabs the lock on
+  // this thread. To make sure we won't deactivate chunk that was just returned
+  // to a new consumer we check mRefCnt once again in
+  // CacheFile::DeactivateChunk() after we grab the lock.
+  if (mActiveChunk && count == 1) {
+    mFile->DeactivateChunk(this);
   }
 
   return count;
@@ -78,7 +106,7 @@ CacheFileChunk::CacheFileChunk(CacheFile *aFile, uint32_t aIndex)
   , mState(INITIAL)
   , mStatus(NS_OK)
   , mIsDirty(false)
-  , mRemovingChunk(false)
+  , mActiveChunk(false)
   , mDataSize(0)
   , mBuf(nullptr)
   , mBufSize(0)

netwerk/cache2/CacheFileChunk.h

@@ -68,6 +68,7 @@ class CacheFileChunk : public CacheFileIOListener
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
+  bool DispatchRelease();
 
   CacheFileChunk(CacheFile *aFile, uint32_t aIndex);
 
@@ -128,7 +129,10 @@ private:
   EState   mState;
   nsresult mStatus;
   bool     mIsDirty;
-  bool     mRemovingChunk;
+  bool     mActiveChunk; // Is true iff the chunk is in CacheFile::mChunks.
+                         // Adding/removing chunk to/from mChunks as well as
+                         // changing this member happens under the CacheFile's
+                         // lock.
   uint32_t mDataSize;
 
   char    *mBuf;