Bug 83894

Summary: Ensure that there's always a provisional document loader if the frame loader is in provisional state
Product: WebKit Reporter: jochen
Component: New BugsAssignee: jochen
Status: RESOLVED INVALID    
Severity: Normal CC: abarth, ap, beidson, dglazkov, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch none

jochen
Reported 2012-04-13 08:00:40 PDT
Ensure that there's always a provisional document loader if the frame loader is in provisional state
Attachments
Patch (4.27 KB, patch)
2012-04-13 08:03 PDT, jochen
no flags
Archive of layout-test-results from ec2-cr-linux-01 (6.37 MB, application/zip)
2012-04-13 08:39 PDT, WebKit Review Bot
no flags
Patch (4.44 KB, patch)
2012-04-27 03:09 PDT, jochen
no flags
jochen
Comment 1 2012-04-13 08:03:29 PDT
jochen
Comment 2 2012-04-13 08:06:18 PDT
We're still seeing crashes in the FrameLoader where the FrameLoader's state is "provisional" but there is no provisional document loader. I added code to update the FrameLoader's state everytime the provisional document loader is cleared, and added checks that the FrameLoader's state can't be set to provisional without a provisional loader. If the crashes go away, or the newly added checks reveal the culprit, we should relex the checks to use ASSERT() instead of CRASH().
WebKit Review Bot
Comment 3 2012-04-13 08:39:09 PDT
Comment on attachment 137085 [details] Patch Attachment 137085 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12399343 New failing tests: http/tests/navigation/changing-frame-hierarchy-in-onload.html
WebKit Review Bot
Comment 4 2012-04-13 08:39:15 PDT
Created attachment 137090 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Adam Barth
Comment 5 2012-04-13 09:39:03 PDT
Comment on attachment 137085 [details] Patch This looks like a good idea, but you seem to have one failing test to look at.
jochen
Comment 6 2012-04-13 11:24:19 PDT
Comment on attachment 137085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137085&action=review > Source/WebCore/loader/FrameLoader.cpp:1531 > + setState(FrameStateComplete); This seems to be the problem. While the provisional document loader is cleared, the state remains to be provisional. Any insight what is happening here? If I replace the setState with if (m_state == FrameStateProvisional) CRASH(); the callstack is: WebCore::FrameLoader::stopAllLoaders WebCore::FrameLoader::stopAllLoaders WebCore::FrameLoader::stopForUserCancel WebCore::DOMWindow::stop ...
Nate Chapin
Comment 7 2012-04-19 16:45:01 PDT
Comment on attachment 137085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137085&action=review >> Source/WebCore/loader/FrameLoader.cpp:1531 >> + setState(FrameStateComplete); > > This seems to be the problem. While the provisional document loader is cleared, the state remains to be provisional. > > Any insight what is happening here? > > If I replace the setState with if (m_state == FrameStateProvisional) CRASH(); the callstack is: > > WebCore::FrameLoader::stopAllLoaders > WebCore::FrameLoader::stopAllLoaders > WebCore::FrameLoader::stopForUserCancel > WebCore::DOMWindow::stop > ... Is there anything interesting under DOMWindow::stop()? It's possible we're getting some sort of unintended re-entrancy. I'm suspicious that this is caused by an unload handling is calling window.stop(). Both the unload event and setState(FrameStateCommittedPage) happen from FrameLoader::transitionToCommitted(), so it's possible we're getting our DocumentLoaders confused and cancelling the provisional load?
Nate Chapin
Comment 8 2012-04-25 11:59:27 PDT
(In reply to comment #6) > (From update of attachment 137085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137085&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1531 > > + setState(FrameStateComplete); > > This seems to be the problem. While the provisional document loader is cleared, the state remains to be provisional. > > Any insight what is happening here? > > If I replace the setState with if (m_state == FrameStateProvisional) CRASH(); the callstack is: > > WebCore::FrameLoader::stopAllLoaders > WebCore::FrameLoader::stopAllLoaders > WebCore::FrameLoader::stopForUserCancel > WebCore::DOMWindow::stop > ... Having looked at this more, I think this failure is a red herring. The timeout and the crash are actually for different reasons. The crash is because a detached FrameLoader is still in the provisional state (which is inconsistent but benign). The timeout is because we're overwriting FrameStateCommittedPage, not FrameStateProvisional. If you change that line to "if (m_state == FrameStateProvisional) setState(FrameStateComplete);", the test passes. Given the stack reported in http://code.google.com/p/chromium/issues/detail?id=121735, I suspect the inconsistency that is actually causing crashes is fixed by adding the setState() call in continueFragmentScrollAfterNavigationPolicy().
jochen
Comment 9 2012-04-27 03:09:49 PDT
jochen
Comment 10 2012-04-27 03:11:36 PDT
(In reply to comment #8) > Having looked at this more, I think this failure is a red herring. The timeout and the crash are actually for different reasons. The crash is because a detached FrameLoader is still in the provisional state (which is inconsistent but benign). The timeout is because we're overwriting FrameStateCommittedPage, not FrameStateProvisional. If you change that line to "if (m_state == FrameStateProvisional) setState(FrameStateComplete);", the test passes. I updated the patch accordingly. > > Given the stack reported in http://code.google.com/p/chromium/issues/detail?id=121735, I suspect the inconsistency that is actually causing crashes is fixed by adding the setState() call in continueFragmentScrollAfterNavigationPolicy(). I manage to make a test that goes through this code path, but I didn't get it there in provisional state. So I updated the patch to also CRASH() there, hopefully getting some stack traces when this happens again
WebKit Review Bot
Comment 11 2012-04-27 23:27:52 PDT
Comment on attachment 139159 [details] Patch Clearing flags on attachment: 139159 Committed r115549: <http://trac.webkit.org/changeset/115549>
WebKit Review Bot
Comment 12 2012-04-27 23:28:06 PDT
All reviewed patches have been landed. Closing bug.
jochen
Comment 13 2012-05-03 15:12:57 PDT
The CRASH() statements are triggered on canary channel. Here are some sample backtraces: 0x66fc65d8 [chrome.dll] - frameloader.cpp:2752] WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool) 0x66fc5d36 [chrome.dll] - frameloader.cpp:2634] WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void *,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool) 0x66fc5c21 [chrome.dll] - policycallback.cpp:103] WebCore::PolicyCallback::call(bool) 0x66fc531d [chrome.dll] - policychecker.cpp:167] WebCore::PolicyChecker::continueAfterNavigationPolicy(WebCore::PolicyAction) 0x66fc1e78 [chrome.dll] - frameloaderclientimpl.cpp:1026] WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction(void ( WebCore::PolicyChecker::*)(WebCore::PolicyAction),WebCore::NavigationAction const &,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>) 0x66fc155c [chrome.dll] - policychecker.cpp:89] WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const &,WebCore::DocumentLoader *,WTF::PassRefPtr<WebCore::FormState>,void (*)(void *,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool),void *) 0x66fbf773 [chrome.dll] - frameloader.cpp:1371] WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader *,WebCore::FrameLoadType,WTF::PassRefPtr<WebCore::FormState>) 0x67712093 [chrome.dll] - frameloader.cpp:1479] WebCore::FrameLoader::reload(bool) 0x01da65d8 [chrome.dll] - frameloader.cpp:2752] WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool) 0x01da5d36 [chrome.dll] - frameloader.cpp:2634] WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void *,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool) 0x01da5c21 [chrome.dll] - policycallback.cpp:103] WebCore::PolicyCallback::call(bool) 0x01da531d [chrome.dll] - policychecker.cpp:167] WebCore::PolicyChecker::continueAfterNavigationPolicy(WebCore::PolicyAction) 0x01da1e78 [chrome.dll] - frameloaderclientimpl.cpp:1026] WebKit::FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction(void ( WebCore::PolicyChecker::*)(WebCore::PolicyAction),WebCore::NavigationAction const &,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>) 0x01da155c [chrome.dll] - policychecker.cpp:89] WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const &,WebCore::DocumentLoader *,WTF::PassRefPtr<WebCore::FormState>,void (*)(void *,WebCore::ResourceRequest const &,WTF::PassRefPtr<WebCore::FormState>,bool),void *) 0x01d9f773 [chrome.dll] - frameloader.cpp:1371] WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader *,WebCore::FrameLoadType,WTF::PassRefPtr<WebCore::FormState>) 0x01d9e794 [chrome.dll] - frameloader.cpp:1312] WebCore::FrameLoader::load(WebCore::DocumentLoader *) 0x01d9b146 [chrome.dll] - frameloader.cpp:1248] WebCore::FrameLoader::load(WebCore::ResourceRequest const &,WebCore::SubstituteData const &,bool) 0x01d9ac92 [chrome.dll] - frameloader.cpp:1235] WebCore::FrameLoader::load(WebCore::ResourceRequest const &,bool) I have no idea yet what's going wrong here. While the ASSERT() at the top of continueLoadAfterNavigationPolicy implies that the method can be entered without a policy document loader, I don't think that's the case in these stack traces. Also, in case that assertion is still valid (the comment uses names that imply it is not), it is strange that stopAllLoaders() is invoked later on which will unconditionally clear the provisional document loader.
jochen
Comment 14 2012-05-04 07:24:21 PDT
I guess at this point, I have enough crashes that we can just roll this out again
jochen
Comment 15 2012-05-04 07:32:35 PDT
Reverted r115549 for reason: The newly added CRASH() statements are triggered too often Committed r116098: <http://trac.webkit.org/changeset/116098>
jochen
Comment 16 2012-05-18 12:14:25 PDT
The original crash doesn't occur anymore after I updated chroium's FrameLoaderClient to check for a document loader before accessing it in http://trac.webkit.org/changeset/116556 I looked at the FrameLoader's code together with Nate and it seems that there are code paths where it is acceptable that the loader is at least temporary in a provisional state but doesn't have a provisional loader, e.g. when stopAllLoaders is invoked the loader is first set to 0 and only later the state is advanced to completed. Anyway, for these reasons, I'm closing this bug
Note You need to log in before you can comment on or make changes to this bug.