- Revision
- 228430
- Author
- [email protected]
- Date
- 2018-02-13 12:46:47 -0800 (Tue, 13 Feb 2018)
Log 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):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (228429 => 228430)
--- trunk/Source/WebCore/ChangeLog 2018-02-13 20:36:41 UTC (rev 228429)
+++ trunk/Source/WebCore/ChangeLog 2018-02-13 20:46:47 UTC (rev 228430)
@@ -1,3 +1,37 @@
+2018-02-13 Chris Dumez <[email protected]>
+
+ REGRESSION (r228299): Broke reader mode in Safari
+ https://bugs.webkit.org/show_bug.cgi?id=182697
+ <rdar://problem/37399012>
+
+ Reviewed by Ryosuke Niwa.
+
+ 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):
+
2018-02-13 Zalan Bujtas <[email protected]>
[RenderTreeBuilder] Move RenderSVG*::takeChild() to RenderTreeBuilder
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (228429 => 228430)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2018-02-13 20:36:41 UTC (rev 228429)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2018-02-13 20:46:47 UTC (rev 228430)
@@ -488,8 +488,6 @@
DatabaseManager::singleton().stopDatabases(*document, nullptr);
}
- policyChecker().stopCheck();
-
// FIXME: This will cancel redirection timer, which really needs to be restarted when restoring the frame from b/f cache.
m_frame.navigationScheduler().cancel();
}
Modified: trunk/Source/WebCore/loader/NavigationScheduler.cpp (228429 => 228430)
--- trunk/Source/WebCore/loader/NavigationScheduler.cpp 2018-02-13 20:36:41 UTC (rev 228429)
+++ trunk/Source/WebCore/loader/NavigationScheduler.cpp 2018-02-13 20:46:47 UTC (rev 228430)
@@ -50,6 +50,7 @@
#include "Logging.h"
#include "NavigationDisabler.h"
#include "Page.h"
+#include "PolicyChecker.h"
#include "ScriptController.h"
#include "UserGestureIndicator.h"
#include <wtf/CurrentTime.h>
@@ -520,6 +521,7 @@
if (redirect->wasDuringLoad()) {
if (DocumentLoader* provisionalDocumentLoader = m_frame.loader().provisionalDocumentLoader())
provisionalDocumentLoader->stopLoading();
+ m_frame.loader().policyChecker().stopCheck();
m_frame.loader().stopLoading(UnloadEventPolicyUnloadAndPageHide);
}
Modified: trunk/Tools/ChangeLog (228429 => 228430)
--- trunk/Tools/ChangeLog 2018-02-13 20:36:41 UTC (rev 228429)
+++ trunk/Tools/ChangeLog 2018-02-13 20:46:47 UTC (rev 228430)
@@ -1,3 +1,23 @@
+2018-02-13 Chris Dumez <[email protected]>
+
+ REGRESSION (r228299): Broke reader mode in Safari
+ https://bugs.webkit.org/show_bug.cgi?id=182697
+ <rdar://problem/37399012>
+
+ Reviewed by Ryosuke Niwa.
+
+ 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):
+
2018-02-12 John Wilander <[email protected]>
Resource Load Statistics: Classify resources as prevalent based on redirects to other prevalent resources
Modified: trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (228429 => 228430)
--- trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2018-02-13 20:36:41 UTC (rev 228429)
+++ trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2018-02-13 20:46:47 UTC (rev 228430)
@@ -537,6 +537,7 @@
83148B06202AC6A400BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83148B05202AC68200BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior.cpp */; };
83148B07202AC6AD00BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83148B04202AC68200BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior_Bundle.cpp */; };
83148B09202AC78D00BADE99 /* override-builtins-test.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 83148B08202AC76800BADE99 /* override-builtins-test.html */; };
+ 834138C7203261CA00F26960 /* AsyncPolicyForNavigationResponse.mm in Sources */ = {isa = PBXBuildFile; fileRef = 834138C6203261B900F26960 /* AsyncPolicyForNavigationResponse.mm */; };
8349D3C21DB96DDE004A9F65 /* ContextMenuDownload.mm in Sources */ = {isa = PBXBuildFile; fileRef = 8349D3C11DB96DDA004A9F65 /* ContextMenuDownload.mm */; };
8349D3C41DB9728E004A9F65 /* link-with-download-attribute.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 8349D3C31DB9724F004A9F65 /* link-with-download-attribute.html */; };
835CF9671D25FCD6001A65D4 /* RestoreSessionStateWithoutNavigation.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 835CF9661D25FCD6001A65D4 /* RestoreSessionStateWithoutNavigation.cpp */; };
@@ -1534,6 +1535,7 @@
83148B04202AC68200BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior_Bundle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = InjectedBundleDisableOverrideBuiltinsBehavior_Bundle.cpp; sourceTree = "<group>"; };
83148B05202AC68200BADE99 /* InjectedBundleDisableOverrideBuiltinsBehavior.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = InjectedBundleDisableOverrideBuiltinsBehavior.cpp; sourceTree = "<group>"; };
83148B08202AC76800BADE99 /* override-builtins-test.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "override-builtins-test.html"; sourceTree = "<group>"; };
+ 834138C6203261B900F26960 /* AsyncPolicyForNavigationResponse.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = AsyncPolicyForNavigationResponse.mm; sourceTree = "<group>"; };
8349D3C11DB96DDA004A9F65 /* ContextMenuDownload.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ContextMenuDownload.mm; sourceTree = "<group>"; };
8349D3C31DB9724F004A9F65 /* link-with-download-attribute.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "link-with-download-attribute.html"; sourceTree = "<group>"; };
835CF9661D25FCD6001A65D4 /* RestoreSessionStateWithoutNavigation.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = RestoreSessionStateWithoutNavigation.cpp; sourceTree = "<group>"; };
@@ -2071,6 +2073,7 @@
A1DF74301C41B65800A2F4D0 /* AlwaysRevalidatedURLSchemes.mm */,
2DE71AFD1D49C0BD00904094 /* AnimatedResize.mm */,
63F668201F97C3AA0032EE51 /* ApplicationManifest.mm */,
+ 834138C6203261B900F26960 /* AsyncPolicyForNavigationResponse.mm */,
754CEC801F6722DC00D0039A /* AutoFillAvailable.mm */,
2DD355351BD08378005DF4A7 /* AutoLayoutIntegration.mm */,
374B7A5E1DF36EEE00ACCB6C /* BundleEditingDelegate.mm */,
@@ -3364,6 +3367,7 @@
2DE71AFE1D49C0BD00904094 /* AnimatedResize.mm in Sources */,
63F668221F97F7F90032EE51 /* ApplicationManifest.mm in Sources */,
6354F4D11F7C3AB500D89DF3 /* ApplicationManifestParser.cpp in Sources */,
+ 834138C7203261CA00F26960 /* AsyncPolicyForNavigationResponse.mm in Sources */,
7CCE7EB41A411A7E00447C4C /* AttributedString.mm in Sources */,
CDC8E48D1BC5CB4500594FEC /* AudioSessionCategoryIOS.mm in Sources */,
7C83E0B91D0A64F100FEBCF3 /* AutoLayoutIntegration.mm in Sources */,
Added: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm (0 => 228430)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm 2018-02-13 20:46:47 UTC (rev 228430)
@@ -0,0 +1,93 @@
+/*
+ * Copyright (C) 2018 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#import "config.h"
+
+#import "PlatformUtilities.h"
+#import "Test.h"
+#import <WebKit/WKWebView.h>
+#import <wtf/RetainPtr.h>
+
+#if WK_API_ENABLED
+static bool navigationComplete = false;
+static bool navigationFailed = false;
+
+@interface TestAsyncNavigationDelegate : NSObject <WKNavigationDelegate, WKUIDelegate>
+@end
+
+@implementation TestAsyncNavigationDelegate
+
+- (void)webView:(WKWebView *)webView didFinishNavigation:(null_unspecified WKNavigation *)navigation
+{
+ navigationComplete = true;
+}
+
+- (void)webView:(WKWebView *)webView didFailNavigation:(null_unspecified WKNavigation *)navigation withError:(NSError *)error
+{
+ navigationFailed = true;
+ navigationComplete = true;
+}
+
+- (void)webView:(WKWebView *)webView didFailProvisionalNavigation:(null_unspecified WKNavigation *)navigation withError:(NSError *)error
+{
+ navigationFailed = true;
+ navigationComplete = true;
+}
+
+- (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
+{
+ decisionHandler(WKNavigationActionPolicyAllow);
+}
+
+- (void)webView:(WKWebView *)webView decidePolicyForNavigationResponse:(WKNavigationResponse *)navigationResponse decisionHandler:(void (^)(WKNavigationResponsePolicy))decisionHandler
+{
+ int64_t deferredWaitTime = 100 * NSEC_PER_MSEC;
+ dispatch_time_t when = dispatch_time(DISPATCH_TIME_NOW, deferredWaitTime);
+ dispatch_after(when, dispatch_get_main_queue(), ^{
+ decisionHandler(WKNavigationResponsePolicyAllow);
+ });
+}
+@end
+
+namespace TestWebKitAPI {
+
+TEST(WebKit, RespondToPolicyForNavigationResponseAsynchronously)
+{
+ RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+ auto webView = adoptNS([[WKWebView alloc] init]);
+ auto delegate = adoptNS([[TestAsyncNavigationDelegate alloc] init]);
+ [webView setNavigationDelegate:delegate.get()];
+ [webView setUIDelegate:delegate.get()];
+
+ [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+ TestWebKitAPI::Util::run(&navigationComplete);
+
+ EXPECT_FALSE(navigationFailed);
+}
+
+} // namespace TestWebKitAPI
+
+#endif // WK_API_ENABLED
