Bug 143036 - [WK2] WebFrameLoaderClient::dispatchDecidePolicyForResponse() should always call the FramePolicyFunction
Summary: [WK2] WebFrameLoaderClient::dispatchDecidePolicyForResponse() should always c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-24 21:23 PDT by Chris Dumez
Modified: 2015-03-25 19:36 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.50 KB, patch)
2015-03-24 21:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2015-03-25 08:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-03-24 21:23:42 PDT
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>
Comment 1 Chris Dumez 2015-03-24 21:32:33 PDT
Created attachment 249385 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-03-24 22:26:36 PDT
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.
Comment 3 Andy Estes 2015-03-24 23:08:59 PDT
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().
Comment 4 Chris Dumez 2015-03-25 08:10:47 PDT
(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.
Comment 5 Chris Dumez 2015-03-25 08:12:29 PDT
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.
Comment 6 Chris Dumez 2015-03-25 08:38:57 PDT
Created attachment 249407 [details]
Patch
Comment 7 Chris Dumez 2015-03-25 08:41:19 PDT
The new iteration does not have a test yet. I'll try and write one today but I don't know how yet.
Comment 8 Chris Dumez 2015-03-25 14:17:12 PDT
(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.
Comment 9 David Kilzer (:ddkilzer) 2015-03-25 15:29:06 PDT
(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?
Comment 10 Chris Dumez 2015-03-25 16:42:29 PDT
(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.
Comment 11 WebKit Commit Bot 2015-03-25 19:36:51 PDT
Comment on attachment 249407 [details]
Patch

Clearing flags on attachment: 249407

Committed r181991: <http://trac.webkit.org/changeset/181991>
Comment 12 WebKit Commit Bot 2015-03-25 19:36:56 PDT
All reviewed patches have been landed.  Closing bug.