Ping loads should not prevent page caching
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 4 Aug 2019 18:32:44 +0000 (18:32 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 4 Aug 2019 18:32:44 +0000 (18:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200418
<rdar://problem/53901632>

Reviewed by Darin Adler.

Source/WebCore:

We normally prevent page caching if there were any pending subresource loads when navigating,
to avoid caching partial / broken content. However, this should not apply to Ping / Beacon
loads since those do not impact page rendering and can outlive the page.

Tests: http/tests/navigation/page-cache-pending-ping-load-cross-origin.html
       http/tests/navigation/page-cache-pending-ping-load-same-origin.html

* history/PageCache.cpp:
(WebCore::PageCache::addIfCacheable):
After we've fired the 'pagehide' event in each frame, stop all the loads again. This is needed
since pages are allowed to start ping / beacon loads in their 'pagehide' handlers. If we do not
stop those loads, then the next call to canCachePage() would fail because the DocumentLoader is
still loading. Note that we're not actually preventing these ping loads from hitting the server
since we never cancel page loads and those can outlive their page.

* loader/DocumentLoader.cpp:
(WebCore::shouldPendingCachedResourceLoadPreventPageCache):
(WebCore::areAllLoadersPageCacheAcceptable):
Make sure that Ping / Beacon / Prefetches / Icon loads do not prevent page caching.

(WebCore::DocumentLoader::addSubresourceLoader):
Tweak assertion that was incorrect since we actually allow ping / beacon loads when the
document is about to enter PageCache (while firing pagehide event).

Tools:

Add TestOption to enable PageCache at UIProcess-level so that we can test
page caching when navigating cross-origin with PSON enabled.

* WebKitTestRunner/TestController.cpp:
(WTR::TestController::resetPreferencesToConsistentValues):
(WTR::updateTestOptionsFromTestHeader):
* WebKitTestRunner/TestOptions.h:
(WTR::TestOptions::hasSameInitializationOptions const):

LayoutTests:

Add layout test coverage.

* http/tests/navigation/page-cache-pending-ping-load-cross-origin-expected.txt: Added.
* http/tests/navigation/page-cache-pending-ping-load-cross-origin.html: Added.
* http/tests/navigation/page-cache-pending-ping-load-same-origin-expected.txt: Added.
* http/tests/navigation/page-cache-pending-ping-load-same-origin.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/navigation/page-cache-pending-ping-load-cross-origin-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-ping-load-cross-origin.html [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-ping-load-same-origin-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-ping-load-same-origin.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/history/PageCache.cpp
Source/WebCore/loader/DocumentLoader.cpp
Tools/ChangeLog
Tools/WebKitTestRunner/TestController.cpp
Tools/WebKitTestRunner/TestOptions.h

index 0ae6ee4..e772834 100644 (file)
@@ -1,3 +1,18 @@
+2019-08-04  Chris Dumez  <cdumez@apple.com>
+
+        Ping loads should not prevent page caching
+        https://bugs.webkit.org/show_bug.cgi?id=200418
+        <rdar://problem/53901632>
+
+        Reviewed by Darin Adler.
+
+        Add layout test coverage.
+
+        * http/tests/navigation/page-cache-pending-ping-load-cross-origin-expected.txt: Added.
+        * http/tests/navigation/page-cache-pending-ping-load-cross-origin.html: Added.
+        * http/tests/navigation/page-cache-pending-ping-load-same-origin-expected.txt: Added.
+        * http/tests/navigation/page-cache-pending-ping-load-same-origin.html: Added.
+
 2019-08-03  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Elements: Styles: add icons for various CSS rule types
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-cross-origin-expected.txt b/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-cross-origin-expected.txt
new file mode 100644 (file)
index 0000000..b4bb6c7
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that a page with pending ping loads can enter PageCache.
+
+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
+Click me
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-cross-origin.html b/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-cross-origin.html
new file mode 100644 (file)
index 0000000..b738538
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enablePageCache=true ] -->
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description('Tests that a page with pending ping loads can enter PageCache.');
+window.jsTestIsAsync = true;
+
+if (window.testRunner)
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+
+let afterPageCacheRestore = false;
+
+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");
+        finishJSTest();
+    }
+}, 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();
+    }
+
+    testPing2.click();
+}, false);
+
+window.addEventListener('load', function() {
+    testPing1.click();
+    setTimeout(function() {
+        testLink.click();
+    }, 0);
+}, false);
+
+</script>
+<a id="testPing1" href="javascript:void(0);" ping="/resources/load-and-stall.cgi?name=load-and-stall.cgi&stallFor=3&stallAt=0&mimeType=text/plain&bar" style="display: none;"></a>
+<a id="testPing2" href="javascript:void(0);" ping="/resources/load-and-stall.cgi?name=load-and-stall.cgi&stallFor=3&stallAt=0&mimeType=text/plain&baz" style="display: none;"></a>
+<a id="testLink" href="http://localhost:8000/navigation/resources/page-cache-helper.html" ping="/resources/load-and-stall.cgi?name=load-and-stall.cgi&stallFor=3&stallAt=0&mimeType=text/plain&foo">Click me</a>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-same-origin-expected.txt b/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-same-origin-expected.txt
new file mode 100644 (file)
index 0000000..b4bb6c7
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that a page with pending ping loads can enter PageCache.
+
+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
+Click me
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-same-origin.html b/LayoutTests/http/tests/navigation/page-cache-pending-ping-load-same-origin.html
new file mode 100644 (file)
index 0000000..6832b1e
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description('Tests that a page with pending ping loads can enter PageCache.');
+window.jsTestIsAsync = true;
+
+if (window.testRunner)
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+
+let afterPageCacheRestore = false;
+
+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");
+        finishJSTest();
+    }
+}, 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();
+    }
+
+    testPing2.click();
+}, false);
+
+window.addEventListener('load', function() {
+    testPing1.click();
+    setTimeout(function() {
+        testLink.click();
+    }, 0);
+}, false);
+
+</script>
+<a id="testPing1" href="javascript:void(0);" ping="/resources/load-and-stall.cgi?name=load-and-stall.cgi&stallFor=3&stallAt=0&mimeType=text/plain&bar" style="display: none;"></a>
+<a id="testPing2" href="javascript:void(0);" ping="/resources/load-and-stall.cgi?name=load-and-stall.cgi&stallFor=3&stallAt=0&mimeType=text/plain&baz" style="display: none;"></a>
+<a id="testLink" href="resources/page-cache-helper.html" ping="/resources/load-and-stall.cgi?name=load-and-stall.cgi&stallFor=3&stallAt=0&mimeType=text/plain&foo">Click me</a>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index c41e8b8..5f96e12 100644 (file)
@@ -1,3 +1,35 @@
+2019-08-04  Chris Dumez  <cdumez@apple.com>
+
+        Ping loads should not prevent page caching
+        https://bugs.webkit.org/show_bug.cgi?id=200418
+        <rdar://problem/53901632>
+
+        Reviewed by Darin Adler.
+
+        We normally prevent page caching if there were any pending subresource loads when navigating,
+        to avoid caching partial / broken content. However, this should not apply to Ping / Beacon
+        loads since those do not impact page rendering and can outlive the page.
+
+        Tests: http/tests/navigation/page-cache-pending-ping-load-cross-origin.html
+               http/tests/navigation/page-cache-pending-ping-load-same-origin.html
+
+        * history/PageCache.cpp:
+        (WebCore::PageCache::addIfCacheable):
+        After we've fired the 'pagehide' event in each frame, stop all the loads again. This is needed
+        since pages are allowed to start ping / beacon loads in their 'pagehide' handlers. If we do not
+        stop those loads, then the next call to canCachePage() would fail because the DocumentLoader is
+        still loading. Note that we're not actually preventing these ping loads from hitting the server
+        since we never cancel page loads and those can outlive their page.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::shouldPendingCachedResourceLoadPreventPageCache):
+        (WebCore::areAllLoadersPageCacheAcceptable):
+        Make sure that Ping / Beacon / Prefetches / Icon loads do not prevent page caching.
+
+        (WebCore::DocumentLoader::addSubresourceLoader):
+        Tweak assertion that was incorrect since we actually allow ping / beacon loads when the
+        document is about to enter PageCache (while firing pagehide event).
+
 2019-08-04  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][TFC] Create formatting context/state.
