Unreviewed, rolling out r211869.
authorryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Feb 2017 17:52:47 +0000 (17:52 +0000)
committerryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Feb 2017 17:52:47 +0000 (17:52 +0000)
This change caused assertion failures on macOS WK2.

Reverted changeset:

"Allow speculative redirects"
https://bugs.webkit.org/show_bug.cgi?id=167982
http://trac.webkit.org/changeset/211869

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@211882 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 9d79a90..070e0e3 100644 (file)
@@ -1,3 +1,15 @@
+2017-02-08  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r211869.
+
+        This change caused assertion failures on macOS WK2.
+
+        Reverted changeset:
+
+        "Allow speculative redirects"
+        https://bugs.webkit.org/show_bug.cgi?id=167982
+        http://trac.webkit.org/changeset/211869
+
 2017-02-08  Eric Carlson  <eric.carlson@apple.com>
 
         Add WebRTC as an off-by-default experimental feature menu item.
index 718062a..48b904c 100644 (file)
@@ -476,8 +476,6 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
 
 #if ENABLE(NETWORK_CACHE)
     if (m_isWaitingContinueWillSendRequestForCachedRedirect) {
-        m_isWaitingContinueWillSendRequestForCachedRedirect = false;
-
         LOG(NetworkCache, "(NetworkProcess) Retrieving cached redirect");
 
         if (canUseCachedRedirect(newRequest))
@@ -485,6 +483,7 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
         else
             startNetworkLoad(newRequest);
 
+        m_isWaitingContinueWillSendRequestForCachedRedirect = false;
         return;
     }
 #endif
@@ -644,8 +643,6 @@ 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 ede21df..315e963 100644 (file)
@@ -395,11 +395,6 @@ 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());
@@ -459,7 +454,8 @@ std::unique_ptr<Entry> Cache::storeRedirect(const WebCore::ResourceRequest& requ
         return nullptr;
     }
 
-    auto cacheEntry = makeRedirectEntry(request, response, redirectRequest);
+    std::unique_ptr<Entry> cacheEntry = std::make_unique<Entry>(makeCacheKey(request), response, redirectRequest, WebCore::collectVaryingRequestHeaders(request, response));
+
     auto record = cacheEntry->encodeAsStorageRecord();
 
     m_storage->store(record, nullptr);
index 51d7762..ab8e273 100644 (file)
@@ -125,16 +125,11 @@ 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 ea23e1c..a94dfef 100644 (file)
@@ -52,10 +52,12 @@ Entry::Entry(const Key& key, const WebCore::ResourceResponse& response, const We
     , m_timeStamp(std::chrono::system_clock::now())
     , m_response(response)
     , m_varyingRequestHeaders(varyingRequestHeaders)
-    , m_redirectRequest(redirectRequest)
 {
     ASSERT(m_key.type() == "Resource");
     // Redirect body is not needed even if exists.
+
+    m_redirectRequest = std::make_unique<WebCore::ResourceRequest>();
+    m_redirectRequest->setAsIsolatedCopy(redirectRequest);
     m_redirectRequest->setHTTPBody(nullptr);
 }
 
@@ -64,7 +66,6 @@ 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)
 {
@@ -128,7 +129,7 @@ std::unique_ptr<Entry> Entry::decodeStorageRecord(const Storage::Record& storage
         return nullptr;
 
     if (isRedirect) {
-        entry->m_redirectRequest.emplace();
+        entry->m_redirectRequest = std::make_unique<WebCore::ResourceRequest>();
         if (!entry->m_redirectRequest->decodeWithoutPlatformData(decoder))
             return nullptr;
     }
index c039108..141e5cd 100644 (file)
@@ -59,7 +59,7 @@ public:
     const Vector<std::pair<String, String>>& varyingRequestHeaders() const { return m_varyingRequestHeaders; }
 
     WebCore::SharedBuffer* buffer() const;
-    const std::optional<WebCore::ResourceRequest>& redirectRequest() const { return m_redirectRequest; }
+    const WebCore::ResourceRequest* redirectRequest() const { return m_redirectRequest.get(); }
 
 #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::optional<WebCore::ResourceRequest> m_redirectRequest;
+    std::unique_ptr<WebCore::ResourceRequest> m_redirectRequest;
     mutable RefPtr<WebCore::SharedBuffer> m_buffer;
 #if ENABLE(SHAREABLE_RESOURCE)
     mutable ShareableResource::Handle m_shareableResourceHandle;
index e5a752d..db76618 100644 (file)
@@ -67,23 +67,11 @@ SpeculativeLoad::~SpeculativeLoad()
     ASSERT(!m_networkLoad);
 }
 
-void SpeculativeLoad::willSendRedirectedRequest(ResourceRequest&& request, ResourceRequest&& redirectRequest, ResourceResponse&& redirectResponse)
+void SpeculativeLoad::willSendRedirectedRequest(ResourceRequest&&, ResourceRequest&& redirectRequest, ResourceResponse&& redirectResponse)
 {
-    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({ });
+    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();
 }
 
 auto SpeculativeLoad::didReceiveResponse(ResourceResponse&& receivedResponse) -> ShouldContinueDidReceiveResponse
@@ -118,10 +106,8 @@ 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, m_bufferedDataForCache.copyRef(), [](auto& mappedBody) { });
+        m_cacheEntry = NetworkCache::singleton().store(m_originalRequest, m_response, WTFMove(m_bufferedDataForCache), [](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));
@@ -139,20 +125,24 @@ 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 3bc2d2e..6d045f6 100644 (file)
@@ -64,6 +64,7 @@ private:
     void didFinishLoading(double finishTime) override;
     void didFailLoading(const WebCore::ResourceError&) override;
 
+    void abort();
     void didComplete();
 
     GlobalFrameID m_frameID;
@@ -76,7 +77,6 @@ private:
 
     RefPtr<WebCore::SharedBuffer> m_bufferedDataForCache;
     std::unique_ptr<NetworkCache::Entry> m_cacheEntry;
-    bool m_didComplete { false };
 };
 
 } // namespace NetworkCache
index fcba7f1..9671188 100644 (file)
@@ -345,9 +345,7 @@ bool SpeculativeLoadManager::canRetrieve(const Key& storageKey, const WebCore::R
 void SpeculativeLoadManager::retrieve(const Key& storageKey, RetrieveCompletionHandler&& completionHandler)
 {
     if (auto preloadedEntry = m_preloadedEntries.take(storageKey)) {
-        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), cacheEntry = preloadedEntry->takeCacheEntry()] () mutable {
-            completionHandler(WTFMove(cacheEntry));
-        });
+        completionHandler(preloadedEntry->takeCacheEntry());
         return;
     }
     ASSERT(m_pendingPreloads.contains(storageKey));