Pages frequently fails to enter the back/forward cache due to pending loads
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Sep 2019 06:17:17 +0000 (06:17 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Sep 2019 06:17:17 +0000 (06:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202289
<rdar://problem/55758506>

Reviewed by Geoffrey Garen.

Source/WebCore:

Allow pages to enter the back / forward cache, even if they have pending loads.
Note that these pending loads get cancelled. Also note that we won't enter page
cache unless the 'load' event has been fired in the main frame, since a
HistoryItem would not get created otherwise. This was causing frequent transient
failures to enter the back / forward cache while browsing (e.g. on weather.com).

Test: http/tests/navigation/page-cache-pending-load.html

* history/PageCache.cpp:
(WebCore::canCacheFrame):
* loader/CrossOriginPreflightChecker.cpp:
* loader/CrossOriginPreflightChecker.h:
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::setMainDocumentError):
(WebCore::DocumentLoader::stopLoading):
* loader/DocumentLoader.h:
* loader/DocumentThreadableLoader.cpp:
* loader/DocumentThreadableLoader.h:
* loader/cache/CachedResource.cpp:
* loader/cache/CachedResource.h:
* loader/cache/CachedResourceClient.h:
(WebCore::CachedResourceClient::deprecatedDidReceiveCachedResource):
* page/DiagnosticLoggingKeys.cpp:
(WebCore::DiagnosticLoggingKeys::mainFrameHasNotFinishedLoadingKey):
* page/DiagnosticLoggingKeys.h:

LayoutTests:

Add layout test coverage.

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

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

16 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/navigation/page-cache-pending-load-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/page-cache-pending-load.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/history/PageCache.cpp
Source/WebCore/loader/CrossOriginPreflightChecker.cpp
Source/WebCore/loader/CrossOriginPreflightChecker.h
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/DocumentLoader.h
Source/WebCore/loader/DocumentThreadableLoader.cpp
Source/WebCore/loader/DocumentThreadableLoader.h
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceClient.h
Source/WebCore/page/DiagnosticLoggingKeys.cpp
Source/WebCore/page/DiagnosticLoggingKeys.h

index 0ee7e0a..72887a0 100644 (file)
@@ -1,3 +1,16 @@
+2019-09-26  Chris Dumez  <cdumez@apple.com>
+
+        Pages frequently fails to enter the back/forward cache due to pending loads
+        https://bugs.webkit.org/show_bug.cgi?id=202289
+        <rdar://problem/55758506>
+
+        Reviewed by Geoffrey Garen.
+
+        Add layout test coverage.
+
+        * http/tests/navigation/page-cache-pending-load-expected.txt: Added.
+        * http/tests/navigation/page-cache-pending-load.html: Added.
+
 2019-09-26  Kate Cheney  <katherine_cheney@apple.com>
 
         Resource Load Statistics: Downgrade all third-party referrer headers
diff --git a/LayoutTests/http/tests/navigation/page-cache-pending-load-expected.txt b/LayoutTests/http/tests/navigation/page-cache-pending-load-expected.txt
new file mode 100644 (file)
index 0000000..4fdd746
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that a page with pending subresource loads 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-load.html b/LayoutTests/http/tests/navigation/page-cache-pending-load.html
new file mode 100644 (file)
index 0000000..4595375
--- /dev/null
@@ -0,0 +1,54 @@
+<!-- webkit-test-runner [ enablePageCache=true ] -->
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+<script src="/resources/js-test-pre.js"></script>
+<script>
+description('Tests that a page with pending subresource loads goes into the page cache.');
+window.jsTestIsAsync = true;
+
+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() {
+    var link = document.createElement("link");
+    link.rel = "stylesheet";
+    link.type = "text/css";
+    link.href = "/incremental/resources/slow-utf8-css.pl";
+    document.head.appendChild(link);
+
+    var script = document.createElement("script");
+    script.async = true;
+    script.src = "/resources/slow-script.pl"
+    document.head.appendChild(script);
+
+    // 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 6496e1c..006fa14 100644 (file)
@@ -1,3 +1,37 @@
+2019-09-26  Chris Dumez  <cdumez@apple.com>
+
+        Pages frequently fails to enter the back/forward cache due to pending loads
+        https://bugs.webkit.org/show_bug.cgi?id=202289
+        <rdar://problem/55758506>
+
+        Reviewed by Geoffrey Garen.
+
+        Allow pages to enter the back / forward cache, even if they have pending loads.
+        Note that these pending loads get cancelled. Also note that we won't enter page
+        cache unless the 'load' event has been fired in the main frame, since a
+        HistoryItem would not get created otherwise. This was causing frequent transient
+        failures to enter the back / forward cache while browsing (e.g. on weather.com).
+
+        Test: http/tests/navigation/page-cache-pending-load.html
+
+        * history/PageCache.cpp:
+        (WebCore::canCacheFrame):
+        * loader/CrossOriginPreflightChecker.cpp:
+        * loader/CrossOriginPreflightChecker.h:
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::setMainDocumentError):
+        (WebCore::DocumentLoader::stopLoading):
+        * loader/DocumentLoader.h:
+        * loader/DocumentThreadableLoader.cpp:
+        * loader/DocumentThreadableLoader.h:
+        * loader/cache/CachedResource.cpp:
+        * loader/cache/CachedResource.h:
+        * loader/cache/CachedResourceClient.h:
+        (WebCore::CachedResourceClient::deprecatedDidReceiveCachedResource):
+        * page/DiagnosticLoggingKeys.cpp:
+        (WebCore::DiagnosticLoggingKeys::mainFrameHasNotFinishedLoadingKey):
+        * page/DiagnosticLoggingKeys.h:
+
 2019-09-26  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][IFC] Line::Run should have a reference to the associated InlineItem
