From 4fa7f4f872352fd3ebe9f24bd416d1ab39b21314 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Tue, 4 Feb 2014 01:52:59 +0100 Subject: [PATCH] Bug 949175 - Remove possibility to un-persist a cache entry after it has been opened, r=michal --- .../html/content/test/browser_bug649778.js | 2 +- netwerk/cache2/CacheEntry.cpp | 34 ++++-------------- netwerk/cache2/CacheEntry.h | 3 +- netwerk/cache2/OldWrappers.cpp | 35 ++++--------------- netwerk/cache2/OldWrappers.h | 5 ++- netwerk/cache2/nsICacheEntry.idl | 17 ++++----- netwerk/protocol/http/nsHttpChannel.cpp | 20 +++++++---- netwerk/test/unit/test_bug812167.js | 4 +-- 8 files changed, 42 insertions(+), 78 deletions(-) diff --git a/content/html/content/test/browser_bug649778.js b/content/html/content/test/browser_bug649778.js index 37d708a50d1..8c5518093c2 100644 --- a/content/html/content/test/browser_bug649778.js +++ b/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"); diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp index 42e80bf3288..5e466fa196e 100644 --- a/netwerk/cache2/CacheEntry.cpp +++ b/netwerk/cache2/CacheEntry.cpp @@ -396,7 +396,8 @@ NS_IMETHODIMP CacheEntry::OnFileDoomed(nsresult aResult) return NS_OK; } -already_AddRefed CacheEntry::ReopenTruncated(nsICacheEntryOpenCallback* aCallback) +already_AddRefed CacheEntry::ReopenTruncated(bool aMemoryOnly, + nsICacheEntryOpenCallback* aCallback) { LOG(("CacheEntry::ReopenTruncated [this=%p]", this)); @@ -413,7 +414,7 @@ already_AddRefed 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 handle = ReopenTruncated(nullptr); + nsRefPtr handle = ReopenTruncated(aMemoryOnly, nullptr); if (handle) { handle.forget(_retval); return NS_OK; diff --git a/netwerk/cache2/CacheEntry.h b/netwerk/cache2/CacheEntry.h index 604eecaabc3..9f9a71b6c8a 100644 --- a/netwerk/cache2/CacheEntry.h +++ b/netwerk/cache2/CacheEntry.h @@ -224,7 +224,8 @@ private: // Called only from DoomAlreadyRemoved() void DoomFile(); - already_AddRefed ReopenTruncated(nsICacheEntryOpenCallback* aCallback); + already_AddRefed ReopenTruncated(bool aMemoryOnly, + nsICacheEntryOpenCallback* aCallback); void TransferCallbacks(CacheEntry & aFromEntry); mozilla::Mutex mLock; diff --git a/netwerk/cache2/OldWrappers.cpp b/netwerk/cache2/OldWrappers.cpp index f92c49f5b90..dbd4687866e 100644 --- a/netwerk/cache2/OldWrappers.cpp +++ b/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) -{ - 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_IMETHODIMP _OldCacheEntryWrapper::Recreate(bool aMemoryOnly, + 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 self(this); self.forget(aResult); return NS_OK; diff --git a/netwerk/cache2/OldWrappers.h b/netwerk/cache2/OldWrappers.h index c7f94f088dd..e54ae0bea92 100644 --- a/netwerk/cache2/OldWrappers.h +++ b/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 SetPersistToDisk(bool aPersistToDisk); + NS_IMETHOD GetPersistent(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); diff --git a/netwerk/cache2/nsICacheEntry.idl b/netwerk/cache2/nsICacheEntry.idl index d8dd61b3c1f..926e1d2fae4 100644 --- a/netwerk/cache2/nsICacheEntry.idl +++ b/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. - * NOTE: This attribute must be set BEFORE opening the output stream. - * Switching this flag does not immediately affect creation of the disk - * file from memory-only data or eviction of the disk file and loading it - * to memory-only. + * Whether the entry is memory/only or persisted to disk. + * Note: private browsing entries are reported as persistent for consistency + * while are not actually persisted to disk. */ - 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. diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index 6320c585047..015ae075633 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/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 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; diff --git a/netwerk/test/unit/test_bug812167.js b/netwerk/test/unit/test_bug812167.js index c3ca52f7b1b..f220ac1e52f 100644 --- a/netwerk/test/unit/test_bug812167.js +++ b/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();