REGRESSION (r130783): Going "back" to a cached page from a page with a main resource error breaks scrolling This includes pages with a long running main resource load where the acting of clicking "back" cancels that main resource load. In these situations where the current page's main resource ended in an error, the Document's parsing was not cancelled and cleared out. Later, when restoring the cached page, FrameLoader::clear() calls Document::cancelParsing(), which ends up closing the Document, which ends up causing a whole bunch of side effects such as checking whether or not the frame load is complete and then dispatching didFinishLoad. Reading the intent of cancelParsing() all the way down the rabbit hole, it's clear that it was meant act on the Document for the current load. But when FrameLoader::clear() is called during a cached page load it's *after* the cached page load has already started, and cancelParsing() doesn't affect the previous load. So, in this case, the callbacks for the cached page load come in the following order: didStartProvisionalLoad didFinishLoad <-- erroneously emitted during Document::cancelParsing() didCommitLoad By design, cancelParsing() is meant to be called multiple times per load, but it only does its dirty work the first time its called. Normally it is called after the main document finishes loading. But it apparently is never called when the main document load ends in error. This doesn't cause a problem for normal page loads, because that process calls clear() very early on before any other work happens. But cached page loads - for better or worse - need to follow a different code path. It makes sense to me to call cancelParsing() the moment a main resource load finishes - whether it was due to completing successfully *or* due to error. This will at least immediately free up the resources associated with the parser instead of waiting for them to be freed on the next navigation. In radar as <rdar://problem/13751844>
Created attachment 203496 [details] Reduction to reproduce. Drop the attached files in a PHP enabled web server and then visit index.html. Go to "slow.php" and while it's loading, go back. You'll notice that you can't scroll. In certain older builds you'll even crash. Listening to the load callbacks you'll also see the out-of-orderness I described.
Created attachment 203497 [details] Patch v1 - Current proposed fix I'm still playing with this locally and know this isn't ready for review, but currently I think the fix might be this simple.
And since layout tests are flaky in general on my local machine, I'd like to see was EWS has to say.
Comment on attachment 203497 [details] Patch v1 - Current proposed fix Attachment 203497 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/659321 New failing tests: media/video-controls-in-media-document.html http/tests/media/media-document.html webarchive/loading/video-in-webarchive.html media/click-volume-bar-not-pausing.html media/video-element-other-namespace-crash.html media/video-click-dblckick-standalone.html fast/events/media-focus-in-standalone-media-document.html media/media-document-audio-size.html
Created attachment 203499 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
(In reply to comment #4) > (From update of attachment 203497 [details]) > Attachment 203497 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/659321 > > New failing tests: > media/video-controls-in-media-document.html > http/tests/media/media-document.html > webarchive/loading/video-in-webarchive.html > media/click-volume-bar-not-pausing.html > media/video-element-other-namespace-crash.html > media/video-click-dblckick-standalone.html > fast/events/media-focus-in-standalone-media-document.html > media/media-document-audio-size.html Most of these fail on my machine locally *WITHOUT* this patch. I'll try to wade through the differences.
Comment on attachment 203497 [details] Patch v1 - Current proposed fix Attachment 203497 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/661388 New failing tests: media/video-controls-in-media-document.html http/tests/media/media-document.html webarchive/loading/video-in-webarchive.html media/click-volume-bar-not-pausing.html media/video-element-other-namespace-crash.html media/video-click-dblckick-standalone.html fast/events/media-focus-in-standalone-media-document.html media/media-document-audio-size.html
Created attachment 203504 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 203497 [details] Patch v1 - Current proposed fix Attachment 203497 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/750396 New failing tests: media/video-controls-in-media-document.html http/tests/media/media-document.html webarchive/loading/video-in-webarchive.html media/click-volume-bar-not-pausing.html media/video-element-other-namespace-crash.html media/video-click-dblckick-standalone.html fast/events/media-focus-in-standalone-media-document.html media/media-document-audio-size.html
Created attachment 203507 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.3
That last patch did indeed change the equation a bit in media-land. Not at all clear why - they must use the parser in some unconventional ways. This new patch limits the change, and also includes a layout test and changelog - It can be landed if EWS checks out (I'm also in the process of running tests locally)
Created attachment 203543 [details] Patch v2 - Landable if tests pass
Comment on attachment 203543 [details] Patch v2 - Landable if tests pass View in context: https://bugs.webkit.org/attachment.cgi?id=203543&action=review r=me > LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt:26 > +This test makes sure that going back to a cached page from a page that is still loading produces the correct frame load callbacks. It is only useful inside of WebKitTestRunner. Would be helpful to have criteria for passing explained. How will we know if any change in the long list of callbacks above means a horrible failure?
(In reply to comment #13) > (From update of attachment 203543 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203543&action=review > > r=me > > > LayoutTests/http/tests/loading/unfinished-load-back-to-cached-page-callbacks-expected.txt:26 > > +This test makes sure that going back to a cached page from a page that is still loading produces the correct frame load callbacks. It is only useful inside of WebKitTestRunner. > > Would be helpful to have criteria for passing explained. How will we know if any change in the long list of callbacks above means a horrible failure? The list of callbacks is long, but fortunately the target of the fix is only the last three - I can definitely add an explanation. Thanks for the review - I'll make sure the layouttest situation is fully understood before landing.
Comment on attachment 203543 [details] Patch v2 - Landable if tests pass Attachment 203543 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/704594 New failing tests: http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
Created attachment 203545 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 203543 [details] Patch v2 - Landable if tests pass Attachment 203543 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/755243 New failing tests: http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html
Created attachment 203547 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
(In reply to comment #17) > (From update of attachment 203543 [details]) > Attachment 203543 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/755243 > > New failing tests: > http/tests/loading/unfinished-load-back-to-cached-page-callbacks.html Yah, my new test is flawed somehow. I don't know why. Passes 100% standalone, flakes with http/tests/loading, and seems to fail when run with much more than ~30 tests. I suspect a bug in the testing harness with frame load callbacks, actually. But don't have time to pursue that - I have a different strategy for the test for now.
Created attachment 203549 [details] Patch for landing Figured out the layouttest mess. Will let EWS chew on this.
http://trac.webkit.org/changeset/151088