Summary: | REGRESSION: Assertion failure in FrameLoader::continueLoadAfterWillSubmitForm() when navigating back to an unreachable URL | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jessie Berlin <jberlin> | ||||||||||
Component: | Page Loading | Assignee: | 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
Jessie Berlin
2011-01-13 12:03:19 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).
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. Created attachment 78880 [details]
Patch
Attachment 78880 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7558024 Attachment 78880 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7570025 + 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. 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. (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. > 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.
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.
Attachment 79032 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7551059 Created attachment 79038 [details]
Patch
Fix Chromium Build errors.
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. (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. (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 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. (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! (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 on attachment 79038 [details] Patch Committed in r75966 http://trac.webkit.org/changeset/75966 |