Title: [229181] trunk
Revision
229181
Author
commit-qu...@webkit.org
Date
2018-03-02 10:29:42 -0800 (Fri, 02 Mar 2018)

Log Message

Loads for a Document controlled by a Service Worker should not use AppCache
https://bugs.webkit.org/show_bug.cgi?id=183148

Patch by Youenn Fablet <you...@apple.com> on 2018-03-02
Reviewed by Chris Dumez.

LayoutTests/imported/w3c:

* web-platform-tests/service-workers/service-worker/appcache-ordering-main.https-expected.txt:

Source/WebCore:

Covered by updated test.

Postponing document loading through app cache after matching service worker registration.
Trying to load through app cache only if there is no service worker registration.

Disabling app cache for any load that has a service worker registration identifier.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::redirectReceived):
(WebCore::DocumentLoader::willSendRequest):
(WebCore::DocumentLoader::tryLoadingRequestFromApplicationCache):
(WebCore::DocumentLoader::tryLoadingRedirectRequestFromApplicationCache):
(WebCore::DocumentLoader::restartLoadingDueToServiceWorkerRegistrationChange):
(WebCore::DocumentLoader::scheduleSubstituteResourceLoad):
(WebCore::DocumentLoader::startLoadingMainResource):
* loader/DocumentLoader.h:
* loader/appcache/ApplicationCacheHost.cpp:
(WebCore::ApplicationCacheHost::maybeLoadMainResource):
(WebCore::ApplicationCacheHost::maybeLoadMainResourceForRedirect):
(WebCore::ApplicationCacheHost::maybeLoadResource):
(WebCore::ApplicationCacheHost::scheduleLoadFallbackResourceFromApplicationCache):
* loader/appcache/ApplicationCacheHost.h:

Modified Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (229180 => 229181)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2018-03-02 18:29:42 UTC (rev 229181)
@@ -1,3 +1,12 @@
+2018-03-02  Youenn Fablet  <you...@apple.com>
+
+        Loads for a Document controlled by a Service Worker should not use AppCache
+        https://bugs.webkit.org/show_bug.cgi?id=183148
+
+        Reviewed by Chris Dumez.
+
+        * web-platform-tests/service-workers/service-worker/appcache-ordering-main.https-expected.txt:
+
 2018-02-28  Youenn Fablet  <you...@apple.com>
 
         Make LayoutTests wait_for_state fail after a given period of time

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/appcache-ordering-main.https-expected.txt (229180 => 229181)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/appcache-ordering-main.https-expected.txt	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/appcache-ordering-main.https-expected.txt	2018-03-02 18:29:42 UTC (rev 229181)
@@ -1,4 +1,4 @@
 CONSOLE MESSAGE: line 1: ApplicationCache is deprecated. Please use ServiceWorkers instead.
 
-FAIL serviceworkers take priority over appcaches assert_unreached: unexpected rejection: assert_false: but serviceworkers should take priority expected false got true Reached unreachable code
+PASS serviceworkers take priority over appcaches 
 

Modified: trunk/Source/WebCore/ChangeLog (229180 => 229181)


