Changeset 228430 in webkit


Ignore:
Timestamp:
Feb 13, 2018 12:46:47 PM (6 years ago)
Author:
Chris Dumez
Message:

REGRESSION (r228299): Broke reader mode in Safari
​https://bugs.webkit.org/show_bug.cgi?id=182697
<rdar://problem/37399012>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Rework the fix for r228299 to be more targeted. I moved the policy check
cencelation from FrameLoader::stopLoading() to NavigationScheduler::schedule()
when a pending load is cancelled by another load. I have verified that the
sites fixed by r228299 still work with this more limited change. However,
reader mode is now working again.

The issue seems to be that we tell CFNetwork to continue with the load after
receiving the response, even if the client has not responded to the
decidePolicyForNavigationResponse delegate yet. As a result, CFNetwork sends
us the resource data and we may commit the provisional load before receiving
the policy response from the client. When the provisional load is committed,
we call FrameLoader::stopLoading() which after r228299 cancelled pending
policy checks. Because we did not wait for the policy check response to
commit the load, we would cancel it which would make the load fail.

The real fix here would be to make not tell CFNetwork to continue until after
we've received the policy delegate response. However, this is a larger and
riskier change at this point. I will follow-up on this issue.

Covered by new API test.

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::stopLoading):

  • loader/NavigationScheduler.cpp:

(WebCore::NavigationScheduler::schedule):

Tools:

Add API test coverage for responding asynchronously to the decidePolicyForNavigationResponse
delegate.

  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm: Added.

