Bug 52388

Summary: REGRESSION: Assertion failure in FrameLoader::continueLoadAfterWillSubmitForm() when navigating back to an unreachable URL
Product: WebKit Reporter: Jessie Berlin <jberlin>
Component: Page LoadingAssignee: Jessie Berlin <jberlin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, ap, beidson, dglazkov, jberlin, mjs, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 52614    
Bug Blocks:    
Attachments:
Description Flags
Patch (idea for a fix, needs looking over and needs tests added once the approach is approved)
none
Patch
none
Patch
none
Patch andersca: review+

Description Jessie Berlin 2011-01-13 12:03:19 PST
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.
Comment 1 Jessie Berlin 2011-01-13 12:12:31 PST
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).
Comment 2 Jessie Berlin 2011-01-13 14:23:16 PST
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.
Comment 3 Jessie Berlin 2011-01-13 17:46:22 PST
Created attachment 78880 [details]
Patch
Comment 4 WebKit Review Bot 2011-01-13 17:50:33 PST
Attachment 78880 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7558024
Comment 5 WebKit Review Bot 2011-01-13 18:43:15 PST
Attachment 78880 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7570025
Comment 6 Alexey Proskuryakov 2011-01-13 21:27:32 PST
+        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.
Comment 7 Alexey Proskuryakov 2011-01-13 21:33:10 PST
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.
Comment 8 Jessie Berlin 2011-01-14 09:34:13 PST
(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.
Comment 9 Alexey Proskuryakov 2011-01-14 09:48:15 PST
> 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.
Comment 10 Jessie Berlin 2011-01-14 17:01:01 PST
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.
Comment 11 WebKit Review Bot 2011-01-14 17:06:04 PST
Attachment 79032 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7551059
Comment 12 Jessie Berlin 2011-01-14 17:25:42 PST
Created attachment 79038 [details]
Patch

Fix Chromium Build errors.
Comment 13 Jessie Berlin 2011-01-14 18:08:38 PST
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.
Comment 14 Maciej Stachowiak 2011-01-17 10:29:09 PST
(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.
Comment 15 Anders Carlsson 2011-01-17 11:40:22 PST
(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.
Comment 16 Anders Carlsson 2011-01-17 11:41:09 PST
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.
Comment 17 Jessie Berlin 2011-01-17 11:50:33 PST
(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!
Comment 18 Jessie Berlin 2011-01-17 12:12:29 PST
(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.
Comment 19 Jessie Berlin 2011-01-17 12:24:06 PST
Comment on attachment 79038 [details]
Patch

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