Inconsistencies in main resource load delegates when loading from history
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Jan 2016 08:23:36 +0000 (08:23 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Jan 2016 08:23:36 +0000 (08:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150927

Reviewed by Michael Catanzaro.

Source/WebCore:

When restoring a page from the page cache, even though there
isn't an actual load of resources, we are still emitting the load
delegates to let the API layer know there are contents being
loaded in the web view. This makes the page cache restoring
transparent for the API layer. However, when restoring a page from
the cache, all the delegates are emitted after the load is
committed. This is not consistent with real loads, where we first
load the main resource and once we get a response we commit the
load. This inconsistency is problematic if the API layer expects
to always have a main resource with a response when the load is
committed. This is the case of the GTK+ port, for example. So,
this patch ensures that when a page is restored from the page
cache, the main resource load delegates that are emitted until a
response is received in normal loads, are emitted before the load
is committed.

Test: http/tests/loading/main-resource-delegates-on-back-navigation.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::commitProvisionalLoad): When loading from
the page cache, send delegate messages up to didReceiveResponse
for the main resource before the load is committed, and the
remaining messages afterwards.

LayoutTests:

Add test to check that main resource load delegates are emitted in
the same order before the load is committed when loading a page
from history with the page cache enabled and disabled.

* http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt: Added.
* http/tests/loading/main-resource-delegates-on-back-navigation.html: Added.
* http/tests/loading/resources/page-go-back-onload.html: Added.
* loader/go-back-cached-main-resource-expected.txt:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation.html [new file with mode: 0644]
LayoutTests/http/tests/loading/resources/page-go-back-onload.html [new file with mode: 0644]
LayoutTests/loader/go-back-cached-main-resource-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp

index 7347e26..6916d6e 100644 (file)
@@ -1,3 +1,19 @@
+2016-01-11  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        Inconsistencies in main resource load delegates when loading from history
+        https://bugs.webkit.org/show_bug.cgi?id=150927
+
+        Reviewed by Michael Catanzaro.
+
+        Add test to check that main resource load delegates are emitted in
+        the same order before the load is committed when loading a page
+        from history with the page cache enabled and disabled.
+
+        * http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt: Added.
+        * http/tests/loading/main-resource-delegates-on-back-navigation.html: Added.
+        * http/tests/loading/resources/page-go-back-onload.html: Added.
+        * loader/go-back-cached-main-resource-expected.txt:
+
 2016-01-11  Johan K. Jensen  <jj@johanjensen.dk>
 
         Web Inspector: console.count() shouldn't show a colon in front of a number
diff --git a/LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt b/LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt
new file mode 100644 (file)
index 0000000..a268402
--- /dev/null
@@ -0,0 +1,41 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didFinishLoading
+main frame - willPerformClientRedirectToURL: http://127.0.0.1:8000/loading/resources/page-go-back-onload.html 
+main frame - didStartProvisionalLoadForFrame
+http://127.0.0.1:8000/loading/resources/page-go-back-onload.html - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/resources/page-go-back-onload.html, main document URL http://127.0.0.1:8000/loading/resources/page-go-back-onload.html, http method GET> redirectResponse (null)
+http://127.0.0.1:8000/loading/resources/page-go-back-onload.html - didReceiveResponse <NSURLResponse http://127.0.0.1:8000/loading/resources/page-go-back-onload.html, http status code 200>
+main frame - didCancelClientRedirectForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+http://127.0.0.1:8000/loading/resources/page-go-back-onload.html - didFinishLoading
+main frame - didStartProvisionalLoadForFrame
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html, main document URL http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html, http method GET> redirectResponse (null)
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didReceiveResponse <NSURLResponse http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html, http status code 200>
+main frame - didCommitLoadForFrame
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didFinishLoading
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+main frame - willPerformClientRedirectToURL: http://127.0.0.1:8000/loading/resources/page-go-back-onload.html 
+main frame - didStartProvisionalLoadForFrame
+http://127.0.0.1:8000/loading/resources/page-go-back-onload.html - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/resources/page-go-back-onload.html, main document URL http://127.0.0.1:8000/loading/resources/page-go-back-onload.html, http method GET> redirectResponse (null)
+http://127.0.0.1:8000/loading/resources/page-go-back-onload.html - didReceiveResponse <NSURLResponse http://127.0.0.1:8000/loading/resources/page-go-back-onload.html, http status code 200>
+main frame - didCancelClientRedirectForFrame
+main frame - didCommitLoadForFrame
+http://127.0.0.1:8000/loading/resources/page-go-back-onload.html - didFinishLoading
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+main frame - didStartProvisionalLoadForFrame
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html, main document URL http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html, http method GET> redirectResponse (null)
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didReceiveResponse <NSURLResponse http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html, http status code 200>
+main frame - didCommitLoadForFrame
+http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didFinishLoading
+main frame - didFinishLoadForFrame
+Test that main resource load delegates are the same when page cache is enabled and disabled
diff --git a/LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation.html b/LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation.html
new file mode 100644 (file)
index 0000000..0b3e85b
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<body>
+Test that main resource load delegates are the same when page cache is enabled and disabled
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.dumpResourceLoadCallbacks();
+    testRunner.waitUntilDone();
+}
+
+window.addEventListener('pageshow', function() {
+    if (!sessionStorage.testLoadCount)
+        sessionStorage.testLoadCount = 1;
+    else
+        sessionStorage.testLoadCount = parseInt(sessionStorage.testLoadCount, 10) + 1
+    var pageCacheEnabled;
+    switch(parseInt(sessionStorage.testLoadCount, 10)) {
+    case 1:
+        pageCacheEnabled = 0;
+        break;
+    case 2:
+        pageCacheEnabled = 1;
+        break;
+    case 3:
+        setTimeout(function() {
+            delete sessionStorage.testLoadCount;
+            testRunner.notifyDone();
+        }, 0);
+        break;
+    }
+
+    if (window.testRunner)
+        testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", pageCacheEnabled);
+
+    setTimeout(function() {
+        window.location.href = "resources/page-go-back-onload.html";
+    }, 0);
+}, false);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/loading/resources/page-go-back-onload.html b/LayoutTests/http/tests/loading/resources/page-go-back-onload.html
new file mode 100644 (file)
index 0000000..260dce5
--- /dev/null
@@ -0,0 +1,7 @@
+<script>
+window.addEventListener("load", function() {
+    setTimeout(function() {
+        history.back();
+    }, 0);
+}, false);
+</script>
index 0be61de..8bbb007 100644 (file)
@@ -11,7 +11,7 @@ resources/first-page.html - didFinishLoading
 resources/other-page.html - willSendRequest <NSURLRequest URL resources/other-page.html, main document URL resources/other-page.html, http method GET> redirectResponse (null)
 resources/other-page.html - didReceiveResponse <NSURLResponse resources/other-page.html, http status code 0>
 resources/other-page.html - didFinishLoading