index 2379d9d..0029655 100644 (file)
@@ -458,6 +458,13 @@ bool PageCache::addIfCacheable(HistoryItem& item, Page* page)
 
     destroyRenderTree(page->mainFrame());
 
+    // Stop all loads again before checking if we can still cache the page after firing the pagehide
+    // event, since the page may have started ping loads in its pagehide event handler.
+    for (Frame* frame = &page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
+        if (auto* documentLoader = frame->loader().documentLoader())
+            documentLoader->stopLoading();
+    }
+
     // Check that the page is still page-cacheable after firing the pagehide event. The JS event handlers
     // could have altered the page in a way that could prevent caching.
     if (!canCache(*page)) {
index de9f47d..de09e70 100644 (file)
@@ -125,6 +125,42 @@ static void setAllDefersLoading(const ResourceLoaderMap& loaders, bool defers)
         loader->setDefersLoading(defers);
 }
 
+static bool shouldPendingCachedResourceLoadPreventPageCache(CachedResource& cachedResource)
+{
+    if (!cachedResource.isLoading())
+        return false;
+
+    switch (cachedResource.type()) {
+    case CachedResource::Type::ImageResource:
+    case CachedResource::Type::Icon:
+    case CachedResource::Type::Beacon:
+    case CachedResource::Type::Ping:
+    case CachedResource::Type::LinkPrefetch:
+        return false;
+    case CachedResource::Type::MainResource:
+    case CachedResource::Type::CSSStyleSheet:
+    case CachedResource::Type::Script:
+    case CachedResource::Type::FontResource:
+#if ENABLE(SVG_FONTS)
+    case CachedResource::Type::SVGFontResource:
+#endif
+    case CachedResource::Type::MediaResource:
+    case CachedResource::Type::RawResource:
+    case CachedResource::Type::SVGDocumentResource:
+#if ENABLE(XSLT)
+    case CachedResource::Type::XSLStyleSheet:
+#endif
+#if ENABLE(VIDEO_TRACK)
+    case CachedResource::Type::TextTrackResource:
+#endif
+#if ENABLE(APPLICATION_MANIFEST)
+    case CachedResource::Type::ApplicationManifest:
+#endif
+        break;
+    };
+    return !cachedResource.areAllClientsXMLHttpRequests();
+}
+
 static bool areAllLoadersPageCacheAcceptable(const ResourceLoaderMap& loaders)
 {
     for (auto& loader : copyToVector(loaders.values())) {
@@ -137,7 +173,7 @@ static bool areAllLoadersPageCacheAcceptable(const ResourceLoaderMap& loaders)
 
         // Only image and XHR loads do not prevent the page from entering the PageCache.
         // All non-image loads will prevent the page from entering the PageCache.
-        if (cachedResource->isLoading() && !cachedResource->isImage() && !cachedResource->areAllClientsXMLHttpRequests())
+        if (shouldPendingCachedResourceLoadPreventPageCache(*cachedResource))
             return false;
     }
     return true;
@@ -1682,8 +1718,24 @@ void DocumentLoader::addSubresourceLoader(ResourceLoader* loader)
     if (loader->options().applicationCacheMode == ApplicationCacheMode::Bypass)
         return;
 
-    // A page in the PageCache or about to enter PageCache should not be able to start loads.
-    ASSERT_WITH_SECURITY_IMPLICATION(!document() || document()->pageCacheState() == Document::NotInPageCache);
+#if !ASSERT_DISABLED
+    if (document()) {
+        switch (document()->pageCacheState()) {
+        case Document::NotInPageCache:
+            break;
+        case Document::AboutToEnterPageCache: {
+            // A page about to enter PageCache should only be able to start ping loads.
+            auto* cachedResource = MemoryCache::singleton().resourceForRequest(loader->request(), loader->frameLoader()->frame().page()->sessionID());
+            ASSERT(cachedResource && CachedResource::shouldUsePingLoad(cachedResource->type()));
+            break;
+        }
+        case Document::InPageCache:
+            // A page in the PageCache should not be able to start loads.
+            ASSERT_NOT_REACHED();
+            break;
+        }
+    }
+#endif
 
     m_subresourceLoaders.add(loader->identifier(), loader);
 }
