Speculative disk cache resource revalidations are sometimes wasted
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Mar 2016 21:22:36 +0000 (21:22 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Mar 2016 21:22:36 +0000 (21:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155187
<rdar://problem/25032905>

Reviewed by Antti Koivisto.

Speculative disk cache resource revalidations were sometimes wasted.

We would sometimes correctly revalidate a resource but the
NetworkResourceLoader then either:
1. Fail to reuse the speculatively validated entry
2. Reuse the speculatively validated entry but then validate it again

Bug 1 was caused by the revalidated entry key sometimes being
different from the cached entry key. This could happen when
revalidation fails (the server did not send back a 304) in
which case we call NetworkCache::store() which creates a new
cache Entry, generating a cache key from our revalidation
request. If the original request has a cache partition or a
range, then the keys would not match because we did not set
the cache partition or the range on the revalidation request.
This has been addressed by setting the cache partition on the
revalidation request in constructRevalidationRequest() and by
not doing revalidation if the original request had a 'range'
header.

Bug 2 was caused by us marking a speculatively revalidated entry
as "not needing revalidating" only in Cache::update(). Cache::update()
is only called in the case the revalidation was successful (server
returned a 304). If revalidation was not successful, Cache::store()
would be called instead was we would fail to update the
needsRevalidation flag. NetworkResourceLoader would then validate
again the resource that was already speculatively revalidated.
To address the problem, we now update the 'needsRevalidation' flag
as soon as the speculative revalidation completes, in
SpeculativeLoad::didComplete().

* NetworkProcess/cache/NetworkCache.cpp:
(WebKit::NetworkCache::Cache::retrieve):
(WebKit::NetworkCache::makeCacheKey):
(WebKit::NetworkCache::Cache::update):
* NetworkProcess/cache/NetworkCacheEntry.cpp:
(WebKit::NetworkCache::Entry::setNeedsValidation):
* NetworkProcess/cache/NetworkCacheEntry.h:
* NetworkProcess/cache/NetworkCacheKey.cpp:
(WebKit::NetworkCache::noPartitionString):
(WebKit::NetworkCache::Key::Key):
(WebKit::NetworkCache::Key::hasPartition):
* NetworkProcess/cache/NetworkCacheKey.h:
* NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
(WebKit::NetworkCache::SpeculativeLoad::didComplete):
* NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
(WebKit::NetworkCache::constructRevalidationRequest):
(WebKit::NetworkCache::SpeculativeLoadManager::retrieveEntryFromStorage):
(WebKit::NetworkCache::SpeculativeLoadManager::revalidateEntry):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@197879 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h
Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h
Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp

index 36321a6..793c16a 100644 (file)
@@ -1,3 +1,61 @@
+2016-03-09  Chris Dumez  <cdumez@apple.com>
+
+        Speculative disk cache resource revalidations are sometimes wasted
+        https://bugs.webkit.org/show_bug.cgi?id=155187
+        <rdar://problem/25032905>
+
+        Reviewed by Antti Koivisto.
+
+        Speculative disk cache resource revalidations were sometimes wasted.
+
+        We would sometimes correctly revalidate a resource but the
+        NetworkResourceLoader then either:
+        1. Fail to reuse the speculatively validated entry
+        2. Reuse the speculatively validated entry but then validate it again
+
+        Bug 1 was caused by the revalidated entry key sometimes being
+        different from the cached entry key. This could happen when
+        revalidation fails (the server did not send back a 304) in
+        which case we call NetworkCache::store() which creates a new
+        cache Entry, generating a cache key from our revalidation
+        request. If the original request has a cache partition or a
+        range, then the keys would not match because we did not set
+        the cache partition or the range on the revalidation request.
+        This has been addressed by setting the cache partition on the
+        revalidation request in constructRevalidationRequest() and by
+        not doing revalidation if the original request had a 'range'
+        header.
+
+        Bug 2 was caused by us marking a speculatively revalidated entry
+        as "not needing revalidating" only in Cache::update(). Cache::update()
+        is only called in the case the revalidation was successful (server
+        returned a 304). If revalidation was not successful, Cache::store()
+        would be called instead was we would fail to update the
+        needsRevalidation flag. NetworkResourceLoader would then validate
+        again the resource that was already speculatively revalidated.
+        To address the problem, we now update the 'needsRevalidation' flag
+        as soon as the speculative revalidation completes, in
+        SpeculativeLoad::didComplete().
+
+        * NetworkProcess/cache/NetworkCache.cpp:
+        (WebKit::NetworkCache::Cache::retrieve):
+        (WebKit::NetworkCache::makeCacheKey):
+        (WebKit::NetworkCache::Cache::update):
+        * NetworkProcess/cache/NetworkCacheEntry.cpp:
+        (WebKit::NetworkCache::Entry::setNeedsValidation):
+        * NetworkProcess/cache/NetworkCacheEntry.h:
+        * NetworkProcess/cache/NetworkCacheKey.cpp:
+        (WebKit::NetworkCache::noPartitionString):
+        (WebKit::NetworkCache::Key::Key):
+        (WebKit::NetworkCache::Key::hasPartition):
+        * NetworkProcess/cache/NetworkCacheKey.h:
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
+        (WebKit::NetworkCache::SpeculativeLoad::didComplete):
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
+        (WebKit::NetworkCache::constructRevalidationRequest):
+        (WebKit::NetworkCache::SpeculativeLoadManager::retrieveEntryFromStorage):
+        (WebKit::NetworkCache::SpeculativeLoadManager::revalidateEntry):
+
 2016-03-09  Brent Fulgham  <bfulgham@apple.com>
 
         Local HTML should be blocked from localStorage access unless "Disable Local File Restrictions" is checked
index 2731780..c5b2a23 100644 (file)
@@ -121,8 +121,6 @@ static Key makeCacheKey(const WebCore::ResourceRequest& request)
 #else
     String partition;
 #endif
-    if (partition.isEmpty())
-        partition = ASCIILiteral("No partition");
 
     // FIXME: This implements minimal Range header disk cache support. We don't parse
     // ranges so only the same exact range request will be served from the cache.
@@ -402,7 +400,7 @@ void Cache::retrieve(const WebCore::ResourceRequest& request, const GlobalFrameI
         case UseDecision::Use:
             break;
         case UseDecision::Validate:
-            entry->setNeedsValidation();
+            entry->setNeedsValidation(true);
             break;
         default:
             entry = nullptr;
@@ -495,7 +493,6 @@ std::unique_ptr<Entry> Cache::update(const WebCore::ResourceRequest& originalReq
 
     WebCore::ResourceResponse response = existingEntry.response();
     WebCore::updateResponseHeadersAfterRevalidation(response, validatingResponse);
-    response.setSource(WebCore::ResourceResponse::Source::DiskCache);
 
     auto updateEntry = std::make_unique<Entry>(existingEntry.key(), response, existingEntry.buffer(), collectVaryingRequestHeaders(originalRequest, response));
     auto updateRecord = updateEntry->encodeAsStorageRecord();
index faa4f04..f74cecf 100644 (file)
@@ -188,10 +188,9 @@ bool Entry::needsValidation() const
     return m_response.source() == WebCore::ResourceResponse::Source::DiskCacheAfterValidation;
 }
 
-void Entry::setNeedsValidation()
+void Entry::setNeedsValidation(bool value)
 {
-    ASSERT(m_response.source() == WebCore::ResourceResponse::Source::DiskCache);
-    m_response.setSource(WebCore::ResourceResponse::Source::DiskCacheAfterValidation);
+    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);
 }
 
 void Entry::asJSON(StringBuilder& json, const Storage::RecordInfo& info) const
index 0855a4e..141e5cd 100644 (file)
@@ -66,7 +66,7 @@ public:
 #endif
 
     bool needsValidation() const;
-    void setNeedsValidation();
+    void setNeedsValidation(bool);
 
     const Storage::Record& sourceStorageRecord() const { return m_sourceStorageRecord; }
 
index 1f06a65..ab58ca6 100644 (file)
 
 #include "NetworkCacheCoders.h"
 #include <wtf/ASCIICType.h>
+#include <wtf/NeverDestroyed.h>
 #include <wtf/text/CString.h>
 #include <wtf/text/StringBuilder.h>
 
 namespace WebKit {
 namespace NetworkCache {
 
+static const String& noPartitionString()
+{
+    static NeverDestroyed<String> noPartition(ASCIILiteral("No partition"));
+    return noPartition;
+}
+
 Key::Key(const Key& o)
     : m_partition(o.m_partition.isolatedCopy())
     , m_type(o.m_type.isolatedCopy())
@@ -46,7 +53,7 @@ Key::Key(const Key& o)
 }
 
 Key::Key(const String& partition, const String& type, const String& range, const String& identifier)
-    : m_partition(partition.isolatedCopy())
+    : m_partition(partition.isEmpty() ? noPartitionString().isolatedCopy() : partition.isolatedCopy())
     , m_type(type.isolatedCopy())
     , m_identifier(identifier.isolatedCopy())
     , m_range(range.isolatedCopy())
@@ -59,6 +66,11 @@ Key::Key(WTF::HashTableDeletedValueType)
 {
 }
 
+bool Key::hasPartition() const
+{
+    return m_partition != noPartitionString();
+}
+
 Key& Key::operator=(const Key& other)
 {
     m_partition = other.m_partition.isolatedCopy();
index 12a7007..d1ff4d7 100644 (file)
@@ -54,6 +54,7 @@ public:
 
     bool isNull() const { return m_identifier.isNull(); }
 
+    bool hasPartition() const;
     const String& partition() const { return m_partition; }
     const String& identifier() const { return m_identifier; }
     const String& type() const { return m_type; }
index f6f5940..9b93fb5 100644 (file)
@@ -139,6 +139,10 @@ void SpeculativeLoad::didComplete()
 
     m_networkLoad = nullptr;
 
+    // Make sure speculatively revalidated resources do not get validated by the NetworkResourceLoader again.
+    if (m_cacheEntryForValidation)
+        m_cacheEntryForValidation->setNeedsValidation(false);
+
     m_completionHandler(WTFMove(m_cacheEntryForValidation));
 }
 
index 0275911..fad2515 100644 (file)
@@ -87,6 +87,11 @@ static inline Key makeSubresourcesKey(const Key& resourceKey)
 static inline ResourceRequest constructRevalidationRequest(const Entry& entry)
 {
     ResourceRequest revalidationRequest(entry.key().identifier());
+#if ENABLE(CACHE_PARTITIONING)
+    if (entry.key().hasPartition())
+        revalidationRequest.setCachePartition(entry.key().partition());
+#endif
+    ASSERT_WITH_MESSAGE(entry.key().range().isEmpty(), "range is not supported");
 
     String eTag = entry.response().httpHeaderField(HTTPHeaderName::ETag);
     if (!eTag.isEmpty())
@@ -344,7 +349,7 @@ void SpeculativeLoadManager::retrieveEntryFromStorage(const Key& key, const Retr
         }
 
         if (responseNeedsRevalidation(response, entry->timeStamp()))
-            entry->setNeedsValidation();
+            entry->setNeedsValidation(true);
 
         completionHandler(WTFMove(entry));
         return true;
@@ -369,9 +374,16 @@ void SpeculativeLoadManager::revalidateEntry(std::unique_ptr<Entry> entry, const
     ASSERT(entry->needsValidation());
 
     auto key = entry->key();
+
+    // Range is not supported.
+    if (!key.range().isEmpty())
+        return;
+
     LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculatively revalidating '%s':", key.identifier().utf8().data());
     auto revalidator = std::make_unique<SpeculativeLoad>(frameID, constructRevalidationRequest(*entry), WTFMove(entry), [this, key, frameID](std::unique_ptr<Entry> revalidatedEntry) {
         ASSERT(!revalidatedEntry || !revalidatedEntry->needsValidation());
+        ASSERT(!revalidatedEntry || revalidatedEntry->key() == key);
+
         auto protectRevalidator = m_pendingPreloads.take(key);
         LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculative revalidation completed for '%s':", key.identifier().utf8().data());