Bug 83894 - Ensure that there's always a provisional document loader if the frame loader is in provisional state
Summary: Ensure that there's always a provisional document loader if the frame loader ...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-13 08:00 PDT by jochen
Modified: 2012-05-18 12:14 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.27 KB, patch)
2012-04-13 08:03 PDT, jochen
no flags Details | Formatted Diff | Diff
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 Details
Patch (4.44 KB, patch)
2012-04-27 03:09 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-04-13 08:00:40 PDT
Ensure that there's always a provisional document loader if the frame loader is in provisional state
Comment 1 jochen 2012-04-13 08:03:29 PDT
Created attachment 137085 [details]
Patch
Comment 2 jochen 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().
Comment 3 WebKit Review Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 Adam Barth 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.
Comment 6 jochen 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
...
Comment 7 Nate Chapin 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?
Comment 8 Nate Chapin 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().
Comment 9 jochen 2012-04-27 03:09:49 PDT
Created attachment 139159 [details]
Patch
Comment 10 jochen 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
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2012-04-27 23:28:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 jochen 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.
Comment 14 jochen 2012-05-04 07:24:21 PDT
I guess at this point, I have enough crashes that we can just roll this out again
Comment 15 jochen 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>
Comment 16 jochen 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