(-[TestAsyncNavigationDelegate webView:didFinishNavigation:]):
(-[TestAsyncNavigationDelegate webView:didFailNavigation:withError:]):
(-[TestAsyncNavigationDelegate webView:didFailProvisionalNavigation:withError:]):
(-[TestAsyncNavigationDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[TestAsyncNavigationDelegate webView:decidePolicyForNavigationResponse:decisionHandler:]):
(TestWebKitAPI::TEST):

Location:
trunk
Files:
1 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r228429 r228430  
     12018-02-13  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION (r228299): Broke reader mode in Safari
     4        https://bugs.webkit.org/show_bug.cgi?id=182697
     5        <rdar://problem/37399012>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        Rework the fix for r228299 to be more targeted. I moved the policy check
     10        cencelation from FrameLoader::stopLoading() to NavigationScheduler::schedule()
     11        when a pending load is cancelled by another load. I have verified that the
     12        sites fixed by r228299 still work with this more limited change. However,
     13        reader mode is now working again.
     14
     15        The issue seems to be that we tell CFNetwork to continue with the load after
     16        receiving the response, even if the client has not responded to the
     17        decidePolicyForNavigationResponse delegate yet. As a result, CFNetwork sends
     18        us the resource data and we may commit the provisional load before receiving
     19        the policy response from the client. When the provisional load is committed,
     20        we call FrameLoader::stopLoading() which after r228299 cancelled pending
     21        policy checks. Because we did not wait for the policy check response to
     22        commit the load, we would cancel it which would make the load fail.
     23
     24        The real fix here would be to make not tell CFNetwork to continue until after
     25        we've received the policy delegate response. However, this is a larger and
     26        riskier change at this point. I will follow-up on this issue.
     27
     28        Covered by new API test.
     29
     30        * loader/FrameLoader.cpp:
     31        (WebCore::FrameLoader::stopLoading):
     32        * loader/NavigationScheduler.cpp:
     33        (WebCore::NavigationScheduler::schedule):
     34
    1352018-02-13  Zalan Bujtas  <zalan@apple.com>
    236

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r228299 r228430  
    489489    }
    490490
    491     policyChecker().stopCheck();
    492 
    493491    // FIXME: This will cancel redirection timer, which really needs to be restarted when restoring the frame from b/f cache.
    494492    m_frame.navigationScheduler().cancel();

  • trunk/Source/WebCore/loader/NavigationScheduler.cpp

    r226745 r228430  
    5151#include "NavigationDisabler.h"
    5252#include "Page.h"
     53#include "PolicyChecker.h"
    5354#include "ScriptController.h"
    5455#include "UserGestureIndicator.h"
    … …  
    521522        if (DocumentLoader* provisionalDocumentLoader = m_frame.loader().provisionalDocumentLoader())
    522523            provisionalDocumentLoader->stopLoading();
     524        m_frame.loader().policyChecker().stopCheck();
    523525        m_frame.loader().stopLoading(UnloadEventPolicyUnloadAndPageHide);
    524526    }

  • trunk/Tools/ChangeLog

    r228416 r228430  
     12018-02-13  Chris Dumez  <cdumez@apple.com>
     2
     3        REGRESSION (r228299): Broke reader mode in Safari
     4        https://bugs.webkit.org/show_bug.cgi?id=182697
     5        <rdar://problem/37399012>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        Add API test coverage for responding asynchronously to the decidePolicyForNavigationResponse
     10        delegate.
     11
     12        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     13        * TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm: Added.
     14        (-[TestAsyncNavigationDelegate webView:didFinishNavigation:]):
     15        (-[TestAsyncNavigationDelegate webView:didFailNavigation:withError:]):
     16        (-[TestAsyncNavigationDelegate webView:didFailProvisionalNavigation:withError:]):
     17        (-[TestAsyncNavigationDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
     18        (-[TestAsyncNavigationDelegate webView:decidePolicyForNavigationResponse:decisionHandler:]):
     19        (TestWebKitAPI::TEST):
     20
    1212018-02-12  John Wilander  <wilander@apple.com>
    222

  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r228352 r228430  
    538538                83148B07202AC6AD00BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83148B04202AC68200BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior_Bundle.cpp */; };
    539539                83148B09202AC78D00BADE99 /* override-builtins-test.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 83148B08202AC76800BADE99 /* override-builtins-test.html */; };
     540                834138C7203261CA00F26960 /* AsyncPolicyForNavigationResponse.mm in Sources */ = {isa = PBXBuildFile; fileRef = 834138C6203261B900F26960 /* AsyncPolicyForNavigationResponse.mm */; };
    540541                8349D3C21DB96DDE004A9F65 /* ContextMenuDownload.mm in Sources */ = {isa = PBXBuildFile; fileRef = 8349D3C11DB96DDA004A9F65 /* ContextMenuDownload.mm */; };
    541542                8349D3C41DB9728E004A9F65 /* link-with-download-attribute.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 8349D3C31DB9724F004A9F65 /* link-with-download-attribute.html */; };
    … …  
    15351536                83148B05202AC68200BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = InjectedBundleDisableOverrideBuiltinsBehavior.cpp; sourceTree = "<group>"; };
    15361537                83148B08202AC76800BADE99 /* override-builtins-test.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "override-builtins-test.html"; sourceTree = "<group>"; };
     1538                834138C6203261B900F26960 /* AsyncPolicyForNavigationResponse.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = AsyncPolicyForNavigationResponse.mm; sourceTree = "<group>"; };
    15371539                8349D3C11DB96DDA004A9F65 /* ContextMenuDownload.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ContextMenuDownload.mm; sourceTree = "<group>"; };
    15381540                8349D3C31DB9724F004A9F65 /* link-with-download-attribute.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "link-with-download-attribute.html"; sourceTree = "<group>"; };
    … …  
    20722074                                2DE71AFD1D49C0BD00904094 /* AnimatedResize.mm */,
    20732075                                63F668201F97C3AA0032EE51 /* ApplicationManifest.mm */,
     2076                                834138C6203261B900F26960 /* AsyncPolicyForNavigationResponse.mm */,
    20742077                                754CEC801F6722DC00D0039A /* AutoFillAvailable.mm */,
    20752078                                2DD355351BD08378005DF4A7 /* AutoLayoutIntegration.mm */,
    … …  
    33653368                                63F668221F97F7F90032EE51 /* ApplicationManifest.mm in Sources */,
    33663369                                6354F4D11F7C3AB500D89DF3 /* ApplicationManifestParser.cpp in Sources */,
     3370                                834138C7203261CA00F26960 /* AsyncPolicyForNavigationResponse.mm in Sources */,
    33673371                                7CCE7EB41A411A7E00447C4C /* AttributedString.mm in Sources */,
    33683372                                CDC8E48D1BC5CB4500594FEC /* AudioSessionCategoryIOS.mm in Sources */,
Note: See TracChangeset for help on using the changeset viewer.