Bug 117112 – REGRESSION (r130783): Going "back" to a cached page from a page with a main resource error breaks scrolling, amongst other issues

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

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>

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

And since layout tests are flaky in general on my local machine, I'd like to see was EWS has to say.

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

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)

[Brady Eidson]

Created attachment 203543 [details]
Patch v2 - Landable if tests pass

[Alexey Proskuryakov]

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?

[Brady Eidson]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 203549 [details]
Patch for landing

Figured out the layouttest mess.  Will let EWS chew on this.

[Brady Eidson]

http://trac.webkit.org/changeset/151088