index af6dc47..543839d 100644 (file)
@@ -116,15 +116,6 @@ static bool canCacheFrame(Frame& frame, DiagnosticLoggingClient& diagnosticLoggi
         PCLOG(" Determining if subframe with URL (", currentURL.string(), ") can be cached:");
      
     bool isCacheable = true;
-    if (!documentLoader->mainDocumentError().isNull()) {
-        PCLOG("   -Main document has an error");
-        logPageCacheFailureDiagnosticMessage(diagnosticLoggingClient, DiagnosticLoggingKeys::mainDocumentErrorKey());
-
-        if (documentLoader->mainDocumentError().isCancellation() && documentLoader->subresourceLoadersArePageCacheAcceptable())
-            PCLOG("    -But, it was a cancellation and all loaders during the cancelation were loading images or XHR.");
-        else
-            isCacheable = false;
-    }
     // Do not cache error pages (these can be recognized as pages with substitute data or unreachable URLs).
     if (documentLoader->substituteData().isValid() && !documentLoader->substituteData().failingURL().isEmpty()) {
         PCLOG("   -Frame is an error page");
index d213f1a..b3df411 100644 (file)
@@ -169,9 +169,4 @@ void CrossOriginPreflightChecker::setDefersLoading(bool value)
         m_resource->setDefersLoading(value);
 }
 
-bool CrossOriginPreflightChecker::isXMLHttpRequest() const
-{
-    return m_loader.isXMLHttpRequest();
-}
-
 } // namespace WebCore
index 9a40026..1a7f4e2 100644 (file)
@@ -59,8 +59,6 @@ private:
     static void handleLoadingFailure(DocumentThreadableLoader&, unsigned long, const ResourceError&);
     static void validatePreflightResponse(DocumentThreadableLoader&, ResourceRequest&&, unsigned long, const ResourceResponse&);
 
-    bool isXMLHttpRequest() const final;
-
     DocumentThreadableLoader& m_loader;
     CachedResourceHandle<CachedRawResource> m_resource;
     ResourceRequest m_request;
index f3a063c..c4816d4 100644 (file)
@@ -125,60 +125,6 @@ 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())) {
-        if (!loader->frameLoader() || !loader->frameLoader()->frame().page())
-            return false;
-
-        CachedResource* cachedResource = MemoryCache::singleton().resourceForRequest(loader->request(), loader->frameLoader()->frame().page()->sessionID());
-        if (!cachedResource)
-            return false;
-
-        // 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 (shouldPendingCachedResourceLoadPreventPageCache(*cachedResource))
-            return false;
-    }
-    return true;
-}
-
 DocumentLoader::DocumentLoader(const ResourceRequest& request, const SubstituteData& substituteData)
     : FrameDestructionObserver(nullptr)
     , m_cachedResourceLoader(CachedResourceLoader::create(this))
@@ -314,13 +260,6 @@ void DocumentLoader::stopLoading()
     // to stop loading. Because of this, we need to save it so we don't return early.
     bool loading = isLoading();
 
