Bug 52388 – REGRESSION: Assertion failure in FrameLoader::continueLoadAfterWillSubmitForm() when navigating back to an unreachable URL

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

[Jessie Berlin]

First noticed in r75714 on both Windows & Mac

STEPS TO REPRODUCE:

1. Go to an unreachable URL (e.g. aljda;ldfjkal;sdjfa;df.com)
2. Go to a reachable URL (e.g. http://webkit.org)
3. Go back.

Note the following assertion failure:

FrameLoader::continueLoadAfterWillSubmitForm()

with the following stack trace:

WebKit.dll!WebCore::FrameLoader::continueLoadAfterWillSubmitForm()  Line 2450 + 0x3c bytes    C++
WebKit.dll!WebCore::FrameLoader::continueLoadAfterNavigationPolicy(const WebCore::ResourceRequest & __formal={...}, WTF::PassRefPtr<WebCore::FormState> formState={...}, bool shouldContinue=true)  Line 2991    C++
WebKit.dll!WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void * argument=0x0becd038, const WebCore::ResourceRequest & request={...}, WTF::PassRefPtr<WebCore::FormState> formState={...}, bool shouldContinue=true)  Line 2865    C++
WebKit.dll!WebCore::PolicyChecker::checkNavigationPolicy(const WebCore::ResourceRequest & request={...}, WebCore::DocumentLoader * loader=0x0bbcff88, WTF::PassRefPtr<WebCore::FormState> formState={...}, void (void *, const WebCore::ResourceRequest &, WTF::PassRefPtr<WebCore::FormState>, bool)* function=0x6697c570, void * argument=0x0becd038)  Line 78 + 0x17 bytes    C++
 WebKit.dll!WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader * loader=0x0bbcff88, WebCore::FrameLoadType type=FrameLoadTypeBack, WTF::PassRefPtr<WebCore::FormState> prpFormState={...})  Line 1485    C++
    WebKit.dll!WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem * item=0x0bb5b098, WebCore::FrameLoadType loadType=FrameLoadTypeBack)  Line 3171    C++
    WebKit.dll!WebCore::FrameLoader::loadItem(WebCore::HistoryItem * item=0x0bb5b098, WebCore::FrameLoadType loadType=FrameLoadTypeBack)  Line 3270    C++
    WebKit.dll!WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem * item=0x0bb5b098, WebCore::HistoryItem * fromItem=0x0be9d6e8, WebCore::FrameLoadType type=FrameLoadTypeBack)  Line 650    C++
    WebKit.dll!WebCore::HistoryController::goToItem(WebCore::HistoryItem * targetItem=0x0bb5b098, WebCore::FrameLoadType type=FrameLoadTypeBack)  Line 246    C++
    WebKit.dll!WebCore::Page::goToItem(WebCore::HistoryItem * item=0x0bb5b098, WebCore::FrameLoadType type=FrameLoadTypeBack)  Line 363    C++
    WebKit.dll!WebCore::Page::goBack()  Line 285    C++

<rdar://problem/8853432>

This appears to be happening because in FrameLoader::load, we change the FrameLoadType to be FrameLoadTypeReload so that we try loading the unreachable URL again. This causes us to not reset the navigation start in FrameLoader::continueLoadAfterNavigationPolicy before calling continueLoadAfterWillSubmitForm.

[Jessie Berlin]

Created attachment 78840 [details]
Patch (idea for a fix, needs looking over and needs tests added once the approach is approved)

The attached patch is a speculative fix Anders and I came up with this morning. Adam Barth, could you give your opinion on this approach? (others feel free to weigh in as well).

[Jessie Berlin]

A better fix for this bug is to never cache error pages (these can be recognized as pages with substitute data or unreachable URLs). I will send out a new patch with that fix and tests.

[Jessie Berlin]

Created attachment 78880 [details]
Patch

[WebKit Review Bot]

Attachment 78880 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7558024

[WebKit Review Bot]

Attachment 78880 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7570025

[Alexey Proskuryakov]

+        Do not cache any error pages (which we can recognize as having substitute data and/or an
+        unreachableURL).

Pages loaded from appcache also have substitute data, although I'm not sure if failingURL() is set. It probably is when displaying fallback resources.

+        * DumpRenderTree/LayoutTestController.cpp:

At this point, we should probably be making WebKit2 WebKitTestRunner tests right away.

[Alexey Proskuryakov]

