Ensure that there's always a provisional document loader if the frame loader is in provisional state
Created attachment 137085 [details] Patch
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().
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
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
Comment on attachment 137085 [details] Patch This looks like a good idea, but you seem to have one failing test to look at.
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 ...
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?
(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().
Created attachment 139159 [details] Patch
(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
Comment on attachment 139159 [details] Patch Clearing flags on attachment: 139159 Committed r115549: <http://trac.webkit.org/changeset/115549>
All reviewed patches have been landed. Closing bug.
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.
I guess at this point, I have enough crashes that we can just roll this out again
Reverted r115549 for reason: The newly added CRASH() statements are triggered too often Committed r116098: <http://trac.webkit.org/changeset/116098>
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