[Fetch API] Memory cache should not bypass redirect mode
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Oct 2016 13:09:56 +0000 (13:09 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Oct 2016 13:09:56 +0000 (13:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162959

Patch by Youenn Fablet <youenn@apple.com> on 2016-10-10
Reviewed by Darin Adler.

Source/WebCore:

Test: http/tests/fetch/redirectmode-and-preload.html

Ensure reloading of resources if the redirect modes are different between request and cached resource, and
cached resource has redirections.

As a temporary workaround, we activate resource update for raw resources in
shouldUpdateCachedResourceWithCurrentRequest but disable it in canUpdateFromResource.
This allows handling reloading of resources with different redirection mode in canUpdateFromResource.

A future patch should allow loading cached raw resources from other cached raw resources.

* loader/cache/CachedResource.h:
(WebCore::CachedResource::hasRedirections):
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::shouldUpdateCachedResourceWithCurrentRequest):
(WebCore::canUpdateFromResource):
(WebCore::CachedResourceLoader::updateCachedResourceWithCurrentRequest):

LayoutTests:

* http/tests/fetch/redirectmode-and-preload-expected.txt: Added.
* http/tests/fetch/redirectmode-and-preload.html: Added.
* http/tests/fetch/resources/redirect-with-cache.php:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/fetch/redirectmode-and-preload-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/fetch/redirectmode-and-preload.html [new file with mode: 0644]
LayoutTests/http/tests/fetch/resources/redirect-with-cache.php
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp

index 097d7a0..cc4df84 100644 (file)
@@ -1,3 +1,14 @@
+2016-10-10  Youenn Fablet  <youenn@apple.com>
+
+        [Fetch API] Memory cache should not bypass redirect mode
+        https://bugs.webkit.org/show_bug.cgi?id=162959
+
+        Reviewed by Darin Adler.
+
+        * http/tests/fetch/redirectmode-and-preload-expected.txt: Added.
+        * http/tests/fetch/redirectmode-and-preload.html: Added.
+        * http/tests/fetch/resources/redirect-with-cache.php:
+
 2016-10-09  Antti Koivisto  <antti@apple.com>
 
         Enable optimized stylesheet updates in shadow trees
diff --git a/LayoutTests/http/tests/fetch/redirectmode-and-preload-expected.txt b/LayoutTests/http/tests/fetch/redirectmode-and-preload-expected.txt
new file mode 100644 (file)
index 0000000..48976a0
--- /dev/null
@@ -0,0 +1,6 @@
+
+PASS Fetch should check for redirections even if resource is preloaded (same fetch options except for redirect mode) 
+PASS Fetch should check for redirections even if resource is preloaded (different fetch mode, different redirect mode) 
+PASS Fetch should check for redirections even if resource is preloaded (same fetch options except for redirect mode) 
+PASS Fetch should check for redirections even if resource is preloaded (different fetch mode, different redirect mode) 
+
diff --git a/LayoutTests/http/tests/fetch/redirectmode-and-preload.html b/LayoutTests/http/tests/fetch/redirectmode-and-preload.html
new file mode 100644 (file)
index 0000000..b1af93f
--- /dev/null
@@ -0,0 +1,50 @@
+<!doctype html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Fetch: retrieve response's body progressively</title>
+    <meta name="help" href="https://fetch.spec.whatwg.org/#main-fetch">
+    <meta name="help" href="https://fetch.spec.whatwg.org/#http-fetch">
+    <meta name="author" title="Canon Research France" href="https://www.crf.canon.fr">
+    <script src="/js-test-resources/testharness.js"></script>
+    <script src="/js-test-resources/testharnessreport.js"></script>
+    <script>
+if (window.internals)
+    internals.setLinkPreloadSupport(true);
+    </script>
+    <link rel=preload onload="startTests()" href="./resources/redirect-with-cache.php?enableCaching&url=http://localhost:8000/security/resources/allow-if-origin.php?allowCache&origin=http%3A%2F%2F127.0.0.1%3A8000&name=alert-fail.js&contentType=text/ascii">
+  </head>
+  <body>
+    <script>
+function startTests()
+{
+    var preloadUrl = "./resources/redirect-with-cache.php?enableCaching&url=http://localhost:8000/security/resources/allow-if-origin.php?allowCache&origin=http%3A%2F%2F127.0.0.1%3A8000&name=alert-fail.js&contentType=text/ascii";
+    promise_test(function(test) {
+        return fetch(preloadUrl, {redirect: "manual", mode: "no-cors", credentials: "include"}).then((response) => {
+            assert_equals(response.type, "opaqueredirect", "Response's type should be opaqueRedirect");
+            return response.text();
+        }).then((text) => {
+            assert_equals(text, "");
+        });
+    }, "Fetch should check for redirections even if resource is preloaded (same fetch options except for redirect mode)");
+
+    promise_test(function(test) {
+        return fetch(preloadUrl, {redirect: "manual", mode: "cors", credentials: "include"}).then((response) => {
+            assert_equals(response.type, "opaqueredirect", "Response's type should be opaqueRedirect");
+            return response.text();
+        }).then((text) => {
+            assert_equals(text, "");
+        });
+    }, "Fetch should check for redirections even if resource is preloaded (different fetch mode, different redirect mode)");
+
+    promise_test(function(test) {
+        return promise_rejects(test, new TypeError(), fetch(preloadUrl, {redirect: "error", mode: "no-cors", credentials: "include"}));
+    }, "Fetch should check for redirections even if resource is preloaded (same fetch options except for redirect mode)");
+
+    promise_test(function(test) {
+        return promise_rejects(test, new TypeError(), fetch(preloadUrl, {redirect: "error", mode: "cors", credentials: "include"}));
+    }, "Fetch should check for redirections even if resource is preloaded (different fetch mode, different redirect mode)");
+}
+    </script>
+  </body>
+</html>
index 19c81ed..f1e4594 100644 (file)
@@ -1,7 +1,7 @@
 <?php
     $url = $_GET["url"];
 
-    $enableCaching = isset($_SERVER["ENABLECACHING"]) ? true : false;
+    $enableCaching = isset($_SERVER["ENABLECACHING"]) || isset($_GET["enableCaching"]) ? true : false;
     $code = isset($_GET["code"]) ? $_GET["code"] : 302;
 
     header("HTTP/1.1 $code");
index 01a2dfc..ec181ea 100644 (file)
@@ -1,5 +1,30 @@
 2016-10-10  Youenn Fablet  <youenn@apple.com>
 
+        [Fetch API] Memory cache should not bypass redirect mode
+        https://bugs.webkit.org/show_bug.cgi?id=162959
+
+        Reviewed by Darin Adler.
+
+        Test: http/tests/fetch/redirectmode-and-preload.html
+
+        Ensure reloading of resources if the redirect modes are different between request and cached resource, and
+        cached resource has redirections.
+
+        As a temporary workaround, we activate resource update for raw resources in
+        shouldUpdateCachedResourceWithCurrentRequest but disable it in canUpdateFromResource.
+        This allows handling reloading of resources with different redirection mode in canUpdateFromResource.
+
+        A future patch should allow loading cached raw resources from other cached raw resources.
+
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::hasRedirections):
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::shouldUpdateCachedResourceWithCurrentRequest):
+        (WebCore::canUpdateFromResource):
+        (WebCore::CachedResourceLoader::updateCachedResourceWithCurrentRequest):
+
+2016-10-10  Youenn Fablet  <youenn@apple.com>
+
         Attribute getter binding generated code should use more references
         https://bugs.webkit.org/show_bug.cgi?id=163179
 
