Attachment 343647 Details for Bug 187065 – Patch

[patch] Patch

bug-187065-20180626152225.patch (text/plain), 13.69 KB, created by Chris Dumez on 2018-06-26 15:22:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-26 15:22:25 PDT

Size: 13.69 KB

patch

obsolete

>Subversion Revision: 233216
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 82b9a9a393a21854ba8b0c95be4baaf5adf2ba7d..80cda8527f90ae47968161cdf93dc1c55b05d6ba 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-06-26  Chris Dumez  <cdumez@apple.com>
>+
>+        Deal better with the network process crashing on startup
>+        https://bugs.webkit.org/show_bug.cgi?id=187065
>+        <rdar://problem/41451622>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When a network process crashes on startup, we would not attempt to relaunch it. If there were web
>+        processes waiting for a connection to this network process, we would send them an invalid connection
>+        identifier which would cause them to forcefully crash.
>+
>+        Instead, we now apply the same policy whether a network process crashes on startup or later:
>+        - We attempt to relaunch the network process
>+        - If there were pending connections from WebContent processes, we ask the new Network process instead.
>+
>+        As a result, WebContent processes no longer crash in this case. Instead, they wait for a valid
>+        connection to the network process.
>+
>+        * UIProcess/API/Cocoa/WKProcessPool.mm:
>+        (-[WKProcessPool _makeNextNetworkProcessLaunchFailForTesting]):
>+        * UIProcess/API/Cocoa/WKProcessPoolPrivate.h:
>+        * UIProcess/Network/NetworkProcessProxy.cpp:
>+        (WebKit::NetworkProcessProxy::getLaunchOptions):
>+        (WebKit::NetworkProcessProxy::didFinishLaunching):
>+        * UIProcess/Network/NetworkProcessProxy.h:
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::networkProcessCrashed):
>+        * UIProcess/WebProcessPool.h:
>+
> 2018-06-26  Tim Horton  <timothy_horton@apple.com>
> 
>         Promote some experimental features to traditional features
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm b/Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
>index bac9c2795a78bd80883b0a69464b50556712d3b4..c4397cff64ffe081c82bd91bfffe2b7167d38d47 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
>@@ -473,6 +473,11 @@ - (void)_makeNextWebProcessLaunchFailForTesting
>     _processPool->setShouldMakeNextWebProcessLaunchFailForTesting(true);
> }
> 
>+- (void)_makeNextNetworkProcessLaunchFailForTesting
>+{
>+    _processPool->setShouldMakeNextNetworkProcessLaunchFailForTesting(true);
>+}
>+
> - (size_t)_prewarmedWebProcessCount
> {
>     size_t result = 0;
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h b/Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h
>index db00c2ffe61ec1d1ebb26b8da74fb17f1e5d9610..0f7ebd21c15b1bf3b6e6b85cf6248842b46d22b2 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h
>@@ -97,6 +97,7 @@
> - (size_t)_serviceWorkerProcessCount WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> - (void)_syncNetworkProcessCookies WK_API_AVAILABLE(macosx(10.13), ios(11.0));
> - (void)_makeNextWebProcessLaunchFailForTesting WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
>+- (void)_makeNextNetworkProcessLaunchFailForTesting WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> // Test only. Returns web processes running web pages (does not include web processes running service workers)
> - (size_t)_webPageContentProcessCount WK_API_AVAILABLE(macosx(10.13.4), ios(11.3));
>diff --git a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
>index e1838cb6128c002162297dfefd954148281e16d2..e1d359924f3e14ab374367beeea336a7c78cc2db 100644
>--- a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
>@@ -102,6 +102,11 @@ void NetworkProcessProxy::getLaunchOptions(ProcessLauncher::LaunchOptions& launc
> {
>     launchOptions.processType = ProcessLauncher::ProcessType::Network;
>     ChildProcessProxy::getLaunchOptions(launchOptions);
>+
>+    if (processPool().shouldMakeNextNetworkProcessLaunchFailForTesting()) {
>+        processPool().setShouldMakeNextNetworkProcessLaunchFailForTesting(false);
>+        launchOptions.shouldMakeProcessLaunchFailForTesting = true;
>+    }
> }
> 
> void NetworkProcessProxy::connectionWillOpen(IPC::Connection& connection)
>@@ -205,25 +210,6 @@ void NetworkProcessProxy::networkProcessCrashed()
>     m_processPool.networkProcessCrashed(*this, WTFMove(pendingReplies));
> }
> 
>-void NetworkProcessProxy::networkProcessFailedToLaunch()
>-{
>-    // The network process must have crashed or exited, send any pending sync replies we might have.
>-    while (!m_pendingConnectionReplies.isEmpty()) {
>-        auto reply = m_pendingConnectionReplies.takeFirst();
>-
>-#if USE(UNIX_DOMAIN_SOCKETS) || OS(WINDOWS)
>-        reply(IPC::Attachment());
>-#elif OS(DARWIN)
>-        reply(IPC::Attachment(0, MACH_MSG_TYPE_MOVE_SEND));
>-#else
>-        notImplemented();
>-#endif
>-    }
>-    clearCallbackStates();
>-    // Tell the network process manager to forget about this network process proxy. This may cause us to be deleted.
>-    m_processPool.networkProcessFailedToLaunch(*this);
>-}
>-
> void NetworkProcessProxy::clearCallbackStates()
> {
>     while (!m_pendingFetchWebsiteDataCallbacks.isEmpty())
>@@ -359,7 +345,7 @@ void NetworkProcessProxy::didFinishLaunching(ProcessLauncher* launcher, IPC::Con
>     ChildProcessProxy::didFinishLaunching(launcher, connectionIdentifier);
> 
>     if (!IPC::Connection::identifierIsValid(connectionIdentifier)) {
>-        networkProcessFailedToLaunch();
>+        networkProcessCrashed();
>         return;
>     }
> 
>diff --git a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
>index f707c69291f68120437cac8c3f23b783f7eb9757..7327f89d85426282a4918e17ad1b1fe2a2cbce3b 100644
>--- a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
>+++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
>@@ -112,7 +112,6 @@ private:
>     void connectionWillOpen(IPC::Connection&) override;
>     void processWillShutDown(IPC::Connection&) override;
> 
>-    void networkProcessFailedToLaunch();
>     void networkProcessCrashed();
>     void clearCallbackStates();
> 
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 72a83df810af37cce4937ff5bf478c66c1b0d7ae..e861c9b427f0961d2a4f8c83d1edbb469e82a25f 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -543,17 +543,6 @@ NetworkProcessProxy& WebProcessPool::ensureNetworkProcess(WebsiteDataStore* with
> }
> 
> void WebProcessPool::networkProcessCrashed(NetworkProcessProxy& networkProcessProxy, Vector<Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply>&& pendingReplies)
>-{
>-    networkProcessFailedToLaunch(networkProcessProxy);
>-    ASSERT(!m_networkProcess);
>-    if (pendingReplies.isEmpty())
>-        return;
>-    auto& newNetworkProcess = ensureNetworkProcess();
>-    for (auto& reply : pendingReplies)
>-        newNetworkProcess.getNetworkProcessConnection(WTFMove(reply));
>-}
>-
>-void WebProcessPool::networkProcessFailedToLaunch(NetworkProcessProxy& networkProcessProxy)
> {
>     ASSERT(m_networkProcess);
>     ASSERT(&networkProcessProxy == m_networkProcess.get());
>@@ -569,6 +558,13 @@ void WebProcessPool::networkProcessFailedToLaunch(NetworkProcessProxy& networkPr
> 
>     // Leave the process proxy around during client call, so that the client could query the process identifier.
>     m_networkProcess = nullptr;
>+
>+    // Attempt to re-launch.
>+    if (pendingReplies.isEmpty())
>+        return;
>+    auto& newNetworkProcess = ensureNetworkProcess();
>+    for (auto& reply : pendingReplies)
>+        newNetworkProcess.getNetworkProcessConnection(WTFMove(reply));
> }
> 
> void WebProcessPool::getNetworkProcessConnection(Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply&& reply)
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.h b/Source/WebKit/UIProcess/WebProcessPool.h
>index 83b063e9e5df516c0dadfefd3b92bc549a17244a..28e08c86b99e51f8326d1fc87e2a74dacf952e55 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.h
>+++ b/Source/WebKit/UIProcess/WebProcessPool.h
>@@ -281,6 +281,8 @@ public:
> 
>     void setShouldMakeNextWebProcessLaunchFailForTesting(bool value) { m_shouldMakeNextWebProcessLaunchFailForTesting = value; }
>     bool shouldMakeNextWebProcessLaunchFailForTesting() const { return m_shouldMakeNextWebProcessLaunchFailForTesting; }
>+    void setShouldMakeNextNetworkProcessLaunchFailForTesting(bool value) { m_shouldMakeNextNetworkProcessLaunchFailForTesting = value; }
>+    bool shouldMakeNextNetworkProcessLaunchFailForTesting() const { return m_shouldMakeNextNetworkProcessLaunchFailForTesting; }
> 
>     void reportWebContentCPUTime(Seconds cpuTime, uint64_t activityState);
> 
>@@ -332,7 +334,6 @@ public:
>     NetworkProcessProxy& ensureNetworkProcess(WebsiteDataStore* withWebsiteDataStore = nullptr);
>     NetworkProcessProxy* networkProcess() { return m_networkProcess.get(); }
>     void networkProcessCrashed(NetworkProcessProxy&, Vector<Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply>&&);
>-    void networkProcessFailedToLaunch(NetworkProcessProxy&);
> 
>     void getNetworkProcessConnection(Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply&&);
> 
>@@ -643,6 +644,7 @@ private:
>     bool m_alwaysRunsAtBackgroundPriority;
>     bool m_shouldTakeUIBackgroundAssertion;
>     bool m_shouldMakeNextWebProcessLaunchFailForTesting { false };
>+    bool m_shouldMakeNextNetworkProcessLaunchFailForTesting { false };
> 
>     UserObservablePageCounter m_userObservablePageCounter;
>     ProcessSuppressionDisabledCounter m_processSuppressionDisabledForPageCounter;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index ed0c1a82c6cea88434e7290251e733eacfa7c0fc..4d14bfe13d086a98318e8c0f5532b555928538f5 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-06-26  Chris Dumez  <cdumez@apple.com>
>+
>+        Deal better with the network process crashing on startup
>+        https://bugs.webkit.org/show_bug.cgi?id=187065
>+        <rdar://problem/41451622>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add layout test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKit/NetworkProcessCrashWithPendingConnection.mm:
>+        (-[MonitorWebContentCrashNavigationDelegate _webView:webContentProcessDidTerminateWithReason:]):
>+        (-[MonitorWebContentCrashNavigationDelegate webView:didFinishNavigation:]):
>+        (TestWebKitAPI::TEST):
>+
> 2018-06-26  Daniel Bates  <dabates@apple.com>
> 
>         contributors.json fails to parse after r233209
>diff --git a/Tools/TestWebKitAPI/Tests/WebKit/NetworkProcessCrashWithPendingConnection.mm b/Tools/TestWebKitAPI/Tests/WebKit/NetworkProcessCrashWithPendingConnection.mm
>index ca593330b4485aecd3f5c9d211107e96610b5488..3343d6afba326db3743d74aa771f530419e0995e 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKit/NetworkProcessCrashWithPendingConnection.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKit/NetworkProcessCrashWithPendingConnection.mm
>@@ -37,12 +37,31 @@
> 
> #if WK_API_ENABLED
> 
>-namespace TestWebKitAPI {
>-
> static bool loadedOrCrashed = false;
> static bool loaded = false;
> static bool webProcessCrashed = false;
> static bool networkProcessCrashed = false;
>+static pid_t pid;
>+static WKWebView* testView;
>+
>+@interface MonitorWebContentCrashNavigationDelegate : NSObject <WKNavigationDelegate>
>+@end
>+
>+@implementation MonitorWebContentCrashNavigationDelegate
>+
>+- (void)_webView:(WKWebView *)webView webContentProcessDidTerminateWithReason:(_WKProcessTerminationReason)reason
>+{
>+    webProcessCrashed = true;
>+}
>+
>+- (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
>+{
>+    loaded = true;
>+}
>+
>+@end
>+
>+namespace TestWebKitAPI {
> 
> TEST(WebKit, NetworkProcessCrashWithPendingConnection)
> {
>@@ -107,6 +126,47 @@ TEST(WebKit, NetworkProcessCrashWithPendingConnection)
>     EXPECT_FALSE(webProcessCrashed);
> }
> 
>+TEST(WebKit, NetworkProcessRelaunchOnLaunchFailure)
>+{
>+    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
>+    auto processPool = adoptNS([[WKProcessPool alloc] init]);
>+
>+    [configuration setProcessPool:processPool.get()];
>+
>+    webProcessCrashed = false;
>+    loaded = false;
>+    networkProcessCrashed = false;
>+
>+    WKContextClientV0 client;
>+    memset(&client, 0, sizeof(client));
>+    client.networkProcessDidCrash = [](WKContextRef context, const void* clientInfo) {
>+        pid = [testView _webProcessIdentifier];
>+        EXPECT_GT(pid, 0); // The WebProcess is already launched when the network process crashes.
>+        networkProcessCrashed = true;
>+    };
>+    WKContextSetClient(static_cast<WKContextRef>(processPool.get()), &client.base);
>+
>+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
>+    testView = webView.get();
>+
>+    // Constucting a WebView starts a network process so terminate this one. The page load below will then request a network process and we
>+    // make this new network process launch crash on startup.
>+    [processPool _terminateNetworkProcess];
>+    [processPool _makeNextNetworkProcessLaunchFailForTesting];
>+
>+    auto delegate = adoptNS([[MonitorWebContentCrashNavigationDelegate alloc] init]);
>+    [webView setNavigationDelegate:delegate.get()];
>+
>+    [webView loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
>+    TestWebKitAPI::Util::run(&loaded);
>+
>+    EXPECT_TRUE(networkProcessCrashed);
>+    EXPECT_FALSE(webProcessCrashed);
>+    EXPECT_GT([webView _webProcessIdentifier], 0);
>+    EXPECT_EQ(pid, [webView _webProcessIdentifier]);
>+    EXPECT_GT([processPool.get() _networkProcessIdentifier], 0);
>+}
>+
> } // namespace TestWebKitAPI
> 
> #endif

Actions: View | Formatted Diff | Diff

Attachments on bug 187065: 343647