REGRESSION (iOS 16.4): Chrome crashes in WebBackForwardCache::takeSus…

…pendedPage

https://bugs.webkit.org/show_bug.cgi?id=255102
rdar://107723629

Reviewed by Geoffrey Garen.

We recently added an AddAllowedFirstPartyForCookies async IPC call inside
WebProcessPool::processForNavigation(), right after we decide which process to
use. Because the IPC is async, this means that the selected process may crash
while we're waiting for a response. If this happens, we now call
processForNavigation() again to select a new process instead of trying to
proceed with the navigation with the terminated process.

Similarly, also make sure that the destination suspendedPage is still valid
after receiving the async IPC, in case the back/forward cache got cleared
during the IPC (e.g. due to memory pressure).

* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigation):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Canonical link: https://commits.webkit.org/262709@main

Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm

@@ -339,9 +339,8 @@ String resolvePathForSandboxExtension(StringView path)
 auto SandboxExtension::createHandlesForMachLookup(Span<const ASCIILiteral> services, std::optional<audit_token_t> auditToken, MachBootstrapOptions machBootstrapOptions, OptionSet<Flags> flags) -> Vector<Handle>
 {
     auto handles = createHandlesForResources(services, [auditToken, flags] (ASCIILiteral service) -> std::optional<Handle> {
-        auto handle = createHandleForMachLookup(service, auditToken, flags);
-        ASSERT(handle);
-        return handle;
+        // Note that createHandleForMachLookup() may return null if the process has just crashed.
+        return createHandleForMachLookup(service, auditToken, flags);
     });
 
 #if HAVE(MACH_BOOTSTRAP_EXTENSION)

Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -1811,7 +1811,7 @@ void WebProcessPool::removeProcessFromOriginCacheSet(WebProcessProxy& process)
 
 void WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigation& navigation, Ref<WebProcessProxy>&& sourceProcess, const URL& sourceURL, ProcessSwapRequestedByClient processSwapRequestedByClient, WebProcessProxy::LockdownMode lockdownMode, const FrameInfoData& frameInfo, Ref<WebsiteDataStore>&& dataStore, CompletionHandler<void(Ref<WebProcessProxy>&&, SuspendedPageProxy*, const String&)>&& completionHandler)
 {
-    processForNavigationInternal(page, navigation, sourceProcess.copyRef(), sourceURL, processSwapRequestedByClient, lockdownMode, frameInfo, WTFMove(dataStore), [this, page = Ref { page }, navigation = Ref { navigation }, sourceProcess = sourceProcess.copyRef(), sourceURL, processSwapRequestedByClient, completionHandler = WTFMove(completionHandler)](Ref<WebProcessProxy>&& process, SuspendedPageProxy* suspendedPage, const String& reason) mutable {
+    processForNavigationInternal(page, navigation, sourceProcess.copyRef(), sourceURL, processSwapRequestedByClient, lockdownMode, frameInfo, dataStore.copyRef(), [this, protectedThis = Ref { *this }, page = Ref { page }, navigation = Ref { navigation }, sourceProcess = sourceProcess.copyRef(), sourceURL, processSwapRequestedByClient, dataStore, frameInfo, lockdownMode, completionHandler = WTFMove(completionHandler)](Ref<WebProcessProxy>&& process, SuspendedPageProxy* suspendedPage, const String& reason) mutable {
         // We are process-swapping so automatic process prewarming would be beneficial if the client has not explicitly enabled / disabled it.
         bool doingAnAutomaticProcessSwap = processSwapRequestedByClient == ProcessSwapRequestedByClient::No && process.ptr() != sourceProcess.ptr();
         if (doingAnAutomaticProcessSwap && !configuration().wasAutomaticProcessWarmingSetByClient() && !configuration().clientWouldBenefitFromAutomaticProcessPrewarming()) {
@@ -1838,7 +1838,14 @@ void WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigat
 
         auto processIdentifier = process->coreProcessIdentifier();
         auto preventProcessShutdownScope = process->shutdownPreventingScope();
-        page->websiteDataStore().networkProcess().sendWithAsyncReply(Messages::NetworkProcess::AddAllowedFirstPartyForCookies(processIdentifier, RegistrableDomain(navigation->currentRequest().url()), loadedWebArchive), [completionHandler = WTFMove(completionHandler), process = WTFMove(process), preventProcessShutdownScope = WTFMove(preventProcessShutdownScope), suspendedPage = WTFMove(suspendedPage), reason] () mutable {
+        page->websiteDataStore().networkProcess().sendWithAsyncReply(Messages::NetworkProcess::AddAllowedFirstPartyForCookies(processIdentifier, RegistrableDomain(navigation->currentRequest().url()), loadedWebArchive), [this, protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler), page, navigation, process = WTFMove(process), preventProcessShutdownScope = WTFMove(preventProcessShutdownScope), suspendedPage = WTFMove(suspendedPage), reason, dataStore = WTFMove(dataStore), frameInfo, sourceProcess = WTFMove(sourceProcess), sourceURL, lockdownMode, processSwapRequestedByClient] () mutable {
+            // Since the IPC is asynchronous, make sure the destination process and suspended page are still valid.
+            if (process->state() == AuxiliaryProcessProxy::State::Terminated) {
+                processForNavigation(page, navigation, WTFMove(sourceProcess), sourceURL, processSwapRequestedByClient, lockdownMode, frameInfo, WTFMove(dataStore), WTFMove(completionHandler));
+                return;
+            }
+            if (suspendedPage && (!navigation->targetItem() || suspendedPage != navigation->targetItem()->suspendedPage()))
+                suspendedPage = nullptr;
             completionHandler(WTFMove(process), suspendedPage, reason);
         });
     });

Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -54,6 +54,7 @@
 #import <WebKit/_WKInspector.h>
 #import <WebKit/_WKProcessPoolConfiguration.h>
 #import <WebKit/_WKWebsiteDataStoreConfiguration.h>
+#import <notify.h>
 #import <wtf/BlockPtr.h>
 #import <wtf/Deque.h>
 #import <wtf/HashMap.h>
@@ -8080,6 +8081,86 @@ static void configureLockdownWKWebViewConfiguration(WKWebViewConfiguration *conf
     EXPECT_NE(pid2, pid3);
 }
 
+TEST(ProcessSwap, DestinationProcessTerminationDuringProcessSwap)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([WKWebViewConfiguration new]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+
+    auto handler = adoptNS([PSONScheme new]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectZero configuration:webViewConfiguration.get()]);
+    auto navigationDelegate = adoptNS([PSONNavigationDelegate new]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+
+    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/source.html"]]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/destination.html"]]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    navigationDelegate->decidePolicyForNavigationAction = [webkitPID](WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
+        decisionHandler(WKNavigationActionPolicyAllow);
+
+        kill(webkitPID, 9);
+    };
+
+    // The navigation may or may not finish depending on timing.
+    // Depending on when we get notified of the process termination, we may cancel the provisional load.
+    [webView goBack];
+
+    // We cannot reliably wait for the load to complete so we just wait a bit to make sure we're not crashing.
+    auto before = MonotonicTime::now();
+    while ((MonotonicTime::now() - before) < 1_s)
+        TestWebKitAPI::Util::spinRunLoop(10);
+}
+
+TEST(ProcessSwap, MemoryPressureDuringProcessSwap)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([WKWebViewConfiguration new]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+
+    auto handler = adoptNS([PSONScheme new]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectZero configuration:webViewConfiguration.get()]);
+    auto navigationDelegate = adoptNS([PSONNavigationDelegate new]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+
+    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/source.html"]]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/destination.html"]]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    navigationDelegate->decidePolicyForNavigationAction = [](WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
+        decisionHandler(WKNavigationActionPolicyAllow);
+
+        notify_post("org.WebKit.lowMemory");
+    };
+
+    // The navigation may or may not finish depending on timing.
+    // Back/Forward cache processes exit on memory pressure, which may cancel the provisional load.
+    [webView goBack];
+
+    // We cannot reliably wait for the load to complete so we just wait a bit to make sure we're not crashing.
+    auto before = MonotonicTime::now();
+    while ((MonotonicTime::now() - before) < 1_s)
+        TestWebKitAPI::Util::spinRunLoop(10);
+}
+
 #if PLATFORM(IOS)
 
 TEST(ProcessSwap, CannotDisableLockdownModeWithoutBrowserEntitlement)