Bing video search result pages are not PageCacheable
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 Apr 2015 06:22:59 +0000 (06:22 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 Apr 2015 06:22:59 +0000 (06:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143463
<rdar://problem/20440916>

Reviewed by Alexey Proskuryakov.

Source/WebCore:

Bing video search result pages are not PageCacheable (tested on iOS).
It both:
- is bad for power usage as it causes a reload when clicking one of the
  results then navigating back in history.
- degrades user experience because the results page uses infinite
  scrolling and the scroll position is not properly restored when
  navigating back, not to mention the user has to wait for the reload
  to complete.

The issue was that the bing search page was doing a ping load when
clicking on one of the search results. The ping load was done by
create an image and its 'src' attribute to the ping URL. This load
usually did not have time to complete when navigating away so we would
cancel it and the main document would end up with an error that would
prevent the page from entering the page cache. We already have code
making sure load cancellations do not prevent page caching as long as
the loads are for XHR or images. However, the latter check was broken
in the case where the ResourceRequest's cachePartition was non-empty.
This is because the check was using the MemoryCache::ResourceForUrl()
API which rarely does what we want because it will request a dummy
ResourceRequest (without cachePartition) and then call
MemoryCache::resourceForRequest(). This patch updates the check
to use resourceForRequest() directly as we have the ResourceRequest
at this point.

This patch also gets rid of the  MemoryCache::ResourceForUrl() API as
it rarely does what we want and it is bug prone. It was only used in
2 places, one of them causing this bug and the other in Internals.

Tests: http/tests/navigation/page-cache-pending-image-load-cache-partition.html
       http/tests/navigation/page-cache-pending-image-load.html

* loader/DocumentLoader.cpp:
(WebCore::areAllLoadersPageCacheAcceptable):
Use MemoryCache::resourceForRequest() instead of resourceForUrl() as
we have the ResourceRequest and calling resourceForUrl() would loose
the cachePartition.

* loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::resourceForURL): Deleted.
Drop this API as it is bug prone and rarely does what we want.

* testing/Internals.cpp:
(WebCore::Internals::isLoadingFromMemoryCache):
Update call to create a dummy ResourceRequest and call
resourceForRequest() instead of resourceForUrl(), as this API no
longer exists. The new code also set the cachePartition on the
dummy request so that this function actually behaves as expected
if the cachePartition in the memory cache is non-empty.

Source/WebKit/mac:

Fix the iOS / WK1 build by using MemoryCache::resourceForRequest()
instead of resourceForUrl().

* Misc/WebCache.mm:
(+[WebCache imageForURL:]):

LayoutTests:

Add layout tests to confirm that a pending image load does not prevent
a page from entering the page cache. There are 2 tests, once that cover
the case where the request's cachePartion is empty (passing without the
fix), and another where the request's cachePartition is non-empty
(which only passes with the fix).