index 46f6dd6..82ecb29 100644 (file)
@@ -1,3 +1,20 @@
+2019-08-04  Chris Dumez  <cdumez@apple.com>
+
+        Ping loads should not prevent page caching
+        https://bugs.webkit.org/show_bug.cgi?id=200418
+        <rdar://problem/53901632>
+
+        Reviewed by Darin Adler.
+
+        Add TestOption to enable PageCache at UIProcess-level so that we can test
+        page caching when navigating cross-origin with PSON enabled.
+
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::resetPreferencesToConsistentValues):
+        (WTR::updateTestOptionsFromTestHeader):
+        * WebKitTestRunner/TestOptions.h:
+        (WTR::TestOptions::hasSameInitializationOptions const):
+
 2019-08-02  Keith Rollin  <krollin@apple.com>
 
         Consistently use Obj-C boolean literals
index 663fac7..7957dc6 100644 (file)
@@ -800,7 +800,6 @@ void TestController::resetPreferencesToConsistentValues(const TestOptions& optio
 #if ENABLE(FULLSCREEN_API)
     WKPreferencesSetFullScreenEnabled(preferences, true);
 #endif
-    WKPreferencesSetPageCacheEnabled(preferences, false);
     WKPreferencesSetAsynchronousPluginInitializationEnabled(preferences, false);
     WKPreferencesSetAsynchronousPluginInitializationEnabledForAllPlugins(preferences, false);
     WKPreferencesSetArtificialPluginInitializationDelayEnabled(preferences, false);
@@ -821,6 +820,7 @@ void TestController::resetPreferencesToConsistentValues(const TestOptions& optio
     WKPreferencesSetAllowCrossOriginSubresourcesToAskForCredentials(preferences, options.allowCrossOriginSubresourcesToAskForCredentials);
     WKPreferencesSetColorFilterEnabled(preferences, options.enableColorFilter);
     WKPreferencesSetPunchOutWhiteBackgroundsInDarkMode(preferences, options.punchOutWhiteBackgroundsInDarkMode);
+    WKPreferencesSetPageCacheEnabled(preferences, options.enablePageCache);
 
     static WKStringRef defaultTextEncoding = WKStringCreateWithUTF8CString("ISO-8859-1");
     WKPreferencesSetDefaultTextEncodingName(preferences, defaultTextEncoding);
@@ -1399,6 +1399,8 @@ static void updateTestOptionsFromTestHeader(TestOptions& testOptions, const std:
             testOptions.contentMode = { value.c_str() };
         else if (key == "enableAppNap")
             testOptions.enableAppNap = parseBooleanTestHeaderValue(value);
+        else if (key == "enablePageCache")
+            testOptions.enablePageCache = parseBooleanTestHeaderValue(value);
         pairStart = pairEnd + 1;
     }
 }
index 4968f5c..309d40f 100644 (file)
@@ -92,6 +92,7 @@ struct TestOptions {
     bool shouldHandleRunOpenPanel { true };
     bool shouldPresentPopovers { true };
     bool enableAppNap { false };
+    bool enablePageCache { false };
 
     double contentInsetTop { 0 };
 
@@ -146,7 +147,8 @@ struct TestOptions {
             || shouldPresentPopovers != options.shouldPresentPopovers
             || contentInsetTop != options.contentInsetTop
             || contentMode != options.contentMode
-            || enableAppNap != options.enableAppNap)
+            || enableAppNap != options.enableAppNap
+            || enablePageCache != options.enablePageCache)
             return false;
 
         if (!contextOptions.hasSameInitializationOptions(options.contextOptions))