Changeset 230128 in webkit


Ignore:
Timestamp:
Mar 30, 2018 11:44:29 PM (6 years ago)
Author:
Chris Dumez
Message:

REGRESSION (r229828): Facebook login popup is blank
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):

Location:
trunk/Source
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r230127 r230128  
     12018-03-30  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION (r229828): Facebook login popup is blank
     4        https://bugs.webkit.org/show_bug.cgi?id=184206
     5        <rdar://problem/39057006>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        Since r229828, we freeze the layer tree during the navigation policy check.
     10        We freeze in WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction()
     11        and unfreeze in WebFrameLoaderClient::didDecidePolicyForNavigationAction().
     12
     13        WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() gets called
     14        from PolicyChecker::checkNavigationPolicy() which has 3 call sites in
     15        FrameLoader and one in DocumentLoader for redirects. The call sites in
     16        FrameLoader were taking care of calling didDecidePolicyForNavigationAction()
     17        on the FrameLoaderClient in their completion handler, but the DocumentLoader
     18        call site was failing to do so. As a result, the layer tree would stay frozen.
     19
     20        To make this a lot less error prone, I moved the call to
     21        WebFrameLoaderClient::didDecidePolicyForNavigationAction() to
     22        PolicyChecker::checkNavigationPolicy(), inside the completion handler passed
     23        to WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(). This way,
     24        even if new code starts calling PolicyChecker::checkNavigationPolicy(), we
     25        do not need to worry about letting the client know when the policy decision
     26        is made.
     27
     28        No new tests, covered by existing redirection tests with the
     29        new assertion I added.
     30
     31        * loader/FrameLoader.cpp:
     32        (WebCore::FrameLoader::continueFragmentScrollAfterNavigationPolicy):
     33        (WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
     34        * loader/PolicyChecker.cpp:
     35        (WebCore::PolicyChecker::checkNavigationPolicy):
     36
    1372018-03-30  Devin Rousso  <webkit@devinrousso.com>
    238
  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r230051 r230128  
    29312931void FrameLoader::continueFragmentScrollAfterNavigationPolicy(const ResourceRequest& request, bool shouldContinue)
    29322932{
    2933     m_client.didDecidePolicyForNavigationAction();
    2934 
    29352933    m_quickRedirectComing = false;
    29362934
     
    31773175    // through this method already, nested; otherwise, policyDataSource should still be set.
    31783176    ASSERT(m_policyDocumentLoader || !m_provisionalDocumentLoader->unreachableURL().isEmpty());
    3179 
    3180     m_client.didDecidePolicyForNavigationAction();
    31813177
    31823178    bool isTargetItem = history().provisionalItem() ? history().provisionalItem()->isTargetItem() : false;
  • trunk/Source/WebCore/loader/PolicyChecker.cpp

    r229778 r230128  
    151151    ResourceRequest requestCopy = request;
    152152    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 {
     153        m_frame.loader().client().didDecidePolicyForNavigationAction();
     154
    153155        m_delegateIsDecidingNavigationPolicy = false;
    154156
  • trunk/Source/WebKit/ChangeLog

    r230126 r230128  
     12018-03-30  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION (r229828): Facebook login popup is blank
     4        https://bugs.webkit.org/show_bug.cgi?id=184206
     5        <rdar://problem/39057006>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        Add assertion to make sure we never try to do a policy check to
     10        a resource response while a policy check for a navigation is
     11        pending. This assertion was being hit by several of our redirection
     12        tests without my fix.
     13
     14        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
     15        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse):
     16
    1172018-03-30  Ryan Haddad  <ryanhaddad@apple.com>
    218
  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

    r230051 r230128  
    730730    }
    731731
     732    ASSERT(!m_isDecidingNavigationPolicyDecision);
     733
    732734    RefPtr<API::Object> userData;
    733735
Note: See TracChangeset for help on using the changeset viewer.