Regression(PSON) MESSAGE_CHECK() hit under WebPageProxy::didFailProvisionalLoadForFra...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2019 02:32:24 +0000 (02:32 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2019 02:32:24 +0000 (02:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194568
<rdar://problem/47944490>

Reviewed by Ryosuke Niwa.

Source/WebKit:

When the provisional process crashes, it is unsafe to call ProvisionalPageProxy::cancel() because
the WebProcessProxy clears its frame map as soon as the process crashes. Calling cancel() after
that would call WebPageProxy::didFailProvisionalLoadForFrameShared(), which would try to look up
the frame by ID and MESSAGE_CHECK() that the frame is not null. We would fail this check since
the frame has been removed from the WebProcessProxy at this point.

* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _provisionalWebProcessIdentifier]):
* UIProcess/API/Cocoa/WKWebViewPrivate.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didFailProvisionalLoadForFrameShared):
(WebKit::WebPageProxy::provisionalProcessDidTerminate):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
(-[PSONNavigationDelegate webView:didStartProvisionalNavigation:]):

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index 23713b5..62cc724 100644 (file)
@@ -1,3 +1,24 @@
+2019-02-12  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON) MESSAGE_CHECK() hit under WebPageProxy::didFailProvisionalLoadForFrameShared()
+        https://bugs.webkit.org/show_bug.cgi?id=194568
+        <rdar://problem/47944490>
+
+        Reviewed by Ryosuke Niwa.
+
+        When the provisional process crashes, it is unsafe to call ProvisionalPageProxy::cancel() because
+        the WebProcessProxy clears its frame map as soon as the process crashes. Calling cancel() after
+        that would call WebPageProxy::didFailProvisionalLoadForFrameShared(), which would try to look up
+        the frame by ID and MESSAGE_CHECK() that the frame is not null. We would fail this check since
+        the frame has been removed from the WebProcessProxy at this point.
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _provisionalWebProcessIdentifier]):
+        * UIProcess/API/Cocoa/WKWebViewPrivate.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didFailProvisionalLoadForFrameShared):
+        (WebKit::WebPageProxy::provisionalProcessDidTerminate):
+
 2019-02-12  Per Arne Vollan  <pvollan@apple.com>
 
         [iOS] Youtube fails to play.
index 5e63e83..752a3a4 100644 (file)
@@ -4768,6 +4768,18 @@ FOR_EACH_PRIVATE_WKCONTENTVIEW_ACTION(FORWARD_ACTION_TO_WKCONTENTVIEW)
     return _page->processIdentifier();
 }
 
+- (pid_t)_provisionalWebProcessIdentifier
+{
+    if (![self _isValid])
+        return 0;
+
+    auto* provisionalPage = _page->provisionalPageProxy();
+    if (!provisionalPage)
+        return 0;
+
+    return provisionalPage->process().processIdentifier();
+}
+
 - (void)_killWebContentProcess
 {
     if (![self _isValid])
index 12b01e8..1851099 100644 (file)
@@ -154,6 +154,7 @@ typedef NS_OPTIONS(NSUInteger, _WKRectEdge) {
 @property (nonatomic, setter=_setUserContentExtensionsEnabled:) BOOL _userContentExtensionsEnabled WK_API_AVAILABLE(macosx(10.11), ios(9.0));
 
 @property (nonatomic, readonly) pid_t _webProcessIdentifier;
+@property (nonatomic, readonly) pid_t _provisionalWebProcessIdentifier WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 
 @property (nonatomic, getter=_isEditable, setter=_setEditable:) BOOL _editable WK_API_AVAILABLE(macosx(10.11), ios(9.0));
 
index 45a8b97..5d73531 100644 (file)
@@ -3936,7 +3936,7 @@ void WebPageProxy::didFailProvisionalLoadForFrameShared(Ref<WebProcessProxy>&& p
     PageClientProtector protector(pageClient());
 
     WebFrameProxy* frame = process->webFrame(frameID);
-    MESSAGE_CHECK(m_process, frame);
+    MESSAGE_CHECK(process, frame);
 
     if (m_controlledByAutomation) {
         if (auto* automationSession = process->processPool().automationSession())
@@ -6506,7 +6506,6 @@ void WebPageProxy::processDidTerminate(ProcessTerminationReason reason)
 void WebPageProxy::provisionalProcessDidTerminate()
 {
     ASSERT(m_provisionalPage);
-    m_provisionalPage->cancel();
     m_provisionalPage = nullptr;
 }
 
index b80525d..fe1a0d7 100644 (file)
@@ -1,3 +1,16 @@
+2019-02-12  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON) MESSAGE_CHECK() hit under WebPageProxy::didFailProvisionalLoadForFrameShared()
+        https://bugs.webkit.org/show_bug.cgi?id=194568
+        <rdar://problem/47944490>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+        (-[PSONNavigationDelegate webView:didStartProvisionalNavigation:]):
+
 2019-02-12  Jonathan Bedard  <jbedard@apple.com>
 
         webkitpy: Remove bug_dummy from parsed test expectations
index a513b42..22e1cd5 100644 (file)
@@ -98,6 +98,7 @@ static RetainPtr<NSURL> clientRedirectDestinationURL;
 
 @interface PSONNavigationDelegate : NSObject <WKNavigationDelegate> {
     @public void (^decidePolicyForNavigationAction)(WKNavigationAction *, void (^)(WKNavigationActionPolicy));
+    @public void (^didStartProvisionalNavigationHandler)();
     @public void (^didCommitNavigationHandler)();
 }
 @end
@@ -125,6 +126,8 @@ static RetainPtr<NSURL> clientRedirectDestinationURL;
 - (void)webView:(WKWebView *)webView didStartProvisionalNavigation:(null_unspecified WKNavigation *)navigation
 {
     didStartProvisionalLoad = true;
+    if (didStartProvisionalNavigationHandler)
+        didStartProvisionalNavigationHandler();
 }
 
 - (void)webView:(WKWebView *)webView didCommitNavigation:(null_unspecified WKNavigation *)navigation
@@ -1488,6 +1491,40 @@ TEST(ProcessSwap, ServerRedirect2)
     EXPECT_WK_STREQ(@"pson://www.webkit.org/main1.html", [[webView URL] absoluteString]);
 }
 
+TEST(ProcessSwap, TerminateProcessRightAfterSwap)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"pson"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    [webView configuration].preferences.safeBrowsingEnabled = NO;
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    delegate->didStartProvisionalNavigationHandler = ^{
+        EXPECT_NE(0, [webView _provisionalWebProcessIdentifier]);
+        kill([webView _provisionalWebProcessIdentifier], 9);
+    };
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::sleep(0.5);
+}
+
 static const char* linkToWebKitBytes = R"PSONRESOURCE(
 <body>
   <a id="testLink" href="pson://www.webkit.org/main.html">Link</a>