Changeset 244799 in webkit

Timestamp:

Apr 30, 2019, 1:20:00 PM (7 years ago)

Author:

Chris Dumez

Message:

Only use a related page's process if that page has not been closed yet
​https://bugs.webkit.org/show_bug.cgi?id=197393
<rdar://problem/50302423>

Reviewed by Tim Horton.

Source/WebKit:

We should not attempt to use a related page's process if that related page has already been closed.
Once closed, a page's process is invalid and trying to launch a new process for the closed page
leads to crashes such as the one in the radar.

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::launchProcess):

• UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::createWebPage):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/WebPageProxy.cpp (modified) (1 diff)
• Source/WebKit/UIProcess/WebProcessPool.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-04-30  Chris Dumez  <cdumez@apple.com>
+
+        Only use a related page's process if that page has not been closed yet
+        https://bugs.webkit.org/show_bug.cgi?id=197393
+        <rdar://problem/50302423>
+
+        Reviewed by Tim Horton.
+
+        We should not attempt to use a related page's process if that related page has already been closed.
+        Once closed, a page's process is invalid and trying to launch a new process for the closed page
+        leads to crashes such as the one in the radar.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::launchProcess):
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::createWebPage):
+
 2019-04-30  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -728 +728 @@
     auto& processPool = m_process->processPool();
 
-    if (auto* relatedPage = m_configuration->relatedPage())
+    auto* relatedPage = m_configuration->relatedPage();
+    if (relatedPage && !relatedPage->isClosed())
         m_process = relatedPage->ensureRunningProcess();
     else

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -1211 +1211 @@
 
     RefPtr<WebProcessProxy> process;
-    if (pageConfiguration->relatedPage()) {
+    auto* relatedPage = pageConfiguration->relatedPage();
+    if (relatedPage && !relatedPage->isClosed()) {
         // Sharing processes, e.g. when creating the page via window.open().
         process = &pageConfiguration->relatedPage()->ensureRunningProcess();

trunk/Tools/ChangeLog

@@ +1 @@
+2019-04-30  Chris Dumez  <cdumez@apple.com>
+
+        Only use a related page's process if that page has not been closed yet
+        https://bugs.webkit.org/show_bug.cgi?id=197393
+        <rdar://problem/50302423>
+
+        Reviewed by Tim Horton.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-04-30  Aakash Jain  <aakash_jain@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -4710 +4710 @@
 }
 
+TEST(ProcessSwap, ReloadRelatedWebViewAfterCrash)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webView1Configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webView1Configuration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webView1Configuration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView1 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webView1Configuration.get()]);
+    auto delegate = adoptNS([[TestNavigationDelegate alloc] init]);
+    __block bool didCrash = false;
+    [delegate setWebContentProcessDidTerminate:^(WKWebView *view) {
+        [view reload];
+        didCrash = true;
+    }];
+    [delegate setDidFinishNavigation:^(WKWebView *, WKNavigation *) {
+        done = true;
+    }];
+
+    [webView1 setNavigationDelegate:delegate.get()];
+
+    auto webView2Configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webView2Configuration setProcessPool:processPool.get()];
+    [webView2Configuration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+    webView2Configuration.get()._relatedWebView = webView1.get(); // webView2 will be related to webView1 and webView1's URL will be used for process swap decision.
+    auto webView2 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webView2Configuration.get()]);
+    [webView2 setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+    [webView1 loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid1 = [webView1 _webProcessIdentifier];
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+    [webView2 loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid2 = [webView2 _webProcessIdentifier];
+
+    EXPECT_EQ(pid1, pid2); // WebViews are related so they should share the same process.
+
+    [webView1 _close];
+    webView1 = nullptr;
+
+    kill(pid1, 9);
+
+    TestWebKitAPI::Util::run(&didCrash);
+    didCrash = false;
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+}
+
 TEST(ProcessSwap, TerminatedSuspendedPageProcess)
 {