diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog index 3317e57f3247..2710c7289cdc 100644 --- a/Source/WebKit/ChangeLog +++ b/Source/WebKit/ChangeLog @@ -1,3 +1,29 @@ +2021-04-05 Alex Christensen + + REGRESSION(r267763) NetworkProcess never terminates + https://bugs.webkit.org/show_bug.cgi?id=224191 + + + Reviewed by Chris Dumez. + + Before r267763, when a WebProcessPool was deallocated, the NetworkProcess it owned was terminated. + Since then, once you start using a NetworkProcess, it will be kept until your app closes or it crashes. + To reclaim these resources in a way similar to how we did before, we now terminate the network process in two situations: + 1. If all WebsiteDataStores associated with it are deallocated. This happens if you have never used the default WKWebsiteDataStore. + 2. If all WebProcessPools are deallocated. This can still happen if you do use the default WKWebsiteDataStore, which is never deallocated. + + Covered by API tests. + + * UIProcess/API/Cocoa/WKWebsiteDataStore.mm: + (+[WKWebsiteDataStore _defaultNetworkProcessExists]): + * UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h: + * UIProcess/Network/NetworkProcessProxy.cpp: + (WebKit::NetworkProcessProxy::defaultNetworkProcessExists): + (WebKit::NetworkProcessProxy::removeSession): + * UIProcess/Network/NetworkProcessProxy.h: + * UIProcess/WebProcessPool.cpp: + (WebKit::WebProcessPool::~WebProcessPool): + 2021-04-05 Alex Christensen Reduce crash inside getAuditToken diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm b/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm index c3934552a23c..270a8dcbe613 100644 --- a/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm +++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm @@ -29,6 +29,7 @@ #import "APIString.h" #import "AuthenticationChallengeDispositionCocoa.h" #import "CompletionHandlerCallChecker.h" +#import "NetworkProcessProxy.h" #import "ShouldGrandfatherStatistics.h" #import "WKHTTPCookieStoreInternal.h" #import "WKNSArray.h" @@ -724,4 +725,9 @@ - (BOOL)_networkProcessExists return !!_websiteDataStore->networkProcessIfExists(); } ++ (BOOL)_defaultNetworkProcessExists +{ + return !!WebKit::NetworkProcessProxy::defaultNetworkProcess(); +} + @end diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h b/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h index 7e8abec6897d..e153b5ef109e 100644 --- a/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h +++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h @@ -98,6 +98,7 @@ typedef NS_OPTIONS(NSUInteger, _WKWebsiteDataStoreFetchOptions) { - (pid_t)_networkProcessIdentifier WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); + (void)_makeNextNetworkProcessLaunchFailForTesting WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); - (BOOL)_networkProcessExists WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); ++ (BOOL)_defaultNetworkProcessExists WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); @end diff --git a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp index 5066d93a8f73..c57cf959e3f7 100644 --- a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp +++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp @@ -101,17 +101,17 @@ Vector> NetworkProcessProxy::allNetworkProcesses() return processes; } -static RefPtr& defaultProcess() +RefPtr& NetworkProcessProxy::defaultNetworkProcess() { static NeverDestroyed> process; return process.get(); } -Ref NetworkProcessProxy::defaultNetworkProcess() +Ref NetworkProcessProxy::ensureDefaultNetworkProcess() { - if (!defaultProcess()) - defaultProcess() = NetworkProcessProxy::create(); - return *defaultProcess(); + if (!defaultNetworkProcess()) + defaultNetworkProcess() = NetworkProcessProxy::create(); + return *defaultNetworkProcess(); } void NetworkProcessProxy::terminate() @@ -310,8 +310,8 @@ void NetworkProcessProxy::networkProcessDidTerminate(TerminationReason reason) m_uploadActivity = WTF::nullopt; - if (defaultProcess() == this) - defaultProcess() = nullptr; + if (defaultNetworkProcess() == this) + defaultNetworkProcess() = nullptr; Ref protectedThis(*this); for (auto* processPool : WebProcessPool::allProcessPools()) @@ -1279,6 +1279,9 @@ void NetworkProcessProxy::removeSession(WebsiteDataStore& websiteDataStore) if (canSendMessage()) send(Messages::NetworkProcess::DestroySession { websiteDataStore.sessionID() }, 0); + + if (m_websiteDataStores.computesEmpty()) + defaultNetworkProcess() = nullptr; } WebsiteDataStore* NetworkProcessProxy::websiteDataStoreFromSessionID(PAL::SessionID sessionID) diff --git a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h index 31c1269b4a6e..2e74e0a7dc35 100644 --- a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h +++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h @@ -108,7 +108,8 @@ class NetworkProcessProxy final : public AuxiliaryProcessProxy, private ProcessT using DomainInNeedOfStorageAccess = WebCore::RegistrableDomain; using OpenerDomain = WebCore::RegistrableDomain; - static Ref defaultNetworkProcess(); + static Ref ensureDefaultNetworkProcess(); + static RefPtr& defaultNetworkProcess(); static Ref create() { return adoptRef(*new NetworkProcessProxy); } ~NetworkProcessProxy(); diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp index 558280666f94..76ea0591bb32 100644 --- a/Source/WebKit/UIProcess/WebProcessPool.cpp +++ b/Source/WebKit/UIProcess/WebProcessPool.cpp @@ -376,6 +376,9 @@ WebProcessPool::~WebProcessPool() process->shutDown(); } + + if (processPools().isEmpty() && !!NetworkProcessProxy::defaultNetworkProcess()) + NetworkProcessProxy::defaultNetworkProcess() = nullptr; } void WebProcessPool::initializeClient(const WKContextClientBase* client) diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp index a27a93a96400..0f5aed3739dd 100644 --- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp +++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp @@ -200,7 +200,7 @@ static Ref networkProcessForSession(PAL::SessionID sessionI return NetworkProcessProxy::create(); #else UNUSED_PARAM(sessionID); - return NetworkProcessProxy::defaultNetworkProcess(); + return NetworkProcessProxy::ensureDefaultNetworkProcess(); #endif } diff --git a/Tools/ChangeLog b/Tools/ChangeLog index 40e693369ff4..4517bdd0642a 100644 --- a/Tools/ChangeLog +++ b/Tools/ChangeLog @@ -1,3 +1,14 @@ +2021-04-05 Alex Christensen + + REGRESSION(r267763) NetworkProcess never terminates + https://bugs.webkit.org/show_bug.cgi?id=224191 + + + Reviewed by Chris Dumez. + + * TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm: + (TEST): + 2021-04-05 Alex Christensen Resurrect Mac CMake build diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm index 44c931f04093..f4d1ef89b27a 100644 --- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm +++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm @@ -36,7 +36,7 @@ #import #import -TEST(WebKit, NetworkProcessEntitlements) +TEST(NetworkProcess, Entitlements) { auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:adoptNS([[WKWebViewConfiguration alloc] init]).get()]); [webView synchronouslyLoadTestPageNamed:@"simple"]; @@ -82,7 +82,7 @@ HTTPServer server([&] (Connection connection) { checkReferer([NSURL URLWithString:shorterHost], shorterHost.UTF8String); } -TEST(WebKit, NetworkProcessLaunchOnlyWhenNecessary) +TEST(NetworkProcess, LaunchOnlyWhenNecessary) { auto webView = adoptNS([WKWebView new]); [webView configuration].websiteDataStore._resourceLoadStatisticsEnabled = YES; @@ -91,7 +91,7 @@ HTTPServer server([&] (Connection connection) { EXPECT_FALSE([[webView configuration].websiteDataStore _networkProcessExists]); } -TEST(WebKit, NetworkProcessCrashWhenNotAssociatedWithDataStore) +TEST(NetworkProcess, CrashWhenNotAssociatedWithDataStore) { pid_t networkProcessPID = 0; @autoreleasepool { @@ -120,3 +120,28 @@ HTTPServer server([&] (Connection connection) { [webView synchronouslyLoadTestPageNamed:@"simple"]; EXPECT_NE(networkProcessPID, [webView configuration].websiteDataStore._networkProcessIdentifier); } + +TEST(NetworkProcess, TerminateWhenUnused) +{ + RetainPtr retainedPool; + @autoreleasepool { + auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]); + configuration.get().websiteDataStore = [WKWebsiteDataStore nonPersistentDataStore]; + retainedPool = configuration.get().processPool; + auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 0, 0) configuration:configuration.get()]); + [webView synchronouslyLoadTestPageNamed:@"simple"]; + EXPECT_TRUE([WKWebsiteDataStore _defaultNetworkProcessExists]); + } + while ([WKWebsiteDataStore _defaultNetworkProcessExists]) + TestWebKitAPI::Util::spinRunLoop(); + + retainedPool = nil; + + @autoreleasepool { + auto webView = adoptNS([WKWebView new]); + [webView synchronouslyLoadTestPageNamed:@"simple"]; + EXPECT_TRUE([WKWebsiteDataStore _defaultNetworkProcessExists]); + } + while ([WKWebsiteDataStore _defaultNetworkProcessExists]) + TestWebKitAPI::Util::spinRunLoop(); +}