Changeset 295707 in webkit

Timestamp:

Jun 21, 2022 6:05:31 PM (2 years ago)

Author:

Matt Woodrow

Message:

ConnectionCocoa doesn't receive disconnect notifications before the client has finished initialising
​https://bugs.webkit.org/show_bug.cgi?id=241666

Reviewed by Kimmo Kinnunen.

Adds a MACH_NOTIFY_NO_SENDERS notification to the receive port of a server-side Connection object, so that
we can receive notifications if we fail to initialize the client side of the connection.
This gets removed again once the client side initialization completes, since we already have handling for
disconnections from that point onwards.

The test WebProcessTerminationAfterTooManyGPUProcessCrashes would hang in case the GPU Process would be
restarted and the test would terminate it before the connection was fully established, before the WebContent
process would receive the send right. The test is written in such a way that it is expected is that the GPUP
kill happens only after the connection has been re-established and the audio is playing.

• Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm:

(IPC::requestNoSenderNotifications):
(IPC::clearNoSenderNotifications):
(IPC::Connection::open):
(IPC::Connection::receiveSourceEventHandler):

• Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm:

(TEST):

Adds some early returns for failure cases, so that we don't call kill(0, 9).

Canonical link: ​https://commits.webkit.org/251712@main

Location:

trunk

Files:

• Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm (modified) (3 diffs)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm (modified) (3 diffs)

trunk/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm

@@ -189 +189 @@
 }
 
+static void requestNoSenderNotifications(mach_port_t port, mach_port_t notify)
+{
+    mach_port_t previousNotificationPort = MACH_PORT_NULL;
+    auto kr = mach_port_request_notification(mach_task_self(), port, MACH_NOTIFY_NO_SENDERS, 0, notify, MACH_MSG_TYPE_MAKE_SEND_ONCE, &previousNotificationPort);
+    ASSERT(kr == KERN_SUCCESS);
+    if (kr != KERN_SUCCESS) {
+        // If mach_port_request_notification fails, 'previousNotificationPort' will be uninitialized.
+        LOG_ERROR("mach_port_request_notification failed: (%x) %s", kr, mach_error_string(kr));
+    } else
+        deallocateSendRightSafely(previousNotificationPort);
+}
+
+static void requestNoSenderNotifications(mach_port_t port)
+{
+    requestNoSenderNotifications(port, port);
+}
+
+static void clearNoSenderNotifications(mach_port_t port)
+{
+    requestNoSenderNotifications(port, MACH_PORT_NULL);
+}
+
 bool Connection::open()
 {
@@ -240 +262 @@
         mach_port_mod_refs(mach_task_self(), receivePort, MACH_PORT_RIGHT_RECEIVE, -1);
     });
+    // Disconnections are normally handled by DISPATCH_MACH_SEND_DEAD on the m_sendSource, but that's not
+    // initialized until we receive the connection message from the client, so we need to request MACH_NOTIFY_NO_SENDERS
+    // on the receiving port until then.
+    if (m_isServer)
+        requestNoSenderNotifications(m_receivePort);
 
     m_connectionQueue->dispatch([strongRef = Ref { *this }, this] {
@@ -584 +611 @@
         if (m_sendPort) {
             ASSERT(MACH_PORT_VALID(m_receivePort));
-            mach_port_t previousNotificationPort = MACH_PORT_NULL;
-            auto kr = mach_port_request_notification(mach_task_self(), m_receivePort, MACH_NOTIFY_NO_SENDERS, 0, MACH_PORT_NULL, MACH_MSG_TYPE_MOVE_SEND_ONCE, &previousNotificationPort);
-            ASSERT(kr == KERN_SUCCESS);
-            if (kr != KERN_SUCCESS) {
-                // If mach_port_request_notification fails, 'previousNotificationPort' will be uninitialized.
-                LOG_ERROR("mach_port_request_notification failed: (%x) %s", kr, mach_error_string(kr));
-                previousNotificationPort = MACH_PORT_NULL;
-            }
-
-            if (previousNotificationPort != MACH_PORT_NULL)
-                deallocateSendRightSafely(previousNotificationPort);
+            clearNoSenderNotifications(m_receivePort);
 
             initializeSendSource();

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

@@ -116 +116 @@
 }
 
-// FIXME: Re-enable after webkit.org/b/240692 is resolved
-#if (PLATFORM(IOS))
-TEST(GPUProcess, DISABLED_WebProcessTerminationAfterTooManyGPUProcessCrashes)
-#else
 TEST(GPUProcess, WebProcessTerminationAfterTooManyGPUProcessCrashes)
-#endif
 {
     auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
@@ -161 +156 @@
     while ((![processPool _gpuProcessIdentifier] || [processPool _gpuProcessIdentifier] == gpuProcessPID) && timeout++ < 100)
         TestWebKitAPI::Util::sleep(0.1);
-    EXPECT_NE([processPool _gpuProcessIdentifier], 0);
-    EXPECT_NE([processPool _gpuProcessIdentifier], gpuProcessPID);
+    ASSERT_NE([processPool _gpuProcessIdentifier], 0);
+    ASSERT_NE([processPool _gpuProcessIdentifier], gpuProcessPID);
     gpuProcessPID = [processPool _gpuProcessIdentifier];
 
@@ -175 +170 @@
     while ((![processPool _gpuProcessIdentifier] || [processPool _gpuProcessIdentifier] == gpuProcessPID) && timeout++ < 100)
         TestWebKitAPI::Util::sleep(0.1);
-    EXPECT_NE([processPool _gpuProcessIdentifier], 0);
-    EXPECT_NE([processPool _gpuProcessIdentifier], gpuProcessPID);
+    ASSERT_NE([processPool _gpuProcessIdentifier], 0);
+    ASSERT_NE([processPool _gpuProcessIdentifier], gpuProcessPID);
     gpuProcessPID = [processPool _gpuProcessIdentifier];