index bea9e38..44f3f74 100644 (file)
@@ -234,25 +234,26 @@ public:
     virtual void destroyDecodedData() { }
 
     void setOwningCachedResourceLoader(CachedResourceLoader* cachedResourceLoader) { m_owningCachedResourceLoader = cachedResourceLoader; }
-    
+
     bool isPreloaded() const { return m_preloadCount; }
     void increasePreloadCount() { ++m_preloadCount; }
     void decreasePreloadCount() { ASSERT(m_preloadCount); --m_preloadCount; }
-    
+
     void registerHandle(CachedResourceHandleBase* h);
     WEBCORE_EXPORT void unregisterHandle(CachedResourceHandleBase* h);
-    
+
     bool canUseCacheValidator() const;
 
     enum class RevalidationDecision { No, YesDueToCachePolicy, YesDueToNoStore, YesDueToNoCache, YesDueToExpired };
     virtual RevalidationDecision makeRevalidationDecision(CachePolicy) const;
     bool redirectChainAllowsReuse(ReuseExpiredRedirectionOrNot) const;
+    bool hasRedirections() const { return m_redirectChainCacheStatus.status != RedirectChainCacheStatus::Status::NoRedirection;  }
 
     bool varyHeaderValuesMatch(const ResourceRequest&, const CachedResourceLoader&);
 
     bool isCacheValidator() const { return m_resourceToRevalidate; }
     CachedResource* resourceToRevalidate() const { return m_resourceToRevalidate; }
