Bug 143036 – [WK2] WebFrameLoaderClient::dispatchDecidePolicyForResponse() should always call the FramePolicyFunction

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

WebFrameLoaderClient::dispatchDecidePolicyForResponse() should always call the FramePolicyFunction. Currently, it fails to do so under 2 conditions:
- m_frame->page() returns null
or
- webPage->sendSync() returns false

If the FramePolicyFunction is not called, we will fail to clear the callback in the PolicyChecker and DocumentLoader::continueAfterContentPolicy() will not be called.
DocumentLoader::continueAfterContentPolicy() is in charge of resetting m_waitingForContentPolicy flag to false.

This could explain the following assertion being hit in DocumentLoader::detachFromFrame():
RELEASE_ASSERT_WITH_MESSAGE(!m_waitingForContentPolicy, "The content policy callback needs a valid frame.");

Radar: <rdar://problem/20252438>


Also, as the PolicyChecker callback is not cleared, it could make it possible for DocumentLoader::continueAfterContentPolicy() to be called *after* the load is finished, when later canceling the PolicyCallback:
FrameLoader::stopAllLoaders() -> PolicyChecker::stopCheck() -> PolicyCallback::cancel() -> DocumentLoader::continueAfterContentPolicy(PolicyIgnore)

Calling continueAfterContentPolicy(PolicyIgnore) after the load is finished would be bad and could explain some of the crashes we've seen in DocumentLoader::continueAfterContentPolicy() / DocumentLoader:: stopLoadingForPolicyChange().

Radar: <rdar://problem/13811738>

[Chris Dumez]

Created attachment 249385 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 249385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249385&action=review

> Source/WebKit2/ChangeLog:13
> +        - m_frame->page() returns null

Can this be tested? It seems like simply removing an iframe while waiting for a policy answer should trigger this.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:692
> +        function(PolicyUse);

I think that this should be PolicyIgnore. It could be a security check, and taking no answer for "yes" is wrong.

[Andy Estes]

Is dispatchDecidePolicyForResponse() the only place where we need to do this? We follow the same pattern in dispatchDecidePolicyForNewWindowAction() and dispatchDecidePolicyForNavigationAction(), and one of the crash logs attached to rdar://problem/13811738 shows a crash in continueAfterNavigationPolicy().

[Chris Dumez]

(In reply to comment #3)
> Is dispatchDecidePolicyForResponse() the only place where we need to do
> this? We follow the same pattern in dispatchDecidePolicyForNewWindowAction()
> and dispatchDecidePolicyForNavigationAction(), and one of the crash logs
> attached to rdar://problem/13811738 shows a crash in
> continueAfterNavigationPolicy().

Yes, I believe you are right. I simply did not have time to investigate the other cases yet. I'll take a look today.

[Chris Dumez]

Comment on attachment 249385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249385&action=review

>> Source/WebKit2/ChangeLog:13
>> +        - m_frame->page() returns null
> 
> Can this be tested? It seems like simply removing an iframe while waiting for a policy answer should trigger this.

It did not look easy to test this but I'll try.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:692
>> +        function(PolicyUse);
> 
> I think that this should be PolicyIgnore. It could be a security check, and taking no answer for "yes" is wrong.

Ok, I wasn't sure if Use or Ignore was the right thing to do in such case.

[Chris Dumez]

Created attachment 249407 [details]
Patch

[Chris Dumez]

The new iteration does not have a test yet. I'll try and write one today but I don't know how yet.

[Chris Dumez]

(In reply to comment #7)
> The new iteration does not have a test yet. I'll try and write one today but
> I don't know how yet.

I am not having much success writing a layout test for this. I tried doing a slow load in an iframe and then removing the frame from the document when the load is starting and before receiving the response. However, this did not reproduce it.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (In reply to comment #7)
> > The new iteration does not have a test yet. I'll try and write one today but
> > I don't know how yet.
> 
> I am not having much success writing a layout test for this. I tried doing a
> slow load in an iframe and then removing the frame from the document when
> the load is starting and before receiving the response. However, this did
> not reproduce it.

Can a TestWebKitAPI test be written that tries to cancel the load at the "wrong" time?

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > The new iteration does not have a test yet. I'll try and write one today but
> > > I don't know how yet.
> > 
> > I am not having much success writing a layout test for this. I tried doing a
> > slow load in an iframe and then removing the frame from the document when
> > the load is starting and before receiving the response. However, this did
> > not reproduce it.
> 
> Can a TestWebKitAPI test be written that tries to cancel the load at the
> "wrong" time?

The thing is that I don't know how we can end up with m_frame.page() returning null in dispatchDecidePolicyForResponse(). All I know is that if it were to happen, it would explain the crashes we see.

I put an ASSERT_NOT_REACHED() in there and tried to reproduce in many different ways. However, I could not get the assertion to be hit, no matter what I tried.

[WebKit Commit Bot]

Comment on attachment 249407 [details]
Patch

Clearing flags on attachment: 249407

Committed r181991: <http://trac.webkit.org/changeset/181991>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.