- Revision
- 229828
- Author
- [email protected]
- Date
- 2018-03-21 14:19:06 -0700 (Wed, 21 Mar 2018)
Log Message
ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
https://bugs.webkit.org/show_bug.cgi?id=183787
Reviewed by Wenson Hsieh.
Source/WebCore:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
* loader/FrameLoaderClient.h:
Source/WebKit:
Without asynchronous policy delegates, when the client requests a navigation, we would:
1. Do a synchronous navigation policy check
2. If the client allows the navigation, start the provisional load
Starting the provisional load would freeze the layer tree until first meaningful
layout via WebFrameLoaderClient::provisionalLoadStarted() -> WebPage::didStartPageTransition().
When constructing a WebView and then requesting a load right away. This would make sure
we do not commit a layer tree for the initial about:blank page because the layer tree
would be frozen until we have something meaningful to show for the following load.
However, with asynchronous policy delegates, we are able to do a layer tree commit
during the asynchronous navigation policy check because the layer tree is not frozen
yet (provisional load has not started) and the process is not stuck on synchronous
IPC. When constructing a WebView and then requesting a load right away, this would
allow a layer tree commit for about:blank to happen before we've even started the
load. This would cause some API tests to fail on iOS.
To address the issue, we try and maintain pre-existing behavior by freezing the
layer tree during navigation policy decision.
* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
(WebKit::WebFrameLoaderClient::didDecidePolicyForNavigationAction):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::didStartNavigationPolicyCheck):
(WebKit::WebPage::didCompleteNavigationPolicyCheck):
* WebProcess/WebPage/WebPage.h:
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:
(-[AsyncPolicyDelegateForInsetTest webView:didFinishNavigation:]):
(-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationResponse:decisionHandler:]):
(-[AsyncPolicyDelegateForInsetTest webViewWebContentProcessDidTerminate:]):
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (229827 => 229828)
--- trunk/Source/WebCore/ChangeLog 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebCore/ChangeLog 2018-03-21 21:19:06 UTC (rev 229828)
@@ -1,3 +1,14 @@
+2018-03-21 Chris Dumez <[email protected]>
+
+ ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
+ https://bugs.webkit.org/show_bug.cgi?id=183787
+
+ Reviewed by Wenson Hsieh.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
+ * loader/FrameLoaderClient.h:
+
2018-03-21 Eric Carlson <[email protected]>
Clean up platform VideoFullscreenLayerManager
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (229827 => 229828)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2018-03-21 21:19:06 UTC (rev 229828)
@@ -3167,6 +3167,8 @@
// 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();
Modified: trunk/Source/WebCore/loader/FrameLoaderClient.h (229827 => 229828)
--- trunk/Source/WebCore/loader/FrameLoaderClient.h 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebCore/loader/FrameLoaderClient.h 2018-03-21 21:19:06 UTC (rev 229828)
@@ -190,6 +190,7 @@
virtual void dispatchDecidePolicyForResponse(const ResourceResponse&, const ResourceRequest&, FramePolicyFunction&&) = 0;
virtual void dispatchDecidePolicyForNewWindowAction(const NavigationAction&, const ResourceRequest&, FormState*, const String& frameName, FramePolicyFunction&&) = 0;
virtual void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool didReceiveRedirectResponse, FormState*, FramePolicyFunction&&) = 0;
+ virtual void didDecidePolicyForNavigationAction() { }
virtual void cancelPolicyCheck() = 0;
virtual void dispatchUnableToImplementPolicy(const ResourceError&) = 0;
Modified: trunk/Source/WebKit/ChangeLog (229827 => 229828)
--- trunk/Source/WebKit/ChangeLog 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebKit/ChangeLog 2018-03-21 21:19:06 UTC (rev 229828)
@@ -1,3 +1,40 @@
+2018-03-21 Chris Dumez <[email protected]>
+
+ ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
+ https://bugs.webkit.org/show_bug.cgi?id=183787
+
+ Reviewed by Wenson Hsieh.
+
+ Without asynchronous policy delegates, when the client requests a navigation, we would:
+ 1. Do a synchronous navigation policy check
+ 2. If the client allows the navigation, start the provisional load
+
+ Starting the provisional load would freeze the layer tree until first meaningful
+ layout via WebFrameLoaderClient::provisionalLoadStarted() -> WebPage::didStartPageTransition().
+
+ When constructing a WebView and then requesting a load right away. This would make sure
+ we do not commit a layer tree for the initial about:blank page because the layer tree
+ would be frozen until we have something meaningful to show for the following load.
+
+ However, with asynchronous policy delegates, we are able to do a layer tree commit
+ during the asynchronous navigation policy check because the layer tree is not frozen
+ yet (provisional load has not started) and the process is not stuck on synchronous
+ IPC. When constructing a WebView and then requesting a load right away, this would
+ allow a layer tree commit for about:blank to happen before we've even started the
+ load. This would cause some API tests to fail on iOS.
+
+ To address the issue, we try and maintain pre-existing behavior by freezing the
+ layer tree during navigation policy decision.
+
+ * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+ (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+ (WebKit::WebFrameLoaderClient::didDecidePolicyForNavigationAction):
+ * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
+ * WebProcess/WebPage/WebPage.cpp:
+ (WebKit::WebPage::didStartNavigationPolicyCheck):
+ (WebKit::WebPage::didCompleteNavigationPolicyCheck):
+ * WebProcess/WebPage/WebPage.h:
+
2018-03-21 Brent Fulgham <[email protected]>
Allow the WebContent process to read ViewBridge preferences
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (229827 => 229828)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2018-03-21 21:19:06 UTC (rev 229828)
@@ -825,6 +825,10 @@
return;
}
+ m_isDecidingNavigationPolicyDecision = true;
+ if (m_frame->isMainFrame())
+ webPage->didStartNavigationPolicyCheck();
+
// Always ignore requests with empty URLs.
if (request.isEmpty()) {
function(PolicyAction::Ignore);
@@ -896,8 +900,25 @@
m_frame->didReceivePolicyDecision(listenerID, policyAction, newNavigationID, downloadID, WTFMove(websitePolicies));
}
+void WebFrameLoaderClient::didDecidePolicyForNavigationAction()
+{
+ if (!m_isDecidingNavigationPolicyDecision)
+ return;
+
+ m_isDecidingNavigationPolicyDecision = false;
+
+ if (!m_frame || !m_frame->isMainFrame())
+ return;
+
+ if (auto* webPage = m_frame->page())
+ webPage->didCompleteNavigationPolicyCheck();
+}
+
void WebFrameLoaderClient::cancelPolicyCheck()
{
+ if (m_isDecidingNavigationPolicyDecision)
+ didDecidePolicyForNavigationAction();
+
m_frame->invalidatePolicyListener();
}
@@ -1311,6 +1332,8 @@
if (!webPage)
return;
+ ASSERT(!m_isDecidingNavigationPolicyDecision);
+
if (m_frame->isMainFrame()) {
webPage->didStartPageTransition();
m_didCompletePageTransition = false;
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h (229827 => 229828)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h 2018-03-21 21:19:06 UTC (rev 229828)
@@ -126,6 +126,7 @@
void dispatchDecidePolicyForResponse(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, WebCore::FramePolicyFunction&&) final;
void dispatchDecidePolicyForNewWindowAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, WebCore::FormState*, const String& frameName, WebCore::FramePolicyFunction&&) final;
void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::FramePolicyFunction&&) final;
+ void didDecidePolicyForNavigationAction() final;
void cancelPolicyCheck() final;
void dispatchUnableToImplementPolicy(const WebCore::ResourceError&) final;
@@ -275,6 +276,7 @@
WebFrame* m_frame;
RefPtr<PluginView> m_pluginView;
bool m_hasSentResponseToPluginView;
+ bool m_isDecidingNavigationPolicyDecision { false };
bool m_didCompletePageTransition;
bool m_frameHasCustomContentProvider;
bool m_frameCameFromPageCache;
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp (229827 => 229828)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2018-03-21 21:19:06 UTC (rev 229828)
@@ -2841,6 +2841,16 @@
frame->continueWillSubmitForm(listenerID);
}
+void WebPage::didStartNavigationPolicyCheck()
+{
+ m_drawingArea->setLayerTreeStateIsFrozen(true);
+}
+
+void WebPage::didCompleteNavigationPolicyCheck()
+{
+ m_drawingArea->setLayerTreeStateIsFrozen(false);
+}
+
void WebPage::didStartPageTransition()
{
m_drawingArea->setLayerTreeStateIsFrozen(true);
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.h (229827 => 229828)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.h 2018-03-21 21:19:06 UTC (rev 229828)
@@ -304,6 +304,8 @@
// -- Called from WebCore clients.
bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);
+ void didStartNavigationPolicyCheck();
+ void didCompleteNavigationPolicyCheck();
void didStartPageTransition();
void didCompletePageTransition();
void didCommitLoad(WebFrame*);
Modified: trunk/Tools/ChangeLog (229827 => 229828)
--- trunk/Tools/ChangeLog 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Tools/ChangeLog 2018-03-21 21:19:06 UTC (rev 229828)
@@ -1,5 +1,21 @@
2018-03-21 Chris Dumez <[email protected]>
+ ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
+ https://bugs.webkit.org/show_bug.cgi?id=183787
+
+ Reviewed by Wenson Hsieh.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:
+ (-[AsyncPolicyDelegateForInsetTest webView:didFinishNavigation:]):
+ (-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationAction:decisionHandler:]):
+ (-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationResponse:decisionHandler:]):
+ (-[AsyncPolicyDelegateForInsetTest webViewWebContentProcessDidTerminate:]):
+ (TestWebKitAPI::TEST):
+
+2018-03-21 Chris Dumez <[email protected]>
+
Fix DataInteractionTests.InjectedBundleAllowPerformTwoStepDrop to use synchronouslyLoadTestPageNamed
https://bugs.webkit.org/show_bug.cgi?id=183858
Modified: trunk/Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm (229827 => 229828)
--- trunk/Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm 2018-03-21 21:16:36 UTC (rev 229827)
+++ trunk/Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm 2018-03-21 21:19:06 UTC (rev 229828)
@@ -33,6 +33,44 @@
#import "UIKitSPI.h"
#import <WebKit/WKWebViewPrivate.h>
+@interface AsyncPolicyDelegateForInsetTest : NSObject<WKNavigationDelegate> {
+ @public BOOL _navigationComplete;
+}
+@property (nonatomic, copy) void (^webContentProcessDidTerminate)(WKWebView *);
+@end
+
+@implementation AsyncPolicyDelegateForInsetTest
+
+- (void)webView:(WKWebView *)webView didFinishNavigation:(null_unspecified WKNavigation *)navigation
+{
+ _navigationComplete = YES;
+}
+
+- (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))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(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);
+ });
+}
+
+- (void)webViewWebContentProcessDidTerminate:(WKWebView *)webView
+{
+ if (_webContentProcessDidTerminate)
+ _webContentProcessDidTerminate(webView);
+}
+@end
+
namespace TestWebKitAPI {
static const CGFloat viewHeight = 500;
@@ -125,6 +163,34 @@
EXPECT_EQ(-400, initialContentOffset.y);
}
+TEST(ScrollViewInsetTests, RestoreInitialContentOffsetAfterCrashWithAsyncPolicyDelegates)
+{
+ auto delegate = adoptNS([[AsyncPolicyDelegateForInsetTest alloc] init]);
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, viewHeight)]);
+ [webView scrollView].contentInset = UIEdgeInsetsMake(400, 0, 0, 0);
+ [webView setNavigationDelegate:delegate.get()];
+ delegate->_navigationComplete = NO;
+ NSURL *testResourceURL = [[[NSBundle mainBundle] bundleURL] URLByAppendingPathComponent:@"TestWebKitAPI.resources"];
+ [webView loadHTMLString:veryTallDocumentMarkup baseURL:testResourceURL];
+ Util::run(&delegate->_navigationComplete);
+
+ CGPoint initialContentOffset = [webView scrollView].contentOffset;
+ __block CGPoint contentOffsetAfterCrash = CGPointZero;
+ __block bool done = false;
+ [delegate setWebContentProcessDidTerminate:^(WKWebView *webView) {
+ contentOffsetAfterCrash = webView.scrollView.contentOffset;
+ done = true;
+ }];
+
+ [webView _killWebContentProcessAndResetState];
+ Util::run(&done);
+
+ EXPECT_EQ(initialContentOffset.x, contentOffsetAfterCrash.x);
+ EXPECT_EQ(initialContentOffset.y, contentOffsetAfterCrash.y);
+ EXPECT_EQ(0, initialContentOffset.x);
+ EXPECT_EQ(-400, initialContentOffset.y);
+}
+
} // namespace TestWebKitAPI
#endif // WK_API_ENABLED && PLATFORM(IOS)
