Changeset 241352 in webkit

Timestamp:

Feb 13, 2019 1:01:49 AM (5 years ago)

Author:

rniwa@webkit.org

Message:

Release assert in PolicyCheckIdentifier::isValidFor via WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction
​https://bugs.webkit.org/show_bug.cgi?id=194582

Reviewed by Antti Koivisto.

Source/WebCore:

Check the zero-ness of m_policyCheck first so that we can differentiate process ID being wrong
from the non-generated identifier being sent to us as it was the case in this failure.

• loader/PolicyChecker.cpp:

(WebCore::PolicyCheckIdentifier::isValidFor):

Source/WebKit:

The bug was caused by WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction invoking the callback
with responseIdentifier even when we had failed to send the policy check IPC. Clearly, responseIdentifier
is invalid in that case, and we should be using requestIdentifier instead.

Unfortunately no new tests since I'm not aware of a way to make sendSync fail in this case.

• WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:

(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/loader/PolicyChecker.cpp (modified) (1 diff)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-02-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert in PolicyCheckIdentifier::isValidFor via WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction
+        https://bugs.webkit.org/show_bug.cgi?id=194582
+
+        Reviewed by Antti Koivisto.
+
+        Check the zero-ness of m_policyCheck first so that we can differentiate process ID being wrong
+        from the non-generated identifier being sent to us as it was the case in this failure.
+
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyCheckIdentifier::isValidFor):
+
 2019-02-13  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/WebCore/loader/PolicyChecker.cpp

@@ -81 +81 @@
 bool PolicyCheckIdentifier::isValidFor(PolicyCheckIdentifier expectedIdentifier)
 {
+    RELEASE_ASSERT_WITH_MESSAGE(m_policyCheck, "Received 0 as the policy check identifier");
     RELEASE_ASSERT_WITH_MESSAGE(m_process == expectedIdentifier.m_process, "Received a policy check response for a wrong process");
-    RELEASE_ASSERT_WITH_MESSAGE(m_policyCheck, "Received 0 as the policy check identifier");
     RELEASE_ASSERT_WITH_MESSAGE(m_policyCheck <= expectedIdentifier.m_policyCheck, "Received a policy check response from the future");
     return m_policyCheck == expectedIdentifier.m_policyCheck;

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-02-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert in PolicyCheckIdentifier::isValidFor via WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction
+        https://bugs.webkit.org/show_bug.cgi?id=194582
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction invoking the callback
+        with responseIdentifier even when we had failed to send the policy check IPC. Clearly, responseIdentifier
+        is invalid in that case, and we should be using requestIdentifier instead.
+
+        Unfortunately no new tests since I'm not aware of a way to make sendSync fail in this case.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+
 2019-02-13  Benjamin Poulain  <benjamin@webkit.org>
 

trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

@@ -912 +912 @@
             IPC::FormDataReference { request.httpBody() }, redirectResponse, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())),
             Messages::WebPageProxy::DecidePolicyForNavigationActionSync::Reply(responseIdentifier, policyAction, newNavigationID, downloadID, websitePolicies))) {
-            m_frame->didReceivePolicyDecision(listenerID, responseIdentifier, PolicyAction::Ignore, 0, { }, { });
+            m_frame->didReceivePolicyDecision(listenerID, requestIdentifier, PolicyAction::Ignore, 0, { }, { });
             return;
         }