-resources/first-page.html - willSendRequest <NSURLRequest URL resources/first-page.html, main document URL (null), http method GET> redirectResponse (null)
+resources/first-page.html - willSendRequest <NSURLRequest URL resources/first-page.html, main document URL resources/first-page.html, http method GET> redirectResponse (null)
 resources/first-page.html - didReceiveResponse <NSURLResponse resources/first-page.html, http status code 0>
 resources/first-page.html - didFinishLoading
 This test check the following situation:
index 6a3669b..14c3f51 100644 (file)
@@ -1,3 +1,34 @@
+2016-01-11  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        Inconsistencies in main resource load delegates when loading from history
+        https://bugs.webkit.org/show_bug.cgi?id=150927
+
+        Reviewed by Michael Catanzaro.
+
+        When restoring a page from the page cache, even though there
+        isn't an actual load of resources, we are still emitting the load
+        delegates to let the API layer know there are contents being
+        loaded in the web view. This makes the page cache restoring
+        transparent for the API layer. However, when restoring a page from
+        the cache, all the delegates are emitted after the load is
+        committed. This is not consistent with real loads, where we first
+        load the main resource and once we get a response we commit the
+        load. This inconsistency is problematic if the API layer expects
+        to always have a main resource with a response when the load is
+        committed. This is the case of the GTK+ port, for example. So,
+        this patch ensures that when a page is restored from the page
+        cache, the main resource load delegates that are emitted until a
+        response is received in normal loads, are emitted before the load
+        is committed.
+
+        Test: http/tests/loading/main-resource-delegates-on-back-navigation.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::commitProvisionalLoad): When loading from
+        the page cache, send delegate messages up to didReceiveResponse
+        for the main resource before the load is committed, and the
+        remaining messages afterwards.
+
 2016-01-09  Andy Estes  <aestes@apple.com>
 
         [Cocoa] Add SPI to opt out a URL scheme from the memory cache
index 4fd1601..e0ab34f 100644 (file)
@@ -1800,6 +1800,14 @@ void FrameLoader::commitProvisionalLoad()
 #endif
         prepareForCachedPageRestore();
 
+        // Start request for the main resource and dispatch didReceiveResponse before the load is committed for
+        // consistency with all other loads. See https://bugs.webkit.org/show_bug.cgi?id=150927.
+        ResourceError mainResouceError;
+        unsigned long mainResourceIdentifier;
+        ResourceRequest mainResourceRequest(cachedPage->documentLoader()->request());
+        requestFromDelegate(mainResourceRequest, mainResourceIdentifier, mainResouceError);
+        notifier().dispatchDidReceiveResponse(cachedPage->documentLoader(), mainResourceIdentifier, cachedPage->documentLoader()->response());
+
         // FIXME: This API should be turned around so that we ground CachedPage into the Page.
         cachedPage->restore(*m_frame.page());
 
@@ -1813,6 +1821,10 @@ void FrameLoader::commitProvisionalLoad()
         if (!title.isNull())
             m_client.dispatchDidReceiveTitle(title);
 
+        // Send remaining notifications for the main resource.
+        notifier().sendRemainingDelegateMessages(m_documentLoader.get(), mainResourceIdentifier, mainResourceRequest, ResourceResponse(),
+            nullptr, static_cast<int>(m_documentLoader->response().expectedContentLength()), 0, mainResouceError);
+
         checkCompleted();
     } else
         didOpenURL();
@@ -1839,7 +1851,9 @@ void FrameLoader::commitProvisionalLoad()
         m_frame.view()->forceLayout();
 #endif
 
-        for (auto& response : m_documentLoader->responses()) {
+        // Main resource delegates were already sent, so we skip the first response here.
+        for (unsigned i = 1; i < m_documentLoader->responses().size(); ++i) {
+            const auto& response = m_documentLoader->responses()[i];
             // FIXME: If the WebKit client changes or cancels the request, this is not respected.
             ResourceError error;
             unsigned long identifier;