--- trunk/Source/WebCore/ChangeLog	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/Source/WebCore/ChangeLog	2018-03-02 18:29:42 UTC (rev 229181)
@@ -1,3 +1,33 @@
+2018-03-02  Youenn Fablet  <you...@apple.com>
+
+        Loads for a Document controlled by a Service Worker should not use AppCache
+        https://bugs.webkit.org/show_bug.cgi?id=183148
+
+        Reviewed by Chris Dumez.
+
+        Covered by updated test.
+
+        Postponing document loading through app cache after matching service worker registration.
+        Trying to load through app cache only if there is no service worker registration.
+
+        Disabling app cache for any load that has a service worker registration identifier.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::redirectReceived):
+        (WebCore::DocumentLoader::willSendRequest):
+        (WebCore::DocumentLoader::tryLoadingRequestFromApplicationCache):
+        (WebCore::DocumentLoader::tryLoadingRedirectRequestFromApplicationCache):
+        (WebCore::DocumentLoader::restartLoadingDueToServiceWorkerRegistrationChange):
+        (WebCore::DocumentLoader::scheduleSubstituteResourceLoad):
+        (WebCore::DocumentLoader::startLoadingMainResource):
+        * loader/DocumentLoader.h:
+        * loader/appcache/ApplicationCacheHost.cpp:
+        (WebCore::ApplicationCacheHost::maybeLoadMainResource):
+        (WebCore::ApplicationCacheHost::maybeLoadMainResourceForRedirect):
+        (WebCore::ApplicationCacheHost::maybeLoadResource):
+        (WebCore::ApplicationCacheHost::scheduleLoadFallbackResourceFromApplicationCache):
+        * loader/appcache/ApplicationCacheHost.h:
+
 2018-03-02  Chris Dumez  <cdu...@apple.com>
 
         fast/events/before-unload-remove-itself.html crashes with async policy delegates

Modified: trunk/Source/WebCore/loader/DocumentLoader.cpp (229180 => 229181)


--- trunk/Source/WebCore/loader/DocumentLoader.cpp	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/Source/WebCore/loader/DocumentLoader.cpp	2018-03-02 18:29:42 UTC (rev 229181)
@@ -519,10 +519,12 @@
 #if ENABLE(SERVICE_WORKER)
     bool isRedirectionFromServiceWorker = redirectResponse.source() == ResourceResponse::Source::ServiceWorker;
     willSendRequest(WTFMove(request), redirectResponse, [isRedirectionFromServiceWorker, completionHandler = WTFMove(completionHandler), protectedThis = makeRef(*this), this] (auto&& request) mutable {
-        if (request.isNull() || !m_mainDocumentError.isNull() || !m_frame || m_substituteData.isValid()) {
+        ASSERT(!m_substituteData.isValid());
+        if (request.isNull() || !m_mainDocumentError.isNull() || !m_frame) {
             completionHandler({ });
             return;
         }
+
         auto url = ""
         this->matchRegistration(url, [request = WTFMove(request), isRedirectionFromServiceWorker, completionHandler = WTFMove(completionHandler), protectedThis = WTFMove(protectedThis), this] (auto&& registrationData) mutable {
             if (!m_mainDocumentError.isNull() || !m_frame) {
@@ -529,6 +531,12 @@
                 completionHandler({ });
                 return;
             }
+
+            if (!registrationData && this->tryLoadingRedirectRequestFromApplicationCache(request)) {
+                completionHandler({ });
+                return;
+            }
+
             bool shouldContinueLoad = areRegistrationsEqual(m_serviceWorkerRegistrationData, registrationData)
                 && isRedirectionFromServiceWorker == !!registrationData;
 
@@ -537,12 +545,8 @@
                 return;
             }
 
-            // Service worker registration changed, we need to cancel the current load to restart a new one.
-            this->clearMainResource();
+            this->restartLoadingDueToServiceWorkerRegistrationChange(WTFMove(request), WTFMove(registrationData));
             completionHandler({ });
-
-            m_serviceWorkerRegistrationData = WTFMove(registrationData);
-            this->loadMainResource(WTFMove(request));
             return;
         });
     });
@@ -625,17 +629,6 @@
 
     setRequest(newRequest);
 
-    if (didReceiveRedirectResponse) {
-        // We checked application cache for initial URL, now we need to check it for redirected one.
-        ASSERT(!m_substituteData.isValid());
-        m_applicationCacheHost->maybeLoadMainResourceForRedirect(newRequest, m_substituteData);
-        if (m_substituteData.isValid()) {
-            RELEASE_ASSERT(m_mainResource);
-            ResourceLoader* loader = m_mainResource->loader();
-            m_identifierForLoadWithoutResourceLoader = loader ? loader->identifier() : m_mainResource->identifierForLoadWithoutResourceLoader();
-        }
-    }
-
     // FIXME: Ideally we'd stop the I/O until we hear back from the navigation policy delegate
     // listener. But there's no way to do that in practice. So instead we cancel later if the
     // listener tells us to. In practice that means the navigation policy needs to be decided
