Title: [295707] trunk
Revision
295707
Author
mattwood...@apple.com
Date
2022-06-21 18:05:31 -0700 (Tue, 21 Jun 2022)

Log 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

Modified Paths

Diff

Modified: trunk/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm (295706 => 295707)


--- trunk/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm	2022-06-22 00:39:46 UTC (rev 295706)
+++ trunk/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm	2022-06-22 01:05:31 UTC (rev 295707)
@@ -188,6 +188,28 @@
     m_xpcConnection = identifier.xpcConnection;
 }
 
+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()
 {
     if (m_isServer) {
@@ -239,6 +261,11 @@
 #endif
         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] {
         dispatch_resume(m_receiveSource.get());
@@ -583,18 +610,8 @@
         
         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;
-            }
+            clearNoSenderNotifications(m_receivePort);
 
-            if (previousNotificationPort != MACH_PORT_NULL)
-                deallocateSendRightSafely(previousNotificationPort);
-
             initializeSendSource();
             dispatch_resume(m_sendSource.get());
         }

Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm (295706 => 295707)


--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm	2022-06-22 00:39:46 UTC (rev 295706)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm	2022-06-22 01:05:31 UTC (rev 295707)
@@ -115,12 +115,7 @@
     EXPECT_TRUE([webView _isPlayingAudio]);
 }
 
-// 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]);
     WKPreferencesSetBoolValueForKeyForTesting((__bridge WKPreferencesRef)[configuration preferences], true, WKStringCreateWithUTF8CString("UseGPUProcessForMediaEnabled"));
@@ -160,8 +155,8 @@
     timeout = 0;
     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];
 
     // Make sure the WebView's WebProcess did not crash or get terminated.
@@ -174,8 +169,8 @@
     timeout = 0;
     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];
 
     // Make sure the WebView's WebProcess did not crash or get terminated.
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to