Title: [151088] trunk
- Revision
- 151088
- Author
- [email protected]
- Date
- 2013-06-02 18:43:24 -0700 (Sun, 02 Jun 2013)
Log Message
Going "back" to a cached page from a page with a main resource error breaks scrolling, amongst other issues.
<rdar://problem/13751844> and https://bugs.webkit.org/show_bug.cgi?id=117112
Reviewed by Alexey Proskuryakov.
Source/WebCore:
If a main resource load ends in error, the Document's parser is never cleared out.
If you then return to a CachedPage, we run the Document->clearParser() step for the
old page and that incorrectly dispatches didFinishLoad for the previous page load
in the middle of the load for the cached page.
The parser should never be needed after a load completes (even if it fails) and
holding on to the parser after the page load failed but before a new navigation is
actually using unnecessary resources.
So we should just clear the parser right when the main resource fails.
Test: http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::stopLoading): After canceling the main resource load, also clear the parser.
LayoutTests:
* http/tests/loading/resources/resource-that-goes-back-while-still-loading.php: Added.
* http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt: Added.
* http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (151087 => 151088)
--- trunk/LayoutTests/ChangeLog 2013-06-02 23:34:09 UTC (rev 151087)
+++ trunk/LayoutTests/ChangeLog 2013-06-03 01:43:24 UTC (rev 151088)
@@ -1,3 +1,14 @@
+2013-06-02 Brady Eidson <[email protected]>
+
+ Going "back" to a cached page from a page with a main resource error breaks scrolling, amongst other issues.
+ <rdar://problem/13751844> and https://bugs.webkit.org/show_bug.cgi?id=117112
+
+ Reviewed by Alexey Proskuryakov.
+
+ * http/tests/loading/resources/resource-that-goes-back-while-still-loading.php: Added.
+ * http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt: Added.
+ * http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html: Added.
+
2013-06-02 Alexey Proskuryakov <[email protected]>
The empty directory did not get deleted by commit queue, deleting manually.
Added: trunk/LayoutTests/http/tests/loading/resources/resource-that-goes-back-while-still-loading.php (0 => 151088)
--- trunk/LayoutTests/http/tests/loading/resources/resource-that-goes-back-while-still-loading.php (rev 0)
+++ trunk/LayoutTests/http/tests/loading/resources/resource-that-goes-back-while-still-loading.php 2013-06-03 01:43:24 UTC (rev 151088)
@@ -0,0 +1,20 @@
+<?php
+
+@apache_setenv('no-gzip', 1);
+@ini_set('zlib.output_compression', 0);
+@ini_set('implicit_flush', 1);
+for ($i = 0; $i < ob_get_level(); $i++) { ob_end_flush(); }
+ob_implicit_flush(1);
+
+header("HTTP/1.1 200 OK"); flush();
+
+echo str_repeat(" ", 1024)."<pre>"; flush();
+echo "This page should trigger a back navigation while it's still loading.<br>\r\n";
+echo "<script>setTimeout('window.history.back()', 0);</script>";
+
+while(true)
+{
+ echo "Still loading...<br>\r\n"; flush(); sleep(1);
+}
+
+?>
Added: trunk/LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt (0 => 151088)
--- trunk/LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt 2013-06-03 01:43:24 UTC (rev 151088)
@@ -0,0 +1,29 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didFailLoadWithError
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishLoadForFrame
+This test dumps frame load callbacks. It is only useful inside of WebKitTestRunner.
+
+This link goes to the slow loading page.s Bug 117112 - Going "back" to a cached page from a page with a main resource error breaks scrolling, amongst other issues
+
+In the broken case, the second page gets a didFinishLoad callback intertwined with the restoration of the cached page, even though it's already gotten a didFailLoad callback.
+The final 4 callbacks look like:
+didFailLoadWithError
+didStartProvisionalLoadForFrame
+didFinishLoadForFrame
+didCommitLoadForFrame
+
+When fixed, the final 4 callbacks should be:
+didFailLoadWithError
+didStartProvisionalLoadForFrame
+didCommitLoadForFrame
+didFinishLoadForFrame
+
Added: trunk/LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html (0 => 151088)
--- trunk/LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html (rev 0)
+++ trunk/LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html 2013-06-03 01:43:24 UTC (rev 151088)
@@ -0,0 +1,48 @@
+<script>
+
+window._onpageshow_ = function(evt) {
+ if (!window.testRunner)
+ return;
+
+ if (!evt.persisted)
+ return;
+
+ setTimeout("testRunner.notifyDone()", 0);
+}
+
+function clickLinkToSlowPage() {
+ var link = document.getElementById("linkToSlowPage");
+ var linkRect = link.getClientRects()[0];
+ var targetX = linkRect.left + linkRect.width / 2;
+ var targetY = linkRect.top + linkRect.height / 2;
+ eventSender.mouseMoveTo(targetX, targetY);
+ eventSender.mouseDown();
+ eventSender.mouseUp();
+}
+
+window._onload_ = function(evt) {
+ setTimeout("clickLinkToSlowPage()", 0);
+}
+
+if (window.testRunner) {
+ testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+} else
+ document.write('This test must be run by WebKitTestRunner!');
+</script>
+
+This test dumps frame load callbacks. It is only useful inside of WebKitTestRunner.<br><br>
+<a id="linkToSlowPage" href="" link goes to the slow loading page.</a>s
+Bug <a href="" - Going "back" to a cached page from a page with a main resource error breaks scrolling, amongst other issues</a><br><br>
+In the broken case, the second page gets a didFinishLoad callback intertwined with the restoration of the cached page, even though it's already gotten a didFailLoad callback.<br>
+The final 4 callbacks look like:<br>
+didFailLoadWithError<br>
+didStartProvisionalLoadForFrame<br>
+didFinishLoadForFrame<br>
+didCommitLoadForFrame<br><br>
+When fixed, the final 4 callbacks should be:<br>
+didFailLoadWithError<br>
+didStartProvisionalLoadForFrame<br>
+didCommitLoadForFrame<br>
+didFinishLoadForFrame<br>
Modified: trunk/Source/WebCore/ChangeLog (151087 => 151088)
--- trunk/Source/WebCore/ChangeLog 2013-06-02 23:34:09 UTC (rev 151087)
+++ trunk/Source/WebCore/ChangeLog 2013-06-03 01:43:24 UTC (rev 151088)
@@ -1,3 +1,27 @@
+2013-06-02 Brady Eidson <[email protected]>
+
+ Going "back" to a cached page from a page with a main resource error breaks scrolling, amongst other issues.
+ <rdar://problem/13751844> and https://bugs.webkit.org/show_bug.cgi?id=117112
+
+ Reviewed by Alexey Proskuryakov.
+
+ If a main resource load ends in error, the Document's parser is never cleared out.
+
+ If you then return to a CachedPage, we run the Document->clearParser() step for the
+ old page and that incorrectly dispatches didFinishLoad for the previous page load
+ in the middle of the load for the cached page.
+
+ The parser should never be needed after a load completes (even if it fails) and
+ holding on to the parser after the page load failed but before a new navigation is
+ actually using unnecessary resources.
+
+ So we should just clear the parser right when the main resource fails.
+
+ Test: http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
+
+ * loader/DocumentLoader.cpp:
+ (WebCore::DocumentLoader::stopLoading): After canceling the main resource load, also clear the parser.
+
2013-06-02 Csaba Osztrogonác <[email protected]>
[CMake] Unreviewed buildfix after r148896 and r150940.
Modified: trunk/Source/WebCore/loader/DocumentLoader.cpp (151087 => 151088)
--- trunk/Source/WebCore/loader/DocumentLoader.cpp 2013-06-02 23:34:09 UTC (rev 151087)
+++ trunk/Source/WebCore/loader/DocumentLoader.cpp 2013-06-03 01:43:24 UTC (rev 151088)
@@ -294,10 +294,16 @@
FrameLoader* frameLoader = DocumentLoader::frameLoader();
- if (isLoadingMainResource())
+ if (isLoadingMainResource()) {
// Stop the main resource loader and let it send the cancelled message.
cancelMainResourceLoad(frameLoader->cancelledError(m_request));
- else if (!m_subresourceLoaders.isEmpty())
+
+ // When cancelling the main resource load, we need to also cancel the Document's parser.
+ // Otherwise cancelling the parser when starting the next page load might result
+ // in unexpected side effects such as erroneous event dispatch. ( http://webkit.org/b/117112 )
+ if (Document* doc = document())
+ doc->cancelParsing();
+ } else if (!m_subresourceLoaders.isEmpty())
// The main resource loader already finished loading. Set the cancelled error on the
// document and let the subresourceLoaders send individual cancelled messages below.
setMainDocumentError(frameLoader->cancelledError(m_request));
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
