Bug 949175 - Remove possibility to un-persist a cache entry after it has been opened, r=michal

content/html/content/test/browser_bug649778.js

@@ -23,7 +23,7 @@ function checkCache(url, inMemory, shouldExist, cb)
     this.onCacheEntryAvailable = function oCEA(entry, isNew, appCache, status) {
       if (shouldExist) {
         ok(entry, "Entry not found");
-        is(this.inMemory, !entry.persistToDisk, "Entry is " + (inMemory ? "" : " not ") + " in memory as expected");
+        is(this.inMemory, !entry.persistent, "Entry is " + (inMemory ? "" : " not ") + " in memory as expected");
         is(status, Components.results.NS_OK, "Entry not found");
       } else {
         ok(!entry, "Entry found");

netwerk/cache2/CacheEntry.cpp

@@ -396,7 +396,8 @@ NS_IMETHODIMP CacheEntry::OnFileDoomed(nsresult aResult)
   return NS_OK;
 }
 
-already_AddRefed<CacheEntryHandle> CacheEntry::ReopenTruncated(nsICacheEntryOpenCallback* aCallback)
+already_AddRefed<CacheEntryHandle> CacheEntry::ReopenTruncated(bool aMemoryOnly,
+                                                               nsICacheEntryOpenCallback* aCallback)
 {
   LOG(("CacheEntry::ReopenTruncated [this=%p]", this));
 
@@ -413,7 +414,7 @@ already_AddRefed<CacheEntryHandle> CacheEntry::ReopenTruncated(nsICacheEntryOpen
     // The following call dooms this entry (calls DoomAlreadyRemoved on us)
     nsresult rv = CacheStorageService::Self()->AddStorageEntry(
       GetStorageID(), GetURI(), GetEnhanceID(),
-      mUseDisk,
+      mUseDisk && !aMemoryOnly,
       true, // always create
       true, // truncate existing (this one)
       getter_AddRefs(handle));
@@ -848,7 +849,7 @@ uint32_t CacheEntry::GetMetadataMemoryConsumption()
 
 // nsICacheEntry
 
-NS_IMETHODIMP CacheEntry::GetPersistToDisk(bool *aPersistToDisk)
+NS_IMETHODIMP CacheEntry::GetPersistent(bool *aPersistToDisk)
 {
   // No need to sync when only reading.
   // When consumer needs to be consistent with state of the memory storage entries
@@ -856,28 +857,6 @@ NS_IMETHODIMP CacheEntry::GetPersistToDisk(bool *aPersistToDisk)
   *aPersistToDisk = mUseDisk;
   return NS_OK;
 }
-NS_IMETHODIMP CacheEntry::SetPersistToDisk(bool aPersistToDisk)
-{
-  LOG(("CacheEntry::SetPersistToDisk [this=%p, persist=%d]", this, aPersistToDisk));
-
-  if (mState >= READY) {
-    LOG(("  failed, called after filling the entry"));
-    return NS_ERROR_NOT_AVAILABLE;
-  }
-
-  if (mUseDisk == aPersistToDisk)
-    return NS_OK;
-
-  mozilla::MutexAutoLock lock(CacheStorageService::Self()->Lock());
-
-  mUseDisk = aPersistToDisk;
-  CacheStorageService::Self()->RecordMemoryOnlyEntry(
-    this, !aPersistToDisk, false /* don't overwrite */);
-
-  // File persistence is setup just before we open output stream on it.
-
-  return NS_OK;
-}
 
 NS_IMETHODIMP CacheEntry::GetKey(nsACString & aKey)
 {
@@ -1210,13 +1189,14 @@ NS_IMETHODIMP CacheEntry::SetValid()
   return NS_OK;
 }
 
-NS_IMETHODIMP CacheEntry::Recreate(nsICacheEntry **_retval)
+NS_IMETHODIMP CacheEntry::Recreate(bool aMemoryOnly,
+                                   nsICacheEntry **_retval)
 {
   LOG(("CacheEntry::Recreate [this=%p, state=%s]", this, StateString(mState)));
 
   mozilla::MutexAutoLock lock(mLock);
 
-  nsRefPtr<CacheEntryHandle> handle = ReopenTruncated(nullptr);
+  nsRefPtr<CacheEntryHandle> handle = ReopenTruncated(aMemoryOnly, nullptr);
   if (handle) {
     handle.forget(_retval);
     return NS_OK;

netwerk/cache2/CacheEntry.h

@@ -224,7 +224,8 @@ private:
   // Called only from DoomAlreadyRemoved()
   void DoomFile();
 
-  already_AddRefed<CacheEntryHandle> ReopenTruncated(nsICacheEntryOpenCallback* aCallback);
+  already_AddRefed<CacheEntryHandle> ReopenTruncated(bool aMemoryOnly,
+                                                     nsICacheEntryOpenCallback* aCallback);
   void TransferCallbacks(CacheEntry & aFromEntry);
 
   mozilla::Mutex mLock;

netwerk/cache2/OldWrappers.cpp

@@ -246,7 +246,7 @@ NS_IMETHODIMP _OldCacheEntryWrapper::GetDataSize(int64_t *aSize)
   return NS_OK;
 }
 
-NS_IMETHODIMP _OldCacheEntryWrapper::GetPersistToDisk(bool *aPersistToDisk)
+NS_IMETHODIMP _OldCacheEntryWrapper::GetPersistent(bool *aPersistToDisk)
 {
   if (!mOldDesc) {
     return NS_ERROR_NULL_POINTER;
@@ -263,34 +263,8 @@ NS_IMETHODIMP _OldCacheEntryWrapper::GetPersistToDisk(bool *aPersistToDisk)
   return NS_OK;
 }
 
-NS_IMETHODIMP _OldCacheEntryWrapper::SetPersistToDisk(bool aPersistToDisk)
+NS_IMETHODIMP _OldCacheEntryWrapper::Recreate(bool aMemoryOnly,
-{
+                                              nsICacheEntry** aResult)
-  if (!mOldDesc) {
-    return NS_ERROR_NULL_POINTER;
-  }
-
-  nsresult rv;
-
-  nsCacheStoragePolicy policy;
-  rv = mOldDesc->GetStoragePolicy(&policy);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (policy == nsICache::STORE_OFFLINE) {
-    return aPersistToDisk
-      ? NS_OK
-      : NS_ERROR_NOT_AVAILABLE;
-  }
-
-  policy = aPersistToDisk
-    ? nsICache::STORE_ON_DISK
-    : nsICache::STORE_IN_MEMORY;
-  rv = mOldDesc->SetStoragePolicy(policy);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  return NS_OK;
-}
-
-NS_IMETHODIMP _OldCacheEntryWrapper::Recreate(nsICacheEntry** aResult)
 {
   NS_ENSURE_TRUE(mOldDesc, NS_ERROR_NOT_AVAILABLE);
 
@@ -303,6 +277,9 @@ NS_IMETHODIMP _OldCacheEntryWrapper::Recreate(nsICacheEntry** aResult)
 
   LOG(("_OldCacheEntryWrapper::Recreate [this=%p]", this));
 
+  if (aMemoryOnly)
+    mOldDesc->SetStoragePolicy(nsICache::STORE_IN_MEMORY);
+
   nsCOMPtr<nsICacheEntry> self(this);
   self.forget(aResult);
   return NS_OK;

netwerk/cache2/OldWrappers.h

@@ -30,11 +30,10 @@ public:
   NS_FORWARD_NSICACHEENTRYINFO(mOldInfo->)
 
   NS_IMETHOD AsyncDoom(nsICacheEntryDoomCallback* listener);
-  NS_IMETHOD GetPersistToDisk(bool *aPersistToDisk);
+  NS_IMETHOD GetPersistent(bool *aPersistToDisk);
-  NS_IMETHOD SetPersistToDisk(bool aPersistToDisk);
   NS_IMETHOD SetValid() { return NS_OK; }
   NS_IMETHOD MetaDataReady() { return NS_OK; }
-  NS_IMETHOD Recreate(nsICacheEntry**);
+  NS_IMETHOD Recreate(bool, nsICacheEntry**);
   NS_IMETHOD GetDataSize(int64_t *size);
   NS_IMETHOD OpenInputStream(int64_t offset, nsIInputStream * *_retval);
   NS_IMETHOD OpenOutputStream(int64_t offset, nsIOutputStream * *_retval);

netwerk/cache2/nsICacheEntry.idl

@@ -16,7 +16,7 @@ interface nsICacheListener;
 interface nsIFile;
 interface nsICacheMetaDataVisitor;
 
-[scriptable, uuid(1785f6f1-18b3-4cb4-ae99-6c5545c1de19)]
+[scriptable, uuid(d3fbd879-6d3a-4bd0-b12e-7d86ab27ea90)]
 interface nsICacheEntry : nsISupports
 {
   /**
@@ -25,13 +25,11 @@ interface nsICacheEntry : nsISupports
   readonly attribute ACString key;
 
   /**
-   * Whether the data can be persist to disk.
+   * Whether the entry is memory/only or persisted to disk.
-   * NOTE: This attribute must be set BEFORE opening the output stream.
+   * Note: private browsing entries are reported as persistent for consistency
-   * Switching this flag does not immediately affect creation of the disk
+   * while are not actually persisted to disk.
-   * file from memory-only data or eviction of the disk file and loading it
-   * to memory-only.
    */
-  attribute boolean persistToDisk;
+  readonly attribute boolean persistent;
 
   /**
    * Get the number of times the cache entry has been opened.
@@ -142,13 +140,16 @@ interface nsICacheEntry : nsISupports
    * to exchange this entry for the newly created.
    * Used on 200 responses to conditional requests.
    *
+   * @param aMemoryOnly
+   *    - whether the entry is to be created as memory/only regardless how
+   *      the entry being recreated persistence is set
    * @returns
    *    - an entry that can be used to write to
    * @throws
    *    - NS_ERROR_NOT_AVAILABLE when the entry cannot be from some reason
    *      recreated for write
    */
-  nsICacheEntry recreate();
+  nsICacheEntry recreate([optional] in boolean aMemoryOnly);
 
   /**
    * Returns the length of data this entry holds.

netwerk/protocol/http/nsHttpChannel.cpp

@@ -3640,11 +3640,22 @@ nsHttpChannel::InitCacheEntry()
     LOG(("nsHttpChannel::InitCacheEntry [this=%p entry=%p]\n",
         this, mCacheEntry.get()));
 
-    if (!mCacheEntryIsWriteOnly) {
+    bool recreate = !mCacheEntryIsWriteOnly;
+    bool dontPersist = mLoadFlags & INHIBIT_PERSISTENT_CACHING;
+
+    if (!recreate && dontPersist) {
+        // If the current entry is persistent but we inhibit peristence
+        // then force recreation of the entry as memory/only.
+        rv = mCacheEntry->GetPersistent(&recreate);
+        if (NS_FAILED(rv))
+            return rv;
+    }
+
+    if (recreate) {
         LOG(("  we have a ready entry, but reading it again from the server -> recreating cache entry\n"));
         nsCOMPtr<nsICacheEntry> currentEntry;
         currentEntry.swap(mCacheEntry);
-        rv = currentEntry->Recreate(getter_AddRefs(mCacheEntry));
+        rv = currentEntry->Recreate(dontPersist, getter_AddRefs(mCacheEntry));
         if (NS_FAILED(rv)) {
           LOG(("  recreation failed, the response will not be cached"));
           return NS_OK;
@@ -3653,11 +3664,6 @@ nsHttpChannel::InitCacheEntry()
         mCacheEntryIsWriteOnly = true;
     }
 
-    if (mLoadFlags & INHIBIT_PERSISTENT_CACHING) {
-        rv = mCacheEntry->SetPersistToDisk(false);
-        if (NS_FAILED(rv)) return rv;
-    }
-
     // Set the expiration time for this cache entry
     rv = UpdateExpirationTime();
     if (NS_FAILED(rv)) return rv;

netwerk/test/unit/test_bug812167.js

@@ -67,7 +67,7 @@ function check_response(path, request, buffer, expectedExpiration, continuation)
     do_check_eq(status, 0);
 
     // Expired entry is on disk, no-store entry is in memory
-    do_check_eq(entry.persistToDisk, expectedExpiration);
+    do_check_eq(entry.persistent, expectedExpiration);
 
     // Do the request again and check the server handler is called appropriately
     var chan = make_channel(path);
@@ -82,7 +82,7 @@ function check_response(path, request, buffer, expectedExpiration, continuation)
         // Handler had to be called second time (no-store forces validate),
         // and we are just in memory
         do_check_eq(redirectHandler_NoStore_calls, 2);
-        do_check_true(!entry.persistToDisk);
+        do_check_true(!entry.persistent);
       }
 
       continuation();