-    
+
     // HTTP revalidation support methods for CachedResourceLoader.
     void setResourceToRevalidate(CachedResource*);
     virtual void switchClientsToRevalidatedResource();
index 11c6476..210910d 100644 (file)
@@ -561,8 +561,6 @@ bool CachedResourceLoader::shouldUpdateCachedResourceWithCurrentRequest(const Ca
         return false;
     case CachedResource::MediaResource:
         return false;
-    case CachedResource::RawResource:
-        return false;
     case CachedResource::MainResource:
         return false;
 #if ENABLE(LINK_PREFETCH)
@@ -574,14 +572,43 @@ bool CachedResourceLoader::shouldUpdateCachedResourceWithCurrentRequest(const Ca
     default:
         break;
     }
-    return resource.options().mode != request.options().mode || request.resourceRequest().httpOrigin() != resource.resourceRequest().httpOrigin();
+
+    if (resource.options().mode != request.options().mode || request.resourceRequest().httpOrigin() != resource.resourceRequest().httpOrigin())
+        return true;
+
+    if (resource.options().redirect != request.options().redirect && resource.hasRedirections())
+        return true;
+
+    return false;
 }
 
-CachedResourceHandle<CachedResource> CachedResourceLoader::updateCachedResourceWithCurrentRequest(const CachedResource& resource, CachedResourceRequest&& request)
+static inline bool isResourceSuitableForDirectReuse(const CachedResource& resource, const CachedResourceRequest& request)
 {
-    // FIXME: For being loaded requests, we currently do not use the same resource, as this may induce errors in the resource response tainting.
+    // FIXME: For being loaded requests, the response tainting may not be correctly computed if the fetch mode is not the same.
+    // Even if the fetch mode is the same, we are not sure that the resource can be reused (Vary: Origin header for instance).
     // We should find a way to improve this.
-    if (resource.status() != CachedResource::Cached) {
+    if (resource.status() != CachedResource::Cached)
+        return false;
+
+    // If the cached resource has not followed redirections, it is incomplete and we should not use it.
+    // Let's make sure the memory cache has no such resource.
+    ASSERT(resource.response().type() != ResourceResponse::Type::Opaqueredirect);
+
+    // We could support redirect modes other than Follow in case of a redirected resource.
+    // This case is rare and is not worth optimizing currently.
+    if (request.options().redirect != FetchOptions::Redirect::Follow && resource.hasRedirections())
+        return false;
+
+    // FIXME: Implement reuse of cached raw resources.
+    if (resource.type() == CachedResource::Type::RawResource)
+        return false;
+
+    return true;
+}
+
+CachedResourceHandle<CachedResource> CachedResourceLoader::updateCachedResourceWithCurrentRequest(const CachedResource& resource, CachedResourceRequest&& request)
+{
+    if (!isResourceSuitableForDirectReuse(resource, request)) {
         request.setCachingPolicy(CachingPolicy::DisallowCaching);
         return loadResource(resource.type(), WTFMove(request));
     }