REGRESSION (r229828): Facebook login popup is blank
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 31 Mar 2018 06:44:29 +0000 (06:44 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 31 Mar 2018 06:44:29 +0000 (06:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184206
<rdar://problem/39057006>

Reviewed by Wenson Hsieh.

Source/WebCore:

Since r229828, we freeze the layer tree during the navigation policy check.
We freeze in WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction()
and unfreeze in WebFrameLoaderClient::didDecidePolicyForNavigationAction().

WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() gets called
from PolicyChecker::checkNavigationPolicy() which has 3 call sites in
FrameLoader and one in DocumentLoader for redirects. The call sites in
FrameLoader were taking care of calling didDecidePolicyForNavigationAction()
on the FrameLoaderClient in their completion handler, but the DocumentLoader
call site was failing to do so. As a result, the layer tree would stay frozen.

To make this a lot less error prone, I moved the call to
WebFrameLoaderClient::didDecidePolicyForNavigationAction() to
PolicyChecker::checkNavigationPolicy(), inside the completion handler passed
to WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(). This way,
even if new code starts calling PolicyChecker::checkNavigationPolicy(), we
do not need to worry about letting the client know when the policy decision
is made.

No new tests, covered by existing redirection tests with the
new assertion I added.

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::continueFragmentScrollAfterNavigationPolicy):
(WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
* loader/PolicyChecker.cpp:
(WebCore::PolicyChecker::checkNavigationPolicy):

Source/WebKit:

Add assertion to make sure we never try to do a policy check to
a resource response while a policy check for a navigation is
pending. This assertion was being hit by several of our redirection
tests without my fix.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230128 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/PolicyChecker.cpp
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

index fdd694f75172893b77e9fc0c14aa28d1861df602..bcb94a806d2d4a3651ffe718b8aa60a44f2e2131 100644 (file)
@@ -1,3 +1,39 @@
+2018-03-30  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229828): Facebook login popup is blank
+        https://bugs.webkit.org/show_bug.cgi?id=184206
+        <rdar://problem/39057006>
+
+        Reviewed by Wenson Hsieh.
+
+        Since r229828, we freeze the layer tree during the navigation policy check.
+        We freeze in WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction()
+        and unfreeze in WebFrameLoaderClient::didDecidePolicyForNavigationAction().
+
+        WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() gets called
+        from PolicyChecker::checkNavigationPolicy() which has 3 call sites in
+        FrameLoader and one in DocumentLoader for redirects. The call sites in
+        FrameLoader were taking care of calling didDecidePolicyForNavigationAction()
+        on the FrameLoaderClient in their completion handler, but the DocumentLoader
+        call site was failing to do so. As a result, the layer tree would stay frozen.
+
+        To make this a lot less error prone, I moved the call to
+        WebFrameLoaderClient::didDecidePolicyForNavigationAction() to
+        PolicyChecker::checkNavigationPolicy(), inside the completion handler passed
+        to WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(). This way,
+        even if new code starts calling PolicyChecker::checkNavigationPolicy(), we
+        do not need to worry about letting the client know when the policy decision
+        is made.
+
+        No new tests, covered by existing redirection tests with the
+        new assertion I added.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::continueFragmentScrollAfterNavigationPolicy):
+        (WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+
 2018-03-30  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: tint all pixels drawn by shader program when hovering ShaderProgramTreeElement
index 8aa5c98f4e241b8b3e4a17257b626a9d2167d51e..b5f833697ce9ba6deb60bf675a8d22be22c81d79 100644 (file)
@@ -2930,8 +2930,6 @@ void FrameLoader::receivedMainResourceError(const ResourceError& error)
 
 void FrameLoader::continueFragmentScrollAfterNavigationPolicy(const ResourceRequest& request, bool shouldContinue)
 {
-    m_client.didDecidePolicyForNavigationAction();
-
     m_quickRedirectComing = false;
 
     if (!shouldContinue)
@@ -3177,8 +3175,6 @@ void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& reque
     // through this method already, nested; otherwise, policyDataSource should still be set.
     ASSERT(m_policyDocumentLoader || !m_provisionalDocumentLoader->unreachableURL().isEmpty());
 
-    m_client.didDecidePolicyForNavigationAction();
-
     bool isTargetItem = history().provisionalItem() ? history().provisionalItem()->isTargetItem() : false;
 
     bool urlIsDisallowed = allowNavigationToInvalidURL == AllowNavigationToInvalidURL::No && !request.url().isValid();
index 2650df6a472d49dd0884316a7511929d49e69298..42d52c1e919f8f2e7d6f91b7e6aeb9af4713566b 100644 (file)
@@ -150,6 +150,8 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
     String suggestedFilename = action.downloadAttribute().isEmpty() ? nullAtom() : action.downloadAttribute();
     ResourceRequest requestCopy = request;
     m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, didReceiveRedirectResponse, formState, [this, function = WTFMove(function), request = WTFMove(requestCopy), formState = makeRefPtr(formState), suggestedFilename = WTFMove(suggestedFilename)](PolicyAction policyAction) mutable {
+        m_frame.loader().client().didDecidePolicyForNavigationAction();
+
         m_delegateIsDecidingNavigationPolicy = false;
 
         switch (policyAction) {
index 64436a73d06fd3cb7e13e739f8cff53f4d54b3c0..65d2ef7965716eaa74ecb65984e32434720faa7c 100644 (file)
@@ -1,3 +1,19 @@
+2018-03-30  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229828): Facebook login popup is blank
+        https://bugs.webkit.org/show_bug.cgi?id=184206
+        <rdar://problem/39057006>
+
+        Reviewed by Wenson Hsieh.
+
+        Add assertion to make sure we never try to do a policy check to
+        a resource response while a policy check for a navigation is
+        pending. This assertion was being hit by several of our redirection
+        tests without my fix.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse):
+
 2018-03-30  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r230125.
index fab290a7739e6508af5aa0c0a2579918e5fb67f0..cc16e01ddcb7eef79c8c986701c7a4f0c6a4aafc 100644 (file)
@@ -729,6 +729,8 @@ void WebFrameLoaderClient::dispatchDecidePolicyForResponse(const ResourceRespons
         return;
     }
 
+    ASSERT(!m_isDecidingNavigationPolicyDecision);
+
     RefPtr<API::Object> userData;
 
     // Notify the bundle client.