* http/tests/navigation/page-cache-pending-image-load-cache-partition-expected.txt: Added.
* http/tests/navigation/page-cache-pending-image-load-cache-partition.html: Added.
* http/tests/navigation/page-cache-pending-image-load-expected.txt: Added.
* http/tests/navigation/page-cache-pending-image-load.html: Added.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition.html [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-image-load-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-image-load.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/cache/MemoryCache.cpp
Source/WebCore/loader/cache/MemoryCache.h
Source/WebCore/testing/Internals.cpp
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/Misc/WebCache.mm

index befb5b3..ffca1ff 100644 (file)
@@ -1,3 +1,22 @@
+2015-04-06  Chris Dumez  <cdumez@apple.com>
+
+        Bing video search result pages are not PageCacheable
+        https://bugs.webkit.org/show_bug.cgi?id=143463
+        <rdar://problem/20440916>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Add layout tests to confirm that a pending image load does not prevent
+        a page from entering the page cache. There are 2 tests, once that cover
+        the case where the request's cachePartion is empty (passing without the
+        fix), and another where the request's cachePartition is non-empty
+        (which only passes with the fix).
+
+        * http/tests/navigation/page-cache-pending-image-load-cache-partition-expected.txt: Added.
+        * http/tests/navigation/page-cache-pending-image-load-cache-partition.html: Added.
+        * http/tests/navigation/page-cache-pending-image-load-expected.txt: Added.
+        * http/tests/navigation/page-cache-pending-image-load.html: Added.
+
 2015-04-06  Andy Estes  <aestes@apple.com>
 
         http/tests/contentfiltering/block-after-redirect.html fails on Windows
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition-expected.txt b/LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition-expected.txt
new file mode 100644 (file)
index 0000000..0739d75
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that a page with a loading image goes into the page cache (with non-empty cache partition).
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+pageshow - not from cache
+pagehide - entering cache
+pageshow - from cache
+PASS Page did enter and was restored from the page cache
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition.html b/LayoutTests/http/tests/navigation/page-cache-pending-image-load-cache-partition.html
new file mode 100644 (file)
index 0000000..b183a1c
--- /dev/null
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/resources/js-test-pre.js"></script>
+<script>
+description('Tests that a page with a loading image goes into the page cache (with non-empty cache partition).');
+window.jsTestIsAsync = true;
+internals.settings.setStorageBlockingPolicy('BlockThirdParty');
+
+if (window.testRunner)
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+
+window.addEventListener("pageshow", function(event) {
+    debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
+
+    if (event.persisted) {
+        testPassed("Page did enter and was restored from the page cache");
+        setTimeout(function() {
+            finishJSTest();
+        }, 0);
+    }
+}, false);
+
+window.addEventListener("pagehide", function(event) {
+    debug("pagehide - " + (event.persisted ? "" : "not ") + "entering cache");
+    if (!event.persisted) {
+        testFailed("Page did not enter the page cache.");
+        finishJSTest();
+    }
+}, false);
+
+window.addEventListener('load', function() {
+    setTimeout(function() {
+        // Slow loading image.
+        var slowImage = new Image();
+        slowImage.src = "/history/resources/slow-image.php";
+        document.body.appendChild(slowImage);
+    }, 0);
+
+    // This needs to happen in a setTimeout because a navigation inside the onload handler would
+    // not create a history entry.
+    setTimeout(function() {
+      // Force a back navigation back to this page.
+      window.location.href = "resources/page-cache-helper.html";
+    }, 0);
+}, false);
+
+</script>
+<script src="/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-image-load-expected.txt b/LayoutTests/http/tests/navigation/page-cache-pending-image-load-expected.txt
new file mode 100644 (file)
index 0000000..53cb0c7
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that a page with a loading image goes into the page cache.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+pageshow - not from cache
+pagehide - entering cache
+pageshow - from cache
+PASS Page did enter and was restored from the page cache
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-image-load.html b/LayoutTests/http/tests/navigation/page-cache-pending-image-load.html
new file mode 100644 (file)
index 0000000..520b526
--- /dev/null
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/resources/js-test-pre.js"></script>
+<script>
+description('Tests that a page with a loading image goes into the page cache.');
+window.jsTestIsAsync = true;
+
+if (window.testRunner)
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+
+window.addEventListener("pageshow", function(event) {
+    debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
+
+    if (event.persisted) {
+        testPassed("Page did enter and was restored from the page cache");
+        setTimeout(function() {
+            finishJSTest();
+        }, 0);
+    }
+}, false);
+
+window.addEventListener("pagehide", function(event) {
+    debug("pagehide - " + (event.persisted ? "" : "not ") + "entering cache");
+    if (!event.persisted) {
+        testFailed("Page did not enter the page cache.");
+        finishJSTest();
+    }
+}, false);
+
+window.addEventListener('load', function() {
+    setTimeout(function() {
+        // Slow loading image.
+        var slowImage = new Image();
+        slowImage.src = "/history/resources/slow-image.php";
+        document.body.appendChild(slowImage);
+    }, 0);
+
+    // This needs to happen in a setTimeout because a navigation inside the onload handler would
+    // not create a history entry.
+    setTimeout(function() {
+      // Force a back navigation back to this page.
+      window.location.href = "resources/page-cache-helper.html";
+    }, 0);
+}, false);
+
+</script>
+<script src="/resources/js-test-post.js"></script>
+</body>
+</html>
index f8680ac..c324574 100644 (file)
@@ -1,3 +1,62 @@
+2015-04-06  Chris Dumez  <cdumez@apple.com>
+
+        Bing video search result pages are not PageCacheable
+        https://bugs.webkit.org/show_bug.cgi?id=143463
+        <rdar://problem/20440916>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Bing video search result pages are not PageCacheable (tested on iOS).
+        It both:
+        - is bad for power usage as it causes a reload when clicking one of the
+          results then navigating back in history.
+        - degrades user experience because the results page uses infinite
+          scrolling and the scroll position is not properly restored when
+          navigating back, not to mention the user has to wait for the reload
+          to complete.
+
+        The issue was that the bing search page was doing a ping load when
+        clicking on one of the search results. The ping load was done by
+        create an image and its 'src' attribute to the ping URL. This load
+        usually did not have time to complete when navigating away so we would
+        cancel it and the main document would end up with an error that would
+        prevent the page from entering the page cache. We already have code
+        making sure load cancellations do not prevent page caching as long as
+        the loads are for XHR or images. However, the latter check was broken
+        in the case where the ResourceRequest's cachePartition was non-empty.
+        This is because the check was using the MemoryCache::ResourceForUrl()
+        API which rarely does what we want because it will request a dummy
+        ResourceRequest (without cachePartition) and then call
+        MemoryCache::resourceForRequest(). This patch updates the check
+        to use resourceForRequest() directly as we have the ResourceRequest
+        at this point.
+
+        This patch also gets rid of the  MemoryCache::ResourceForUrl() API as
+        it rarely does what we want and it is bug prone. It was only used in
+        2 places, one of them causing this bug and the other in Internals.
+
+        Tests: http/tests/navigation/page-cache-pending-image-load-cache-partition.html
+               http/tests/navigation/page-cache-pending-image-load.html
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::areAllLoadersPageCacheAcceptable):
+        Use MemoryCache::resourceForRequest() instead of resourceForUrl() as
+        we have the ResourceRequest and calling resourceForUrl() would loose
+        the cachePartition.
+
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::resourceForURL): Deleted.
+        Drop this API as it is bug prone and rarely does what we want.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::isLoadingFromMemoryCache):
+        Update call to create a dummy ResourceRequest and call
+        resourceForRequest() instead of resourceForUrl(), as this API no
+        longer exists. The new code also set the cachePartition on the
+        dummy request so that this function actually behaves as expected
+        if the cachePartition in the memory cache is non-empty.
+
+
 2015-04-06  Jer Noble  <jer.noble@apple.com>
 
         Synchronize fullscreen animation between processes.