I'm not sure which behavior is best for appcache. It probably makes sense to at least test that we're not crashing or hitting an assertion - see http/tests/appcache/offline-access.html for an example of testing offline appcache behavior.

[Jessie Berlin]

(In reply to comment #7)
> I'm not sure which behavior is best for appcache. It probably makes sense to at least test that we're not crashing or hitting an assertion - see http/tests/appcache/offline-access.html for an example of testing offline appcache behavior.

I ran that test on Mac with these changes applied and it worked fine.

I then decided to try out a demo for Application Cache (http://people.opera.com/shwetankd/demos/2/index.htm), and it still works fine.

Talking to Anders, he pointed out that ApplicationCache document loaders don't have unreachable URLs.

Since we do not want to expose the ability to do loading in the Injected Bundle, making this test work for WebKit2 would require a message up the UI Process to call WKPageLoadAlternateHTMLString. Anders suggested I do that in a separate patch.

I will try to fix the Chromium build errors in my next version of this patch.

[Alexey Proskuryakov]

> I ran that test on Mac with these changes applied and it worked fine.

FWIW, I'm pretty sure that we have no tests that involve going back to documents coming from appcache.

[Jessie Berlin]

Created attachment 79032 [details]
Patch

I have tested the AppCache by hand (going back to a page that uses - with and without this patch applied, without internet connectivity - works), and will be working on getting this test working in WTR in another patch.

[WebKit Review Bot]

Attachment 79032 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7551059

[Jessie Berlin]

Created attachment 79038 [details]
Patch

Fix Chromium Build errors.

[Jessie Berlin]

After looking just a little more into it, we would need to port the WorkQueue concept over to WebKitTestRunner before making this test work with WebKit2 tests.

That is non-trivial, and I am looking into it now. It should definitely be a separate patch.

[Maciej Stachowiak]

(In reply to comment #10)
> Created an attachment (id=79032) [details]
> Patch
> 
> I have tested the AppCache by hand (going back to a page that uses - with and without this patch applied, without internet connectivity - works),

Did you test whether pages coming from the AppCache can successfully go into the page cache? Going back to AppCache pages without internet connectivity works even if they are not in the page cache. A way to test whether the AppCache page is going into the page cache is to make a DOM change (e.g. with the Web Inspector) and see if it's still there after you go back.

[Anders Carlsson]

(In reply to comment #14)
> (In reply to comment #10)
> > Created an attachment (id=79032) [details] [details]
> > Patch
> > 
> > I have tested the AppCache by hand (going back to a page that uses - with and without this patch applied, without internet connectivity - works),
> 
> Did you test whether pages coming from the AppCache can successfully go into the page cache? Going back to AppCache pages without internet connectivity works even if they are not in the page cache. A way to test whether the AppCache page is going into the page cache is to make a DOM change (e.g. with the Web Inspector) and see if it's still there after you go back.

Pages from the application cache aren't put in the page cache, nor do they have an unreachable URL.

[Anders Carlsson]

Comment on attachment 79038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79038&action=review

> Source/WebCore/history/PageCache.cpp:255
> +        // Do not cache error pages (these can be recognized as pages with substitute data or unreachable URLs).

Please move the check up here so that the call to FrameLoaderClient::canCachePage is the last call.

[Jessie Berlin]

(In reply to comment #16)
> (From update of attachment 79038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79038&action=review
> 
> > Source/WebCore/history/PageCache.cpp:255
> > +        // Do not cache error pages (these can be recognized as pages with substitute data or unreachable URLs).
> 
> Please move the check up here so that the call to FrameLoaderClient::canCachePage is the last call.

Done. Thanks for the review!

[Jessie Berlin]

(In reply to comment #14)
> (In reply to comment #10)
> > Created an attachment (id=79032) [details] [details]
> > Patch
> > 
> > I have tested the AppCache by hand (going back to a page that uses - with and without this patch applied, without internet connectivity - works),
> 
> Did you test whether pages coming from the AppCache can successfully go into the page cache? Going back to AppCache pages without internet connectivity works even if they are not in the page cache. A way to test whether the AppCache page is going into the page cache is to make a DOM change (e.g. with the Web Inspector) and see if it's still there after you go back.

I just tested that case with this patch applied and the changes were there when I went back.

[Jessie Berlin]

Comment on attachment 79038 [details]
Patch

Committed in r75966
http://trac.webkit.org/changeset/75966