From 30d493e72841dfd0743a4f39eb003e9b6348c4d8 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Wed, 22 Jan 2014 18:54:51 +0100 Subject: [PATCH] Bug 958311 - Fix partial content condition in nsHttpChannel, prevent any reuse of doomed files in cache v2, r=michal --- netwerk/cache2/CacheEntry.cpp | 62 ++++++++++++++----------- netwerk/cache2/CacheEntry.h | 7 ++- netwerk/cache2/CacheFile.cpp | 3 +- netwerk/protocol/http/nsHttpChannel.cpp | 4 +- 4 files changed, 44 insertions(+), 32 deletions(-) diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp index 9929036bb28..42e80bf3288 100644 --- a/netwerk/cache2/CacheEntry.cpp +++ b/netwerk/cache2/CacheEntry.cpp @@ -1133,11 +1133,14 @@ NS_IMETHODIMP CacheEntry::AsyncDoom(nsICacheEntryDoomCallback *aCallback) mIsDoomed = true; mDoomCallback = aCallback; - BackgroundOp(Ops::DOOM); } - // Immediately remove the entry from the storage hash table - CacheStorageService::Self()->RemoveEntry(this); + // This immediately removes the entry from the master hashtable and also + // immediately dooms the file. This way we make sure that any consumer + // after this point asking for the same entry won't get + // a) this entry + // b) a new entry with the same file + PurgeAndDoom(); return NS_OK; } @@ -1403,8 +1406,6 @@ void CacheEntry::PurgeAndDoom() { LOG(("CacheEntry::PurgeAndDoom [this=%p]", this)); - MOZ_ASSERT(CacheStorageService::IsOnManagementThread()); - CacheStorageService::Self()->RemoveEntry(this); DoomAlreadyRemoved(); } @@ -1413,36 +1414,45 @@ void CacheEntry::DoomAlreadyRemoved() { LOG(("CacheEntry::DoomAlreadyRemoved [this=%p]", this)); + mozilla::MutexAutoLock lock(mLock); + mIsDoomed = true; - if (!CacheStorageService::IsOnManagementThread()) { - mozilla::MutexAutoLock lock(mLock); + // This schedules dooming of the file, dooming is ensured to happen + // sooner than demand to open the same file made after this point + // so that we don't get this file for any newer opened entry(s). + DoomFile(); - BackgroundOp(Ops::DOOM); - return; - } + // Must force post here since may be indirectly called from + // InvokeCallbacks of this entry and we don't want reentrancy here. + BackgroundOp(Ops::CALLBACKS, true); + // Process immediately when on the management thread. + BackgroundOp(Ops::UNREGISTER); +} - CacheStorageService::Self()->UnregisterEntry(this); - - { - mozilla::MutexAutoLock lock(mLock); - - if (mCallbacks.Length()) { - // Must force post here since may be indirectly called from - // InvokeCallbacks of this entry and we don't want reentrancy here. - BackgroundOp(Ops::CALLBACKS, true); - } - } +void CacheEntry::DoomFile() +{ + nsresult rv = NS_ERROR_NOT_AVAILABLE; if (NS_SUCCEEDED(mFileStatus)) { - nsresult rv = mFile->Doom(mDoomCallback ? this : nullptr); + // Always calls the callback asynchronously. + rv = mFile->Doom(mDoomCallback ? this : nullptr); if (NS_SUCCEEDED(rv)) { LOG((" file doomed")); return; } + + if (NS_ERROR_FILE_NOT_FOUND == rv) { + // File is set to be just memory-only, notify the callbacks + // and pretend dooming has succeeded. From point of view of + // the entry it actually did - the data is gone and cannot be + // reused. + rv = NS_OK; + } } - OnFileDoomed(NS_OK); + // Always posts to the main thread. + OnFileDoomed(rv); } void CacheEntry::BackgroundOp(uint32_t aOperations, bool aForceAsync) @@ -1496,10 +1506,10 @@ void CacheEntry::BackgroundOp(uint32_t aOperations, bool aForceAsync) CacheStorageService::Self()->RegisterEntry(this); } - if (aOperations & Ops::DOOM) { - LOG(("CacheEntry DOOM [this=%p]", this)); + if (aOperations & Ops::UNREGISTER) { + LOG(("CacheEntry UNREGISTER [this=%p]", this)); - DoomAlreadyRemoved(); + CacheStorageService::Self()->UnregisterEntry(this); } if (aOperations & Ops::CALLBACKS) { diff --git a/netwerk/cache2/CacheEntry.h b/netwerk/cache2/CacheEntry.h index 12eb5ea9ce4..604eecaabc3 100644 --- a/netwerk/cache2/CacheEntry.h +++ b/netwerk/cache2/CacheEntry.h @@ -221,6 +221,9 @@ private: void BackgroundOp(uint32_t aOperation, bool aForceAsync = false); void StoreFrecency(); + // Called only from DoomAlreadyRemoved() + void DoomFile(); + already_AddRefed ReopenTruncated(nsICacheEntryOpenCallback* aCallback); void TransferCallbacks(CacheEntry & aFromEntry); @@ -305,8 +308,8 @@ private: public: static uint32_t const REGISTER = 1 << 0; static uint32_t const FRECENCYUPDATE = 1 << 1; - static uint32_t const DOOM = 1 << 2; - static uint32_t const CALLBACKS = 1 << 3; + static uint32_t const CALLBACKS = 1 << 2; + static uint32_t const UNREGISTER = 1 << 3; Ops() : mFlags(0) { } uint32_t Grab() { uint32_t flags = mFlags; mFlags = 0; return flags; } diff --git a/netwerk/cache2/CacheFile.cpp b/netwerk/cache2/CacheFile.cpp index 49e0d51605f..92d3c6bc32d 100644 --- a/netwerk/cache2/CacheFile.cpp +++ b/netwerk/cache2/CacheFile.cpp @@ -920,8 +920,7 @@ CacheFile::Doom(CacheFileListener *aCallback) nsresult rv = NS_OK; if (mMemoryOnly) { - // TODO what exactly to do here? - return NS_ERROR_NOT_AVAILABLE; + return NS_ERROR_FILE_NOT_FOUND; } nsCOMPtr listener; diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index 317211b6355..34d832544e8 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -2018,7 +2018,7 @@ nsHttpChannel::MaybeSetupByteRangeRequest(int64_t partialLen, int64_t contentLen mIsPartialRequest = false; if (!IsResumable(partialLen, contentLength)) - return NS_OK; + return NS_ERROR_NOT_RESUMABLE; // looks like a partial entry we can reuse; add If-Range // and Range headers. @@ -2757,7 +2757,7 @@ nsHttpChannel::OnCacheEntryCheck(nsICacheEntry* entry, nsIApplicationCache* appC "[content-length=%lld size=%lld]\n", contentLength, size)); rv = MaybeSetupByteRangeRequest(size, contentLength); - mCachedContentIsPartial = NS_SUCCEEDED(rv); + mCachedContentIsPartial = NS_SUCCEEDED(rv) && mIsPartialRequest; if (mCachedContentIsPartial) { rv = OpenCacheInputStream(entry, false); *aResult = ENTRY_NEEDS_REVALIDATION;