@@ -646,41 +639,67 @@
     ASSERT(!m_waitingForNavigationPolicy);
     m_waitingForNavigationPolicy = true;
     frameLoader()->policyChecker().checkNavigationPolicy(ResourceRequest(newRequest), didReceiveRedirectResponse, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request, FormState*, bool shouldContinue) mutable {
-        continueAfterNavigationPolicy(request, shouldContinue);
+        m_waitingForNavigationPolicy = false;
+        if (!shouldContinue)
+            stopLoadingForPolicyChange();
         completionHandler(WTFMove(request));
     });
 }
 
-void DocumentLoader::continueAfterNavigationPolicy(const ResourceRequest&, bool shouldContinue)
+bool DocumentLoader::tryLoadingRequestFromApplicationCache()
 {
-    ASSERT(m_waitingForNavigationPolicy);
-    m_waitingForNavigationPolicy = false;
-    if (!shouldContinue)
-        stopLoadingForPolicyChange();
-    else if (m_substituteData.isValid()) {
-        // A redirect resulted in loading substitute data.
-        ASSERT(timing().redirectCount());
+    m_applicationCacheHost->maybeLoadMainResource(m_request, m_substituteData);
 
-        // We need to remove our reference to the CachedResource in favor of a SubstituteData load.
-        // This will probably trigger the cancellation of the CachedResource's underlying ResourceLoader, though there is a
-        // small chance that the resource is being loaded by a different Frame, preventing the ResourceLoader from being cancelled.
-        // If the ResourceLoader is indeed cancelled, it would normally send resource load callbacks.
-        // However, from an API perspective, this isn't a cancellation. Therefore, sever our relationship with the network load,
-        // but prevent the ResourceLoader from sending ResourceLoadNotifier callbacks.
-        RefPtr<ResourceLoader> resourceLoader = mainResourceLoader();
-        if (resourceLoader) {
-            ASSERT(resourceLoader->shouldSendResourceLoadCallbacks());
-            resourceLoader->setSendCallbackPolicy(DoNotSendCallbacks);
-        }
+    if (!m_substituteData.isValid() || !m_frame->page())
+        return false;
 
-        clearMainResource();
+    RELEASE_LOG_IF_ALLOWED("startLoadingMainResource: Returning cached main resource (frame = %p, main = %d)", m_frame, m_frame->isMainFrame());
+    m_identifierForLoadWithoutResourceLoader = m_frame->page()->progress().createUniqueIdentifier();
+    frameLoader()->notifier().assignIdentifierToInitialRequest(m_identifierForLoadWithoutResourceLoader, this, m_request);
+    frameLoader()->notifier().dispatchWillSendRequest(this, m_identifierForLoadWithoutResourceLoader, m_request, ResourceResponse());
+    handleSubstituteDataLoadSoon();
+    return true;
+}
 
-        if (resourceLoader)
-            resourceLoader->setSendCallbackPolicy(SendCallbacks);
-        handleSubstituteDataLoadSoon();
+bool DocumentLoader::tryLoadingRedirectRequestFromApplicationCache(const ResourceRequest& request)
+{
+    m_applicationCacheHost->maybeLoadMainResourceForRedirect(request, m_substituteData);
+    if (!m_substituteData.isValid())
+        return false;
+
+    RELEASE_ASSERT(m_mainResource);
+    auto* loader = m_mainResource->loader();
+    m_identifierForLoadWithoutResourceLoader = loader ? loader->identifier() : m_mainResource->identifierForLoadWithoutResourceLoader();
+
+    // We need to remove our reference to the CachedResource in favor of a SubstituteData load, which can triger the cancellation of the underyling ResourceLoader.
+    // If the ResourceLoader is indeed cancelled, it would normally send resource load callbacks.
+    // Therefore, sever our relationship with the network load but prevent the ResourceLoader from sending ResourceLoadNotifier callbacks.
+
+    auto resourceLoader = makeRefPtr(mainResourceLoader());
+    if (resourceLoader) {
+        ASSERT(resourceLoader->shouldSendResourceLoadCallbacks());
+        resourceLoader->setSendCallbackPolicy(DoNotSendCallbacks);
     }
+
+    clearMainResource();
+
+    if (resourceLoader)
+        resourceLoader->setSendCallbackPolicy(SendCallbacks);
+
+    handleSubstituteDataLoadNow();
+    return true;
 }
 
+#if ENABLE(SERVICE_WORKER)
+void DocumentLoader::restartLoadingDueToServiceWorkerRegistrationChange(ResourceRequest&& request, std::optional<ServiceWorkerRegistrationData>&& registrationData)
+{
+    clearMainResource();
+
+    m_serviceWorkerRegistrationData = WTFMove(registrationData);
+    loadMainResource(WTFMove(request));
+}
+#endif
+
 void DocumentLoader::stopLoadingAfterXFrameOptionsOrContentSecurityPolicyDenied(unsigned long identifier, const ResourceResponse& response)
 {
     InspectorInstrumentation::continueAfterXFrameOptionsDenied(*m_frame, identifier, *this, response);
@@ -1435,6 +1454,9 @@
 
 void DocumentLoader::scheduleSubstituteResourceLoad(ResourceLoader& loader, SubstituteResource& resource)
 {
+#if ENABLE(SERVICE_WORKER)
+    ASSERT(!loader.options().serviceWorkerRegistrationIdentifier);
+#endif
     m_pendingSubstituteResources.set(&loader, &resource);
     deliverSubstituteResourcesAfterDelay();
 }
@@ -1647,17 +1669,6 @@
             return;
         }
 
-        m_applicationCacheHost->maybeLoadMainResource(m_request, m_substituteData);
-
-        if (m_substituteData.isValid() && m_frame->page()) {
-            RELEASE_LOG_IF_ALLOWED("startLoadingMainResource: Returning cached main resource (frame = %p, main = %d)", m_frame, m_frame->isMainFrame());
-            m_identifierForLoadWithoutResourceLoader = m_frame->page()->progress().createUniqueIdentifier();
-            frameLoader()->notifier().assignIdentifierToInitialRequest(m_identifierForLoadWithoutResourceLoader, this, m_request);
-            frameLoader()->notifier().dispatchWillSendRequest(this, m_identifierForLoadWithoutResourceLoader, m_request, ResourceResponse());
-            handleSubstituteDataLoadSoon();
-            return;
-        }
-
         request.setRequester(ResourceRequest::Requester::Main);
         // If this is a reload the cache layer might have made the previous request conditional. DocumentLoader can't handle 304 responses itself.
         request.makeUnconditional();
@@ -1672,9 +1683,13 @@
                 return;
 
             m_serviceWorkerRegistrationData = WTFMove(registrationData);
+            if (!m_serviceWorkerRegistrationData && tryLoadingRequestFromApplicationCache())
+                return;
             this->loadMainResource(WTFMove(request));
         });
 #else
