From 8530936fe0315e0397bd4a9127c87e182dbfb2b8 Mon Sep 17 00:00:00 2001 From: Michal Novotny Date: Wed, 28 May 2014 16:16:03 +0200 Subject: [PATCH] Bug 1011771 - fix for crash in CacheFile::RemoveChunk() and CacheFile::Unlock(), r=honzab --- netwerk/cache2/CacheFile.cpp | 39 +++++++++++++++++-------------- netwerk/cache2/CacheFile.h | 2 +- netwerk/cache2/CacheFileChunk.cpp | 34 ++++++++++++++++++++++++--- netwerk/cache2/CacheFileChunk.h | 6 ++++- 4 files changed, 58 insertions(+), 23 deletions(-) diff --git a/netwerk/cache2/CacheFile.cpp b/netwerk/cache2/CacheFile.cpp index 34f46b440b10..9e044fe81a70 100644 --- a/netwerk/cache2/CacheFile.cpp +++ b/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( 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()); diff --git a/netwerk/cache2/CacheFile.h b/netwerk/cache2/CacheFile.h index ea41c7028738..bdaf8088e053 100644 --- a/netwerk/cache2/CacheFile.h +++ b/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); diff --git a/netwerk/cache2/CacheFileChunk.cpp b/netwerk/cache2/CacheFileChunk.cpp index 3b1c55518df7..5d4db22bcf94 100644 --- a/netwerk/cache2/CacheFileChunk.cpp +++ b/netwerk/cache2/CacheFileChunk.cpp @@ -45,11 +45,29 @@ protected: nsRefPtr mChunk; }; +bool +CacheFileChunk::DispatchRelease() +{ + if (NS_IsMainThread()) { + return false; + } + + nsRefPtr > 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) diff --git a/netwerk/cache2/CacheFileChunk.h b/netwerk/cache2/CacheFileChunk.h index 7a759a6ccc55..08b0b027364a 100644 --- a/netwerk/cache2/CacheFileChunk.h +++ b/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;