Bug 83894 – Ensure that there's always a provisional document loader if the frame loader is in provisional state

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

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().

[WebKit Review Bot]

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]

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 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
...

[Nate Chapin]

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]

(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

[WebKit Review Bot]

Comment on attachment 139159 [details]
Patch

Clearing flags on attachment: 139159

Committed r115549: <http://trac.webkit.org/changeset/115549>

[WebKit Review Bot]

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