+        if (tryLoadingRequestFromApplicationCache())
+            return;
         loadMainResource(WTFMove(request));
 #endif
     });

Modified: trunk/Source/WebCore/loader/DocumentLoader.h (229180 => 229181)


--- trunk/Source/WebCore/loader/DocumentLoader.h	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/Source/WebCore/loader/DocumentLoader.h	2018-03-02 18:29:42 UTC (rev 229181)
@@ -367,7 +367,11 @@
     bool isMultipartReplacingLoad() const;
     bool isPostOrRedirectAfterPost(const ResourceRequest&, const ResourceResponse&);
 
-    void continueAfterNavigationPolicy(const ResourceRequest&, bool shouldContinue);
+    bool tryLoadingRequestFromApplicationCache();
+    bool tryLoadingRedirectRequestFromApplicationCache(const ResourceRequest&);
+#if ENABLE(SERVICE_WORKER)
+    void restartLoadingDueToServiceWorkerRegistrationChange(ResourceRequest&&, std::optional<ServiceWorkerRegistrationData>&&);
+#endif
     void continueAfterContentPolicy(PolicyAction);
 
     void stopLoadingForPolicyChange();

Modified: trunk/Source/WebCore/loader/appcache/ApplicationCacheHost.cpp (229180 => 229181)


