From 2dbbdbf493dbbebdaba039efa0e0b35bfa9fce53 Mon Sep 17 00:00:00 2001 From: Chris Dumez Date: Fri, 7 Apr 2023 09:23:27 -0700 Subject: [PATCH] REGRESSION (iOS 16.4): Chrome crashes in WebBackForwardCache::takeSuspendedPage 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 --- .../Shared/Cocoa/SandboxExtensionCocoa.mm | 5 +- Source/WebKit/UIProcess/WebProcessPool.cpp | 11 ++- .../WebKitCocoa/ProcessSwapOnNavigation.mm | 81 +++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm b/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm index 4b903f4bbccd..299814f77d3e 100644 --- a/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm +++ b/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm @@ -339,9 +339,8 @@ String resolvePathForSandboxExtension(StringView path) auto SandboxExtension::createHandlesForMachLookup(Span services, std::optional auditToken, MachBootstrapOptions machBootstrapOptions, OptionSet flags) -> Vector { auto handles = createHandlesForResources(services, [auditToken, flags] (ASCIILiteral service) -> std::optional { - 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) diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp index 43f3fbc61079..38b46f03d3c9 100644 --- a/Source/WebKit/UIProcess/WebProcessPool.cpp +++ b/Source/WebKit/UIProcess/WebProcessPool.cpp @@ -1811,7 +1811,7 @@ void WebProcessPool::removeProcessFromOriginCacheSet(WebProcessProxy& process) void WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigation& navigation, Ref&& sourceProcess, const URL& sourceURL, ProcessSwapRequestedByClient processSwapRequestedByClient, WebProcessProxy::LockdownMode lockdownMode, const FrameInfoData& frameInfo, Ref&& dataStore, CompletionHandler&&, 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&& 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&& 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); }); }); diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm index 578a8a141372..20c394595038 100644 --- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm +++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm @@ -54,6 +54,7 @@ #import #import #import +#import #import #import #import @@ -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)