Allow speculative redirects
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Feb 2017 21:11:32 +0000 (21:11 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Feb 2017 21:11:32 +0000 (21:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167982

Reviewed by Andreas Kling.

If speculative loader hits a redirect it will drop it on the floor. We should use it instead.

* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::continueWillSendRequest):
(WebKit::NetworkResourceLoader::dispatchWillSendRequestForCacheEntry):

    Reset m_isWaitingContinueWillSendRequestForCachedRedirect bit immediately.

* NetworkProcess/cache/NetworkCache.cpp:
(WebKit::NetworkCache::Cache::makeRedirectEntry):

    Factor to a function.

(WebKit::NetworkCache::Cache::storeRedirect):
* NetworkProcess/cache/NetworkCache.h:
(WebKit::NetworkCache::Cache::speculativeLoadManager):
* NetworkProcess/cache/NetworkCacheEntry.cpp:
(WebKit::NetworkCache::Entry::Entry):

    Use std::optional instead std::unique_ptr for the redirect request.

(WebKit::NetworkCache::Entry::decodeStorageRecord):
* NetworkProcess/cache/NetworkCacheEntry.h:
(WebKit::NetworkCache::Entry::redirectRequest):
* NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
(WebKit::NetworkCache::SpeculativeLoad::willSendRedirectedRequest):

    Make a cache entry for speculative redirect.
    Redirect is not actually performed, the target resource will have a separate
    speculative entry.

(WebKit::NetworkCache::SpeculativeLoad::didFinishLoading):
(WebKit::NetworkCache::SpeculativeLoad::didFailLoading):
(WebKit::NetworkCache::SpeculativeLoad::didComplete):

    Protect against multiple completions.

(WebKit::NetworkCache::SpeculativeLoad::abort): Deleted.
* NetworkProcess/cache/NetworkCacheSpeculativeLoad.h:
* NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
(WebKit::NetworkCache::SpeculativeLoadManager::retrieve):

    Make successful retrieves asynchronous to avoid re-entrancy problems.

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

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

index a4c3edd..b6d3d4a 100644 (file)
@@ -1,3 +1,54 @@
+2017-02-08  Antti Koivisto  <antti@apple.com>
+
+        Allow speculative redirects
+        https://bugs.webkit.org/show_bug.cgi?id=167982
+
+        Reviewed by Andreas Kling.
+
+        If speculative loader hits a redirect it will drop it on the floor. We should use it instead.
+
+        * NetworkProcess/NetworkResourceLoader.cpp:
+        (WebKit::NetworkResourceLoader::continueWillSendRequest):
+        (WebKit::NetworkResourceLoader::dispatchWillSendRequestForCacheEntry):
+
+            Reset m_isWaitingContinueWillSendRequestForCachedRedirect bit immediately.
+
+        * NetworkProcess/cache/NetworkCache.cpp:
+        (WebKit::NetworkCache::Cache::makeRedirectEntry):
+
+            Factor to a function.
+
+        (WebKit::NetworkCache::Cache::storeRedirect):
+        * NetworkProcess/cache/NetworkCache.h:
+        (WebKit::NetworkCache::Cache::speculativeLoadManager):
+        * NetworkProcess/cache/NetworkCacheEntry.cpp:
+        (WebKit::NetworkCache::Entry::Entry):
+
+            Use std::optional instead std::unique_ptr for the redirect request.
+
+        (WebKit::NetworkCache::Entry::decodeStorageRecord):
+        * NetworkProcess/cache/NetworkCacheEntry.h:
+        (WebKit::NetworkCache::Entry::redirectRequest):
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
+        (WebKit::NetworkCache::SpeculativeLoad::willSendRedirectedRequest):
+
+            Make a cache entry for speculative redirect.
+            Redirect is not actually performed, the target resource will have a separate
+            speculative entry.
+
+        (WebKit::NetworkCache::SpeculativeLoad::didFinishLoading):
+        (WebKit::NetworkCache::SpeculativeLoad::didFailLoading):
+        (WebKit::NetworkCache::SpeculativeLoad::didComplete):
+
+            Protect against multiple completions.
+
+        (WebKit::NetworkCache::SpeculativeLoad::abort): Deleted.
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoad.h:
+        * NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
+        (WebKit::NetworkCache::SpeculativeLoadManager::retrieve):
+
+            Make successful retrieves asynchronous to avoid re-entrancy problems.
+
 2017-02-08  Jer Noble  <jer.noble@apple.com>
 
         Unreviewed build fix; FullscreenClient only avaialble on 32-bit builds.
index 48b904c..718062a 100644 (file)
@@ -476,6 +476,8 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
 
 #if ENABLE(NETWORK_CACHE)
     if (m_isWaitingContinueWillSendRequestForCachedRedirect) {
+        m_isWaitingContinueWillSendRequestForCachedRedirect = false;
+
         LOG(NetworkCache, "(NetworkProcess) Retrieving cached redirect");
 
         if (canUseCachedRedirect(newRequest))
@@ -483,7 +485,6 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
         else
             startNetworkLoad(newRequest);
 
-        m_isWaitingContinueWillSendRequestForCachedRedirect = false;
         return;
     }
 #endif
@@ -643,6 +644,8 @@ void NetworkResourceLoader::validateCacheEntry(std::unique_ptr<NetworkCache::Ent
 void NetworkResourceLoader::dispatchWillSendRequestForCacheEntry(std::unique_ptr<NetworkCache::Entry> entry)
 {
     ASSERT(entry->redirectRequest());
+    ASSERT(!m_isWaitingContinueWillSendRequestForCachedRedirect);
+
     LOG(NetworkCache, "(NetworkProcess) Executing cached redirect");
 
     ++m_redirectCount;
index 315e963..ede21df 100644 (file)
@@ -395,6 +395,11 @@ std::unique_ptr<Entry> Cache::makeEntry(const WebCore::ResourceRequest& request,
     return std::make_unique<Entry>(makeCacheKey(request), response, WTFMove(responseData), WebCore::collectVaryingRequestHeaders(request, response));
 }
 
+std::unique_ptr<Entry> Cache::makeRedirectEntry(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, const WebCore::ResourceRequest& redirectRequest)
+{
+    return std::make_unique<Entry>(makeCacheKey(request), response, redirectRequest, WebCore::collectVaryingRequestHeaders(request, response));
+}
+
 std::unique_ptr<Entry> Cache::store(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, RefPtr<WebCore::SharedBuffer>&& responseData, Function<void (MappedBody&)>&& completionHandler)
 {
     ASSERT(isEnabled());
@@ -454,8 +459,7 @@ std::unique_ptr<Entry> Cache::storeRedirect(const WebCore::ResourceRequest& requ
         return nullptr;
     }
 
-    std::unique_ptr<Entry> cacheEntry = std::make_unique<Entry>(makeCacheKey(request), response, redirectRequest, WebCore::collectVaryingRequestHeaders(request, response));
-
+    auto cacheEntry = makeRedirectEntry(request, response, redirectRequest);
     auto record = cacheEntry->encodeAsStorageRecord();
 
     m_storage->store(record, nullptr);
index ab8e273..51d7762 100644 (file)
@@ -125,11 +125,16 @@ public:
     void storeData(const DataKey&,  const uint8_t* data, size_t);
     
     std::unique_ptr<Entry> makeEntry(const WebCore::ResourceRequest&, const WebCore::ResourceResponse&, RefPtr<WebCore::SharedBuffer>&&);
+    std::unique_ptr<Entry> makeRedirectEntry(const WebCore::ResourceRequest&, const WebCore::ResourceResponse&, const WebCore::ResourceRequest& redirectRequest);
 
     void dumpContentsToFile();
 
     String recordsPath() const;
 
+#if ENABLE(NETWORK_CACHE_SPECULATIVE_REVALIDATION)
+    SpeculativeLoadManager* speculativeLoadManager() { return m_speculativeLoadManager.get(); }
+#endif
+
 private:
     Cache() = default;
     ~Cache() = delete;
index a94dfef..5ee62b1 100644 (file)
@@ -54,10 +54,10 @@ Entry::Entry(const Key& key, const WebCore::ResourceResponse& response, const We
     , m_varyingRequestHeaders(varyingRequestHeaders)
 {
     ASSERT(m_key.type() == "Resource");
-    // Redirect body is not needed even if exists.
 
-    m_redirectRequest = std::make_unique<WebCore::ResourceRequest>();
+    m_redirectRequest.emplace();
     m_redirectRequest->setAsIsolatedCopy(redirectRequest);
+    // Redirect body is not needed even if exists.
     m_redirectRequest->setHTTPBody(nullptr);
 }
 
@@ -66,6 +66,7 @@ Entry::Entry(const Entry& other)
     , m_timeStamp(other.m_timeStamp)
     , m_response(other.m_response)
     , m_varyingRequestHeaders(other.m_varyingRequestHeaders)
+    , m_redirectRequest(other.m_redirectRequest)
     , m_buffer(other.m_buffer)
     , m_sourceStorageRecord(other.m_sourceStorageRecord)
 {
@@ -129,7 +130,7 @@ std::unique_ptr<Entry> Entry::decodeStorageRecord(const Storage::Record& storage
         return nullptr;
 
     if (isRedirect) {
-        entry->m_redirectRequest = std::make_unique<WebCore::ResourceRequest>();
+        entry->m_redirectRequest.emplace();
         if (!entry->m_redirectRequest->decodeWithoutPlatformData(decoder))
             return nullptr;
     }
index 141e5cd..c039108 100644 (file)
@@ -59,7 +59,7 @@ public:
     const Vector<std::pair<String, String>>& varyingRequestHeaders() const { return m_varyingRequestHeaders; }
 
     WebCore::SharedBuffer* buffer() const;
-    const WebCore::ResourceRequest* redirectRequest() const { return m_redirectRequest.get(); }
+    const std::optional<WebCore::ResourceRequest>& redirectRequest() const { return m_redirectRequest; }
 
 #if ENABLE(SHAREABLE_RESOURCE)
     ShareableResource::Handle& shareableResourceHandle() const;
@@ -83,7 +83,7 @@ private:
     WebCore::ResourceResponse m_response;
     Vector<std::pair<String, String>> m_varyingRequestHeaders;
 
-    std::unique_ptr<WebCore::ResourceRequest> m_redirectRequest;
+    std::optional<WebCore::ResourceRequest> m_redirectRequest;
     mutable RefPtr<WebCore::SharedBuffer> m_buffer;
 #if ENABLE(SHAREABLE_RESOURCE)
     mutable ShareableResource::Handle m_shareableResourceHandle;
index db76618..e5a752d 100644 (file)
@@ -67,11 +67,23 @@ SpeculativeLoad::~SpeculativeLoad()
     ASSERT(!m_networkLoad);
 }
 
-void SpeculativeLoad::willSendRedirectedRequest(ResourceRequest&&, ResourceRequest&& redirectRequest, ResourceResponse&& redirectResponse)
+void SpeculativeLoad::willSendRedirectedRequest(ResourceRequest&& request, ResourceRequest&& redirectRequest, ResourceResponse&& redirectResponse)
 {
-    LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculative revalidation for %s hit a redirect, aborting the load.", redirectResponse.url().string().utf8().data());
-    // We drop speculative revalidations if they redirect for now as we would need to notify WebCore of such redirects.
-    abort();
+    LOG(NetworkCacheSpeculativePreloading, "Speculative redirect %s -> %s", request.url().string().utf8().data(), redirectRequest.url().string().utf8().data());
+
+    m_cacheEntry = NetworkCache::singleton().storeRedirect(request, redirectResponse, redirectRequest);
+    // Create a synthetic cache entry if we can't store.
+    if (!m_cacheEntry)
+        m_cacheEntry = NetworkCache::singleton().makeRedirectEntry(request, redirectResponse, redirectRequest);
+
+    auto load = WTFMove(m_networkLoad);
+
+    // Don't follow the redirect. The redirect target will be registered for speculative load when it is loaded.
+    didComplete();
+
+    // This causes call to didFailLoading().
+    if (load)
+        load->continueWillSendRequest({ });
 }
 
 auto SpeculativeLoad::didReceiveResponse(ResourceResponse&& receivedResponse) -> ShouldContinueDidReceiveResponse
@@ -106,8 +118,10 @@ void SpeculativeLoad::didReceiveBuffer(Ref<SharedBuffer>&& buffer, int reportedE
 
 void SpeculativeLoad::didFinishLoading(double finishTime)
 {
+    if (m_didComplete)
+        return;
     if (!m_cacheEntry && m_bufferedDataForCache) {
-        m_cacheEntry = NetworkCache::singleton().store(m_originalRequest, m_response, WTFMove(m_bufferedDataForCache), [](auto& mappedBody) { });
+        m_cacheEntry = NetworkCache::singleton().store(m_originalRequest, m_response, m_bufferedDataForCache.copyRef(), [](auto& mappedBody) { });
         // Create a synthetic cache entry if we can't store.
         if (!m_cacheEntry && m_response.httpStatusCode() == 200)
             m_cacheEntry = NetworkCache::singleton().makeEntry(m_originalRequest, m_response, WTFMove(m_bufferedDataForCache));
@@ -125,24 +139,20 @@ void SpeculativeLoad::canAuthenticateAgainstProtectionSpaceAsync(const WebCore::
 
 void SpeculativeLoad::didFailLoading(const ResourceError&)
 {
+    if (m_didComplete)
+        return;
     m_cacheEntry = nullptr;
 
     didComplete();
 }
 
-void SpeculativeLoad::abort()
-{
-    if (m_networkLoad)
-        m_networkLoad->cancel();
-
-    m_cacheEntry = nullptr;
-    didComplete();
-}
-
 void SpeculativeLoad::didComplete()
 {
     RELEASE_ASSERT(RunLoop::isMain());
 
+    if (m_didComplete)
+        return;
+    m_didComplete = true;
     m_networkLoad = nullptr;
 
     // Make sure speculatively revalidated resources do not get validated by the NetworkResourceLoader again.
index 6d045f6..3bc2d2e 100644 (file)
@@ -64,7 +64,6 @@ private:
     void didFinishLoading(double finishTime) override;
     void didFailLoading(const WebCore::ResourceError&) override;
 
-    void abort();
     void didComplete();
 
     GlobalFrameID m_frameID;
@@ -77,6 +76,7 @@ private:
 
     RefPtr<WebCore::SharedBuffer> m_bufferedDataForCache;
     std::unique_ptr<NetworkCache::Entry> m_cacheEntry;
+    bool m_didComplete { false };
 };
 
 } // namespace NetworkCache
index 9671188..fcba7f1 100644 (file)
@@ -345,7 +345,9 @@ bool SpeculativeLoadManager::canRetrieve(const Key& storageKey, const WebCore::R
 void SpeculativeLoadManager::retrieve(const Key& storageKey, RetrieveCompletionHandler&& completionHandler)
 {
     if (auto preloadedEntry = m_preloadedEntries.take(storageKey)) {
-        completionHandler(preloadedEntry->takeCacheEntry());
+        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), cacheEntry = preloadedEntry->takeCacheEntry()] () mutable {
+            completionHandler(WTFMove(cacheEntry));
+        });
         return;
     }
     ASSERT(m_pendingPreloads.contains(storageKey));