--- trunk/Source/WebCore/loader/appcache/ApplicationCacheHost.cpp	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/Source/WebCore/loader/appcache/ApplicationCacheHost.cpp	2018-03-02 18:29:42 UTC (rev 229181)
@@ -75,7 +75,7 @@
     ApplicationCacheGroup::selectCache(*m_documentLoader.frame(), manifestURL);
 }
 
-void ApplicationCacheHost::maybeLoadMainResource(ResourceRequest& request, SubstituteData& substituteData)
+void ApplicationCacheHost::maybeLoadMainResource(const ResourceRequest& request, SubstituteData& substituteData)
 {
     // Check if this request should be loaded from the application cache
     if (!substituteData.isValid() && isApplicationCacheEnabled() && !isApplicationCacheBlockedForRequest(request)) {
@@ -104,7 +104,7 @@
     }
 }
 
-void ApplicationCacheHost::maybeLoadMainResourceForRedirect(ResourceRequest& request, SubstituteData& substituteData)
+void ApplicationCacheHost::maybeLoadMainResourceForRedirect(const ResourceRequest& request, SubstituteData& substituteData)
 {
     ASSERT(status() == UNCACHED);
     maybeLoadMainResource(request, substituteData);
@@ -178,6 +178,11 @@
     if (request.url() != originalURL)
         return false;
 
+#if ENABLE(SERVICE_WORKER)
+    if (loader.options().serviceWorkerRegistrationIdentifier)
+        return false;
+#endif
+
     ApplicationCacheResource* resource;
     if (!shouldLoadResourceFromApplicationCache(request, resource))
         return false;
@@ -453,9 +458,17 @@
 
 bool ApplicationCacheHost::scheduleLoadFallbackResourceFromApplicationCache(ResourceLoader* loader, ApplicationCache* cache)
 {
+    if (!loader)
+        return false;
+
     if (!isApplicationCacheEnabled() && !isApplicationCacheBlockedForRequest(loader->request()))
         return false;
 
+#if ENABLE(SERVICE_WORKER)
+    if (loader->options().serviceWorkerRegistrationIdentifier)
+        return false;
+#endif
+
     ApplicationCacheResource* resource;
     if (!getApplicationCacheFallbackResource(loader->request(), resource, cache))
         return false;

Modified: trunk/Source/WebCore/loader/appcache/ApplicationCacheHost.h (229180 => 229181)


--- trunk/Source/WebCore/loader/appcache/ApplicationCacheHost.h	2018-03-02 17:55:22 UTC (rev 229180)
+++ trunk/Source/WebCore/loader/appcache/ApplicationCacheHost.h	2018-03-02 18:29:42 UTC (rev 229181)
@@ -89,8 +89,8 @@
     void selectCacheWithoutManifest();
     void selectCacheWithManifest(const URL& manifestURL);
 
-    void maybeLoadMainResource(ResourceRequest&, SubstituteData&);
-    void maybeLoadMainResourceForRedirect(ResourceRequest&, SubstituteData&);
+    void maybeLoadMainResource(const ResourceRequest&, SubstituteData&);
+    void maybeLoadMainResourceForRedirect(const ResourceRequest&, SubstituteData&);
     bool maybeLoadFallbackForMainResponse(const ResourceRequest&, const ResourceResponse&);
     void mainResourceDataReceived(const char* data, int length, long long encodedDataLength, bool allAtOnce);
     void finishedLoadingMainResource();
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to