Changeset 273695 in webkit

Timestamp:

Mar 1, 2021, 3:07:58 PM (6 years ago)

Author:

Chris Dumez

Message:

Crash under WebProcessPool::pageBeginUsingWebsiteDataStore()
​https://bugs.webkit.org/show_bug.cgi?id=222574
<rdar://68340471>

Reviewed by Geoffrey Garen.

Source/WebKit:

The issue was that when WebProcessProxy::requestTermination() was called (e.g. process is killed by WebKit
for using too much memory), we would fail to remove the process from the WebProcessCache. Because the
terminated would stay in the cache (even though WebProcessProxy::shutDown() was called), we could potentially
try and use it later on for a navigation to the same domain. This would lead to crashes because
WebProcessProxy::shutDown() has already been called.

Note that we were previously correctly removing the process from the cache in case of a proper crash, inside
WebProcessProxy::processDidTerminateOrFailedToLaunch(). I have moved the logic to remove from the cache
from processDidTerminateOrFailedToLaunch() to shutDown() to avoid similar issues in the future.

• UIProcess/API/Cocoa/WKProcessPool.mm:

(-[WKProcessPool _requestWebProcessTermination:]):

• UIProcess/API/Cocoa/WKProcessPoolPrivate.h:
• UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::shutDown):
(WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm (modified) (1 diff)
• Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h (modified) (1 diff)
• Source/WebKit/UIProcess/WebProcessProxy.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-03-01  Chris Dumez  <cdumez@apple.com>
+
+        Crash under WebProcessPool::pageBeginUsingWebsiteDataStore()
+        https://bugs.webkit.org/show_bug.cgi?id=222574
+        <rdar://68340471>
+
+        Reviewed by Geoffrey Garen.
+
+        The issue was that when WebProcessProxy::requestTermination() was called (e.g. process is killed by WebKit
+        for using too much memory), we would fail to remove the process from the WebProcessCache. Because the
+        terminated would stay in the cache (even though WebProcessProxy::shutDown() was called), we could potentially
+        try and use it later on for a navigation to the same domain. This would lead to crashes because
+        WebProcessProxy::shutDown() has already been called.
+
+        Note that we were previously correctly removing the process from the cache in case of a proper crash, inside
+        WebProcessProxy::processDidTerminateOrFailedToLaunch(). I have moved the logic to remove from the cache
+        from processDidTerminateOrFailedToLaunch() to shutDown() to avoid similar issues in the future.
+
+        * UIProcess/API/Cocoa/WKProcessPool.mm:
+        (-[WKProcessPool _requestWebProcessTermination:]):
+        * UIProcess/API/Cocoa/WKProcessPoolPrivate.h:
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::shutDown):
+        (WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
+
 2021-03-01  Stephan Szabo  <stephan.szabo@sony.com>
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm

@@ -388 +388 @@
 }
 
+- (BOOL)_requestWebProcessTermination:(pid_t)pid
+{
+    for (auto& process : _processPool->processes()) {
+        if (process->processIdentifier() == pid)
+            process->requestTermination(WebKit::ProcessTerminationReason::RequestedByClient);
+        return YES;
+    }
+    return NO;
+}
+
 - (void)_makeNextWebProcessLaunchFailForTesting
 {

trunk/Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h

@@ -111 +111 @@
 - (void)_setUseSeparateServiceWorkerProcess:(BOOL)forceServiceWorkerProcess WK_API_AVAILABLE(macos(10.15.4), ios(13.4));
 - (pid_t)_gpuProcessIdentifier WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
+- (BOOL)_requestWebProcessTermination:(pid_t)pid WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
 
 // Test only. Returns web processes running web pages (does not include web processes running service workers)

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -449 +449 @@
     RELEASE_ASSERT(isMainThreadOrCheckDisabled());
 
+    if (m_isInProcessCache) {
+        processPool().webProcessCache().removeProcess(*this, WebProcessCache::ShouldShutDownProcess::No);
+        ASSERT(!m_isInProcessCache);
+    }
+
     shutDownProcess();
 
@@ -867 +872 @@
     for (auto& callback : isResponsiveCallbacks)
         callback(false);
-
-    if (m_isInProcessCache) {
-        processPool().webProcessCache().removeProcess(*this, WebProcessCache::ShouldShutDownProcess::No);
-        ASSERT(!m_isInProcessCache);
-    }
 
     if (isStandaloneServiceWorkerProcess())

trunk/Tools/ChangeLog

@@ +1 @@
+2021-03-01  Chris Dumez  <cdumez@apple.com>
+
+        Crash under WebProcessPool::pageBeginUsingWebsiteDataStore()
+        https://bugs.webkit.org/show_bug.cgi?id=222574
+        <rdar://68340471>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2021-03-01  Sam Weinig  <weinig@apple.com>
 

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

@@ -3691 +3691 @@
 }
 
+TEST(ProcessSwap, ProcessCrashedWhileInTheCache)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    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 navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+    [navigationDelegate setDidFinishNavigation:^(WKWebView *, WKNavigation *) {
+        done = true;
+    }];
+
+    int webkitPID = 0;
+
+    @autoreleasepool {
+        auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+        [webView setNavigationDelegate:navigationDelegate.get()];
+
+        NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+
+        [webView loadRequest:request];
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+        webkitPID = [webView _webProcessIdentifier];
+    }
+
+    while ([processPool _processCacheSize] != 1)
+        TestWebKitAPI::Util::sleep(0.1);
+
+    kill(webkitPID, 9);
+
+    while ([processPool _processCacheSize])
+        TestWebKitAPI::Util::sleep(0.1);
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+
+    [webView loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_NE(webkitPID, [webView _webProcessIdentifier]);
+}
+
+TEST(ProcessSwap, ProcessTerminatedWhileInTheCache)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    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 navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+    [navigationDelegate setDidFinishNavigation:^(WKWebView *, WKNavigation *) {
+        done = true;
+    }];
+
+    int webkitPID = 0;
+
+    @autoreleasepool {
+        auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+        [webView setNavigationDelegate:navigationDelegate.get()];
+
+        NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+
+        [webView loadRequest:request];
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+        webkitPID = [webView _webProcessIdentifier];
+    }
+
+    while ([processPool _processCacheSize] != 1)
+        TestWebKitAPI::Util::sleep(0.1);
+
+    EXPECT_TRUE([processPool _requestWebProcessTermination:webkitPID]);
+    TestWebKitAPI::Util::spinRunLoop(100);
+
+    EXPECT_EQ(0U, [processPool _processCacheSize]);
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+
+    [webView loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_NE(webkitPID, [webView _webProcessIdentifier]);
+}
+
 TEST(ProcessSwap, UseWebProcessCacheForLoadInNewView)
 {