index 2a8f2f0..fb8be6a 100644 (file)
@@ -103,7 +103,7 @@ static bool areAllLoadersPageCacheAcceptable(const ResourceLoaderMap& loaders)
         if (!loader->frameLoader())
             return false;
 
-        CachedResource* cachedResource = MemoryCache::singleton().resourceForURL(loader->request().url(), loader->frameLoader()->frame().page()->sessionID());
+        CachedResource* cachedResource = MemoryCache::singleton().resourceForRequest(loader->request(), loader->frameLoader()->frame().page()->sessionID());
         if (!cachedResource)
             return false;
 
index 80486d4..fab1c0e 100644 (file)
@@ -168,11 +168,6 @@ void MemoryCache::revalidationFailed(CachedResource& revalidatingResource)
     revalidatingResource.clearResourceToRevalidate();
 }
 
-CachedResource* MemoryCache::resourceForURL(const URL& resourceURL, SessionID sessionID)
-{
-    return resourceForRequest(ResourceRequest(resourceURL), sessionID);
-}
-
 CachedResource* MemoryCache::resourceForRequest(const ResourceRequest& request, SessionID sessionID)
 {
     auto* resources = sessionResourceMap(sessionID);
index 7674242..5a3d37e 100644 (file)
@@ -91,7 +91,6 @@ public:
 
     WEBCORE_EXPORT static MemoryCache& singleton();
 
-    WEBCORE_EXPORT CachedResource* resourceForURL(const URL&, SessionID = SessionID::defaultSessionID());
     WEBCORE_EXPORT CachedResource* resourceForRequest(const ResourceRequest&, SessionID);
 
     bool add(CachedResource&);
index b78486d..7bff9ea 100644 (file)
@@ -405,7 +405,12 @@ bool Internals::isLoadingFromMemoryCache(const String& url)
 {
     if (!contextDocument() || !contextDocument()->page())
         return false;
-    CachedResource* resource = MemoryCache::singleton().resourceForURL(contextDocument()->completeURL(url), contextDocument()->page()->sessionID());
+
+    ResourceRequest request(contextDocument()->completeURL(url));
+#if ENABLE(CACHE_PARTITIONING)
+    request.setDomainForCachePartition(contextDocument()->topOrigin()->domainForCachePartition());
+#endif
+    CachedResource* resource = MemoryCache::singleton().resourceForRequest(request, contextDocument()->page()->sessionID());
     return resource && resource->status() == CachedResource::Cached;
 }
 
index cf33b5a..f738716 100644 (file)
@@ -1,3 +1,17 @@
+2015-04-06  Chris Dumez  <cdumez@apple.com>
+
+        Bing video search result pages are not PageCacheable
+        https://bugs.webkit.org/show_bug.cgi?id=143463
+        <rdar://problem/20440916>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Fix the iOS / WK1 build by using MemoryCache::resourceForRequest()
+        instead of resourceForUrl().
+
+        * Misc/WebCache.mm:
+        (+[WebCache imageForURL:]):
+
 2015-04-05  Simon Fraser  <simon.fraser@apple.com>
 
         Remove "go ahead and" from comments
index 18d3142..6fb5d85 100644 (file)
     if (!url)
         return nullptr;
     
-    WebCore::CachedResource* cachedResource = WebCore::MemoryCache::singleton().resourceForURL(url);
+    WebCore::ResourceRequest request(url);
+    WebCore::CachedResource* cachedResource = WebCore::MemoryCache::singleton().resourceForRequest(request, WebCore::SessionID::defaultSessionID());
     if (!is<WebCore::CachedImage>(cachedResource))
         return nullptr;
     WebCore::CachedImage& cachedImage = downcast<WebCore::CachedImage>(*cachedResource);