Bug 958311 - Fix partial content condition in nsHttpChannel, prevent any reuse of doomed files in cache v2, r=michal

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) {

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<CacheEntryHandle> 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; }

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<CacheFileIOListener> listener;

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;