Speculative loading sometimes happens too early and is missing login cookies
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Nov 2019 17:25:29 +0000 (17:25 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Nov 2019 17:25:29 +0000 (17:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204305
<rdar://problem/57063840>

Reviewed by Antti Koivisto.

Source/WebKit:

Speculative loads were issued before receiving the response from the main resource. However,
the main resource may set important cookies that are thus missing from the speculative requests.

To address the issue we now delay speculative loads for first-party subresources until we've
received the response from the main resource. To avoid regressing PLT, we still warm up the
first-party subresources from disk right away and preconnect to the server.

No new tests, extended existing test.

* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::didReceiveResponse):
(WebKit::NetworkResourceLoader::didReceiveMainResourceResponse):
(WebKit::NetworkResourceLoader::didRetrieveCacheEntry):
* NetworkProcess/NetworkResourceLoader.h:
* NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
(WebKit::NetworkCache::SpeculativeLoadManager::PendingFrameLoad::didReceiveMainResourceResponse const):
(WebKit::NetworkCache::SpeculativeLoadManager::PendingFrameLoad::markMainResourceResponseAsReceived):
(WebKit::NetworkCache::SpeculativeLoadManager::PendingFrameLoad::addPostMainResourceResponseTask):
(WebKit::NetworkCache::SpeculativeLoadManager::shouldRegisterLoad):
(WebKit::NetworkCache::SpeculativeLoadManager::registerLoad):
(WebKit::NetworkCache::SpeculativeLoadManager::registerMainResourceLoadResponse):
(WebKit::NetworkCache::SpeculativeLoadManager::preconnectForSubresource):
(WebKit::NetworkCache::SpeculativeLoadManager::revalidateSubresource):
* NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.h:
* NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp:
(WebKit::NetworkCache::SubresourceInfo::isFirstParty const):
* NetworkProcess/cache/NetworkCacheSubresourcesEntry.h:

LayoutTests:

Extend layout test coverage to make sure that the validation request contains the latest cookies
set by the main resource.

* http/tests/cache/disk-cache/speculative-validation/resources/validation-request-frame.php:
* http/tests/cache/disk-cache/speculative-validation/validation-request-expected.txt:
* http/tests/cache/disk-cache/speculative-validation/validation-request.html:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/disk-cache/speculative-validation/resources/validation-request-frame.php
LayoutTests/http/tests/cache/disk-cache/speculative-validation/validation-request-expected.txt
LayoutTests/http/tests/cache/disk-cache/speculative-validation/validation-request.html
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
Source/WebKit/NetworkProcess/NetworkResourceLoader.h
Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp
Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.h
Source/WebKit/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp
Source/WebKit/NetworkProcess/cache/NetworkCacheSubresourcesEntry.h

index a6f7317..87d8539 100644 (file)
@@ -1,3 +1,18 @@
+2019-11-22  Chris Dumez  <cdumez@apple.com>
+
+        Speculative loading sometimes happens too early and is missing login cookies
+        https://bugs.webkit.org/show_bug.cgi?id=204305
+        <rdar://problem/57063840>
+
+        Reviewed by Antti Koivisto.
+
+        Extend layout test coverage to make sure that the validation request contains the latest cookies
+        set by the main resource.
+
+        * http/tests/cache/disk-cache/speculative-validation/resources/validation-request-frame.php:
+        * http/tests/cache/disk-cache/speculative-validation/validation-request-expected.txt:
+        * http/tests/cache/disk-cache/speculative-validation/validation-request.html:
+
 2019-11-22  Per Arne Vollan  <pvollan@apple.com>
 
         Layout Test storage/indexeddb/modern/new-database-after-user-delete.html is flaky
index d88c966..4cb3621 100644 (file)
@@ -2,9 +2,15 @@
 header('Content-Type: text/html');
 header('Cache-Control: max-age=0');
 header('Etag: 123456789');
-
+$cookie = "speculativeRequestValidation=" . uniqid();
+header('Set-Cookie: ' . $cookie);
 ?>
 <!DOCTYPE html>
 <body>
 <script src="request-headers-script.php"></script>
+<script>
+<?php
+echo "sentSetCookieHeader = '" . $cookie . "';";
+?>
+</script>
 </body>
index ca48223..1c12eb0 100644 (file)
@@ -4,6 +4,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
 
 
 PASS validationRequestHeader('If-None-Match') is "123456789"
+PASS isCookieHeaderCorrect is true
 PASS validationRequestHeader('Accept') is initialHeaderValues['Accept']
 PASS validationRequestHeader('Accept-Encoding') is initialHeaderValues['Accept-Encoding']
 PASS validationRequestHeader('Accept-Language') is initialHeaderValues['Accept-Language']
index de6a58d..81c4f61 100644 (file)
@@ -37,6 +37,8 @@ function frameLoaded()
     if (state == "speculativeRevalidation") {
         // Validate the HTTP headers of the speculative validation request.
         shouldBeEqualToString("validationRequestHeader('If-None-Match')", "123456789");
+        isCookieHeaderCorrect = validationRequestHeader('Cookie') === document.getElementById("testFrame").contentWindow.sentSetCookieHeader;
+        shouldBeTrue("isCookieHeaderCorrect");
 
         for (var i = 0; i < headersToCheck.length; i++) {
             headerToCheck = headersToCheck[i];
index 1492cec..71993b3 100644 (file)
@@ -1,3 +1,39 @@
+2019-11-22  Chris Dumez  <cdumez@apple.com>
+
+        Speculative loading sometimes happens too early and is missing login cookies
+        https://bugs.webkit.org/show_bug.cgi?id=204305
+        <rdar://problem/57063840>
+
+        Reviewed by Antti Koivisto.
+
+        Speculative loads were issued before receiving the response from the main resource. However,
+        the main resource may set important cookies that are thus missing from the speculative requests.
+
+        To address the issue we now delay speculative loads for first-party subresources until we've
+        received the response from the main resource. To avoid regressing PLT, we still warm up the
+        first-party subresources from disk right away and preconnect to the server.
+
+        No new tests, extended existing test.
+
+        * NetworkProcess/NetworkResourceLoader.cpp:
+        (WebKit::NetworkResourceLoader::didReceiveResponse):
+        (WebKit::NetworkResourceLoader::didReceiveMainResourceResponse):
+        (WebKit::NetworkResourceLoader::didRetrieveCacheEntry):
+        * NetworkProcess/NetworkResourceLoader.h:
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
+        (WebKit::NetworkCache::SpeculativeLoadManager::PendingFrameLoad::didReceiveMainResourceResponse const):
+        (WebKit::NetworkCache::SpeculativeLoadManager::PendingFrameLoad::markMainResourceResponseAsReceived):
+        (WebKit::NetworkCache::SpeculativeLoadManager::PendingFrameLoad::addPostMainResourceResponseTask):
+        (WebKit::NetworkCache::SpeculativeLoadManager::shouldRegisterLoad):
+        (WebKit::NetworkCache::SpeculativeLoadManager::registerLoad):
+        (WebKit::NetworkCache::SpeculativeLoadManager::registerMainResourceLoadResponse):
+        (WebKit::NetworkCache::SpeculativeLoadManager::preconnectForSubresource):
+        (WebKit::NetworkCache::SpeculativeLoadManager::revalidateSubresource):
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.h:
+        * NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp:
+        (WebKit::NetworkCache::SubresourceInfo::isFirstParty const):
+        * NetworkProcess/cache/NetworkCacheSubresourcesEntry.h:
+
 2019-11-22  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK][WPE] RemoteInspector: use sockets instead of DBus
index c63c0a0..42086a0 100644 (file)
@@ -30,6 +30,7 @@
 #include "FormDataReference.h"
 #include "Logging.h"
 #include "NetworkCache.h"
+#include "NetworkCacheSpeculativeLoadManager.h"
 #include "NetworkConnectionToWebProcess.h"
 #include "NetworkConnectionToWebProcessMessages.h"
 #include "NetworkLoad.h"
@@ -466,6 +467,9 @@ void NetworkResourceLoader::didReceiveResponse(ResourceResponse&& receivedRespon
 {
     RELEASE_LOG_IF_ALLOWED("didReceiveResponse: (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ", httpStatusCode = %d, length = %" PRId64 ")", m_parameters.webPageID.toUInt64(), m_parameters.webFrameID.toUInt64(), m_parameters.identifier, receivedResponse.httpStatusCode(), receivedResponse.expectedContentLength());
 
+    if (isMainResource())
+        didReceiveMainResourceResponse(receivedResponse);
+
     m_response = WTFMove(receivedResponse);
 
     if (shouldCaptureExtraNetworkLoadMetrics() && m_networkLoadChecker) {
@@ -900,10 +904,21 @@ void NetworkResourceLoader::tryStoreAsCacheEntry()
     });
 }
 
+void NetworkResourceLoader::didReceiveMainResourceResponse(const WebCore::ResourceResponse& response)
+{
+#if ENABLE(NETWORK_CACHE_SPECULATIVE_REVALIDATION)
+    if (auto* speculativeLoadManager = m_cache ? m_cache->speculativeLoadManager() : nullptr)
+        speculativeLoadManager->registerMainResourceLoadResponse(globalFrameID(), originalRequest(), response);
+#endif
+}
+
 void NetworkResourceLoader::didRetrieveCacheEntry(std::unique_ptr<NetworkCache::Entry> entry)
 {
     auto response = entry->response();
 
+    if (isMainResource())
+        didReceiveMainResourceResponse(response);
+
     if (isMainResource() && shouldInterruptLoadForCSPFrameAncestorsOrXFrameOptions(response)) {
         response = sanitizeResponseIfPossible(WTFMove(response), ResourceResponse::SanitizationType::CrossOriginSafe);
         send(Messages::WebResourceLoader::StopLoadingAfterXFrameOptionsOrContentSecurityPolicyDenied { response });
index 66b2560..6890bf9 100644 (file)
@@ -155,6 +155,7 @@ private:
     void startNetworkLoad(WebCore::ResourceRequest&&, FirstLoad);
     void restartNetworkLoad(WebCore::ResourceRequest&&);
     void continueDidReceiveResponse();
+    void didReceiveMainResourceResponse(const WebCore::ResourceResponse&);
 
     enum class LoadResult {
         Unknown,
index 9806f62..67047bf 100644 (file)
@@ -33,6 +33,7 @@
 #include "NetworkCacheSpeculativeLoad.h"
 #include "NetworkCacheSubresourcesEntry.h"
 #include "NetworkProcess.h"
+#include "PreconnectTask.h"
 #include <WebCore/DiagnosticLoggingKeys.h>
 #include <pal/HysteresisActivity.h>
 #include <wtf/HashCountedSet.h>
@@ -203,6 +204,16 @@ public:
         saveToDiskIfReady();
     }
 
+    bool didReceiveMainResourceResponse() const { return m_didReceiveMainResourceResponse; }
+    void markMainResourceResponseAsReceived()
+    {
+        m_didReceiveMainResourceResponse = true;
+        for (auto& task : m_postMainResourceResponseTasks)
+            task();
+    }
+
+    void addPostMainResourceResponseTask(Function<void()>&& task) { m_postMainResourceResponseTasks.append(WTFMove(task)); }
+
 private:
     PendingFrameLoad(Storage& storage, const Key& mainResourceKey, WTF::Function<void()>&& loadCompletionHandler)
         : m_storage(storage)
@@ -242,8 +253,10 @@ private:
     WTF::Function<void()> m_loadCompletionHandler;
     PAL::HysteresisActivity m_loadHysteresisActivity;
     std::unique_ptr<SubresourcesEntry> m_existingEntry;
+    Vector<Function<void()>> m_postMainResourceResponseTasks;
     bool m_didFinishLoad { false };
     bool m_didRetrieveExistingEntry { false };
+    bool m_didReceiveMainResourceResponse { false };
 };
 
 SpeculativeLoadManager::SpeculativeLoadManager(Cache& cache, Storage& storage)
@@ -323,14 +336,21 @@ void SpeculativeLoadManager::retrieve(const Key& storageKey, RetrieveCompletionH
     addResult.iterator->value->append(WTFMove(completionHandler));
 }
 
+bool SpeculativeLoadManager::shouldRegisterLoad(const WebCore::ResourceRequest& request)
+{
+    if (request.httpMethod() != "GET")
+        return false;
+    if (!request.httpHeaderField(HTTPHeaderName::Range).isEmpty())
+        return false;
+    return true;
+}
+
 void SpeculativeLoadManager::registerLoad(const GlobalFrameID& frameID, const ResourceRequest& request, const Key& resourceKey)
 {
     ASSERT(RunLoop::isMain());
     ASSERT(request.url().protocolIsInHTTPFamily());
 
-    if (request.httpMethod() != "GET")
-        return;
-    if (!request.httpHeaderField(HTTPHeaderName::Range).isEmpty())
+    if (!shouldRegisterLoad(request))
         return;
 
     auto isMainResource = request.requester() == ResourceRequest::Requester::Main;
@@ -362,6 +382,18 @@ void SpeculativeLoadManager::registerLoad(const GlobalFrameID& frameID, const Re
         pendingFrameLoad->registerSubresourceLoad(request, resourceKey);
 }
 
+void SpeculativeLoadManager::registerMainResourceLoadResponse(const GlobalFrameID& frameID, const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response)
+{
+    if (!shouldRegisterLoad(request))
+        return;
+
+    if (response.isRedirection())
+        return;
+
+    if (auto* pendingFrameLoad = m_pendingFrameLoads.get(frameID))
+        pendingFrameLoad->markMainResourceResponseAsReceived();
+}
+
 void SpeculativeLoadManager::addPreloadedEntry(std::unique_ptr<Entry> entry, const GlobalFrameID& frameID, Optional<ResourceRequest>&& revalidationRequest)
 {
     ASSERT(entry);
@@ -417,6 +449,26 @@ bool SpeculativeLoadManager::satisfyPendingRequests(const Key& key, Entry* entry
     return true;
 }
 
+void SpeculativeLoadManager::preconnectForSubresource(const SubresourceInfo& subresourceInfo, Entry* entry, const GlobalFrameID& frameID)
+{
+#if ENABLE(SERVER_PRECONNECT)
+    NetworkLoadParameters parameters;
+    parameters.webPageProxyID = frameID.webPageProxyID;
+    parameters.webPageID = frameID.webPageID;
+    parameters.webFrameID = frameID.frameID;
+    parameters.storedCredentialsPolicy = StoredCredentialsPolicy::Use;
+    parameters.contentSniffingPolicy = ContentSniffingPolicy::DoNotSniffContent;
+    parameters.contentEncodingSniffingPolicy = ContentEncodingSniffingPolicy::Sniff;
+    parameters.shouldPreconnectOnly = PreconnectOnly::Yes;
+    parameters.request = constructRevalidationRequest(subresourceInfo.key(), subresourceInfo, entry);
+    new PreconnectTask(m_cache.networkProcess(), m_cache.sessionID(), WTFMove(parameters), [](const WebCore::ResourceError&) { });
+#else
+    UNUSED_PARAM(subresourceInfo);
+    UNUSED_PARAM(entry);
+    UNUSED_PARAM(frameID);
+#endif
+}
+
 void SpeculativeLoadManager::revalidateSubresource(const SubresourceInfo& subresourceInfo, std::unique_ptr<Entry> entry, const GlobalFrameID& frameID)
 {
     ASSERT(!entry || entry->needsValidation());
@@ -427,6 +479,18 @@ void SpeculativeLoadManager::revalidateSubresource(const SubresourceInfo& subres
     if (!key.range().isEmpty())
         return;
 
+    auto* pendingLoad = m_pendingFrameLoads.get(frameID);
+
+    // Delay first-party speculative loads until we've received the response for the main resource, in case the main resource
+    // response sets cookies that are needed for subsequent loads.
+    if (pendingLoad && !pendingLoad->didReceiveMainResourceResponse() && subresourceInfo.isFirstParty()) {
+        preconnectForSubresource(subresourceInfo, entry.get(), frameID);
+        pendingLoad->addPostMainResourceResponseTask([this, subresourceInfo, entry = WTFMove(entry), frameID]() mutable {
+            revalidateSubresource(subresourceInfo, WTFMove(entry), frameID);
+        });
+        return;
+    }
+
     ResourceRequest revalidationRequest = constructRevalidationRequest(key, subresourceInfo, entry.get());
 
     LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculatively revalidating '%s':", key.identifier().utf8().data());
index 3af4fdb..29f7609 100644 (file)
@@ -50,6 +50,7 @@ public:
     ~SpeculativeLoadManager();
 
     void registerLoad(const GlobalFrameID&, const WebCore::ResourceRequest&, const Key& resourceKey);
+    void registerMainResourceLoadResponse(const GlobalFrameID&, const WebCore::ResourceRequest&, const WebCore::ResourceResponse&);
 
     typedef Function<void (std::unique_ptr<Entry>)> RetrieveCompletionHandler;
 
@@ -59,10 +60,12 @@ public:
 private:
     class PreloadedEntry;
 
+    static bool shouldRegisterLoad(const WebCore::ResourceRequest&);
     void addPreloadedEntry(std::unique_ptr<Entry>, const GlobalFrameID&, Optional<WebCore::ResourceRequest>&& revalidationRequest = WTF::nullopt);
     void preloadEntry(const Key&, const SubresourceInfo&, const GlobalFrameID&);
     void retrieveEntryFromStorage(const SubresourceInfo&, RetrieveCompletionHandler&&);
     void revalidateSubresource(const SubresourceInfo&, std::unique_ptr<Entry>, const GlobalFrameID&);
+    void preconnectForSubresource(const SubresourceInfo&, Entry*, const GlobalFrameID&);
     bool satisfyPendingRequests(const Key&, Entry*);
     void retrieveSubresourcesEntry(const Key& storageKey, WTF::Function<void (std::unique_ptr<SubresourcesEntry>)>&&);
     void startSpeculativeRevalidation(const GlobalFrameID&, SubresourcesEntry&);
index 63ad040..0c896e0 100644 (file)
@@ -81,6 +81,12 @@ bool SubresourceInfo::decode(WTF::Persistence::Decoder& decoder, SubresourceInfo
     return true;
 }
 
+bool SubresourceInfo::isFirstParty() const
+{
+    RegistrableDomain firstPartyDomain { m_firstPartyForCookies };
+    return firstPartyDomain.matches(URL(URL(), key().identifier()));
+}
+
 Storage::Record SubresourcesEntry::encodeAsStorageRecord() const
 {
     WTF::Persistence::Encoder encoder;
index 43921a0..6fe9530 100644 (file)
@@ -58,6 +58,8 @@ public:
 
     void setNonTransient() { m_isTransient = false; }
 
+    bool isFirstParty() const;
+
 private:
     Key m_key;
     WallTime m_lastSeen;