Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
ConnectionCocoa doesn't receive disconnect notifications before the c…
…lient 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
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295707 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
mattwoodrow committed Jun 22, 2022
1 parent 4b7ee11 commit c7aa5b9
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 20 deletions.
39 changes: 28 additions & 11 deletions Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm
Expand Up @@ -188,6 +188,28 @@ void watchdogTimerFired()
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) {
Expand Down Expand Up @@ -239,6 +261,11 @@ void watchdogTimerFired()
#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());
Expand Down Expand Up @@ -583,17 +610,7 @@ void watchdogTimerFired()

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();
dispatch_resume(m_sendSource.get());
Expand Down
13 changes: 4 additions & 9 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm
Expand Up @@ -115,12 +115,7 @@ static NSInteger getPixelIndex(NSInteger x, NSInteger y, NSInteger width)
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"));
Expand Down Expand Up @@ -160,8 +155,8 @@ static NSInteger getPixelIndex(NSInteger x, NSInteger y, NSInteger width)
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.
Expand All @@ -174,8 +169,8 @@ static NSInteger getPixelIndex(NSInteger x, NSInteger y, NSInteger width)
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.
Expand Down

0 comments on commit c7aa5b9

Please sign in to comment.