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>
Created attachment 249385 [details] Patch
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.
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().
(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 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.
Created attachment 249407 [details] Patch
The new iteration does not have a test yet. I'll try and write one today but I don't know how yet.
(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.
(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?
(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 on attachment 249407 [details] Patch Clearing flags on attachment: 249407 Committed r181991: <http://trac.webkit.org/changeset/181991>
All reviewed patches have been landed. Closing bug.