-    // We may want to audit the existing subresource loaders when we are on a page which has completed
-    // loading but there are subresource loads during cancellation. This must be done before the
-    // frame->stopLoading() call, which may evict the CachedResources, which we rely on to check
-    // the type of the resource loads.
-    if (loading && m_committed && !mainResourceLoader() && !m_subresourceLoaders.isEmpty())
-        m_subresourceLoadersArePageCacheAcceptable = areAllLoadersPageCacheAcceptable(m_subresourceLoaders);
-
     if (m_committed) {
         // Attempt to stop the frame if the document loader is loading, or if it is done loading but
         // still  parsing. Failure to do so can cause a world leak.
index 998ff5a..a85a076 100644 (file)
@@ -268,8 +268,6 @@ public:
     bool didCreateGlobalHistoryEntry() const { return m_didCreateGlobalHistoryEntry; }
     void setDidCreateGlobalHistoryEntry(bool didCreateGlobalHistoryEntry) { m_didCreateGlobalHistoryEntry = didCreateGlobalHistoryEntry; }
 
-    bool subresourceLoadersArePageCacheAcceptable() const { return m_subresourceLoadersArePageCacheAcceptable; }
-
     void setDefersLoading(bool);
     void setMainResourceDataBufferingPolicy(DataBufferingPolicy);
 
index 5e79916..a9bf4b7 100644 (file)
@@ -655,11 +655,6 @@ bool DocumentThreadableLoader::isAllowedRedirect(const URL& url)
     return m_sameOriginRequest && securityOrigin().canRequest(url);
 }
 
-bool DocumentThreadableLoader::isXMLHttpRequest() const
-{
-    return m_options.initiator == cachedResourceRequestInitiators().xmlhttprequest;
-}
-
 SecurityOrigin& DocumentThreadableLoader::securityOrigin() const
 {
     return m_origin ? *m_origin : m_document.securityOrigin();
index ca25ec5..e9e48d2 100644 (file)
@@ -102,8 +102,6 @@ namespace WebCore {
         bool isAllowedRedirect(const URL&);
         bool isAllowedByContentSecurityPolicy(const URL&, ContentSecurityPolicy::RedirectResponseReceived);
 
-        bool isXMLHttpRequest() const final;
-
         SecurityOrigin& securityOrigin() const;
         const ContentSecurityPolicy& contentSecurityPolicy() const;
 
index 761a10b..7033c51 100644 (file)
@@ -869,18 +869,6 @@ unsigned CachedResource::overheadSize() const
     return sizeof(CachedResource) + m_response.memoryUsage() + kAverageClientsHashMapSize + m_resourceRequest.url().string().length() * 2;
 }
 
-bool CachedResource::areAllClientsXMLHttpRequests() const
-{
-    if (type() != Type::RawResource)
-        return false;
-
-    for (auto& client : m_clients) {
-        if (!client.key->isXMLHttpRequest())
-            return false;
-    }
-    return true;
-}
-
 void CachedResource::setLoadPriority(const Optional<ResourceLoadPriority>& loadPriority)
 {
     if (loadPriority)
index 2ffe2b4..fc30b0f 100644 (file)
@@ -165,8 +165,6 @@ public:
 
     SubresourceLoader* loader() { return m_loader.get(); }
 
-    bool areAllClientsXMLHttpRequests() const;
-
     bool isImage() const { return type() == Type::ImageResource; }
     // FIXME: CachedRawResource could be a main resource, an audio/video resource, or a raw XHR/icon resource.
     bool isMainOrMediaOrIconOrRawResource() const { return type() == Type::MainResource || type() == Type::MediaResource || type() == Type::Icon || type() == Type::RawResource || type() == Type::Beacon || type() == Type::Ping; }
index 30bb1e8..793677d 100644 (file)
@@ -41,7 +41,6 @@ public:
     virtual ~CachedResourceClient() = default;
     virtual void notifyFinished(CachedResource&) { }
     virtual void deprecatedDidReceiveCachedResource(CachedResource&) { }
-    virtual bool isXMLHttpRequest() const { return false; }
 
     static CachedResourceClientType expectedType() { return BaseResourceType; }
     virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
index 1796fe8..b9e6186 100644 (file)
@@ -233,11 +233,6 @@ String DiagnosticLoggingKeys::otherKey()
     return "other"_s;
 }
 
-String DiagnosticLoggingKeys::mainDocumentErrorKey()
-{
-    return "mainDocumentError"_s;
-}
-
 String DiagnosticLoggingKeys::mainResourceKey()
 {
     return "mainResource"_s;
index fd944f0..d459c4d 100644 (file)
@@ -88,7 +88,6 @@ public:
     WEBCORE_EXPORT static String isReloadIgnoringCacheDataKey();
     static String loadingKey();
     static String isLoadingKey();
-    static String mainDocumentErrorKey();
     static String mainResourceKey();
     static String mediaLoadedKey();
     static String mediaLoadingFailedKey();