WebContent process sometimes hangs in WebProcess::ensureNetworkProcessConnection
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Apr 2018 21:21:50 +0000 (21:21 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Apr 2018 21:21:50 +0000 (21:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184326

Reviewed by Chris Dumez.

The hang was caused by UI process never sending the reply back to GetNetworkProcessConnection
due to m_pendingOutgoingMachMessage being set and the event handler for DISPATCH_MACH_SEND_POSSIBLE
never getting called. This is because the event handler registration happens asynchronously,
and may not have completed by the time we send the first IPC to the web content process
in which case it can timeout and we may never get the callback.

Fixed the hang by waiting for the event handler registration to be completed using
dispatch_source_set_registration_handler. To do this, this patch adds a new boolean instance variable,
m_isInitializingSendSource, to Connection which is set to true between the time mach port is created
and until the event handler registration has been completed. platformCanSendOutgoingMessages returns
false while m_isInitializingSendSource is set to prevent the attempt to send messages like we do when
m_pendingOutgoingMachMessage is set to true.

* Platform/IPC/Connection.h:
(IPC::Connection::m_isInitializingSendSource): Added.
* Platform/IPC/mac/ConnectionMac.mm:
(IPC::Connection::platformInvalidate): Set m_isInitializingSendSource to false.
(IPC::Connection::sendMessage): Assert that m_isInitializingSendSource is false.
(IPC::Connection::platformCanSendOutgoingMessages const): Return false if m_isInitializingSendSource
is set to true.
(IPC::Connection::sendOutgoingMessage): Assert that m_isInitializingSendSource is false.
(IPC::Connection::initializeSendSource): Set m_isInitializingSendSource to true temporarily until
dispatch_source_set_registration_handler's callback is called. Resume and send any pending outgoing
messages.
(IPC::Connection::resumeSendSource): Extracted from initializeSendSource.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230314 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Platform/IPC/Connection.h
Source/WebKit/Platform/IPC/mac/ConnectionMac.mm

index f8fcfba..18b023c 100644 (file)
@@ -1,3 +1,36 @@
+2018-04-05  Ryosuke Niwa  <rniwa@webkit.org>
+
+        WebContent process sometimes hangs in WebProcess::ensureNetworkProcessConnection
+        https://bugs.webkit.org/show_bug.cgi?id=184326
+
+        Reviewed by Chris Dumez.
+
+        The hang was caused by UI process never sending the reply back to GetNetworkProcessConnection
+        due to m_pendingOutgoingMachMessage being set and the event handler for DISPATCH_MACH_SEND_POSSIBLE
+        never getting called. This is because the event handler registration happens asynchronously,
+        and may not have completed by the time we send the first IPC to the web content process
+        in which case it can timeout and we may never get the callback.
+
+        Fixed the hang by waiting for the event handler registration to be completed using
+        dispatch_source_set_registration_handler. To do this, this patch adds a new boolean instance variable,
+        m_isInitializingSendSource, to Connection which is set to true between the time mach port is created
+        and until the event handler registration has been completed. platformCanSendOutgoingMessages returns
+        false while m_isInitializingSendSource is set to prevent the attempt to send messages like we do when
+        m_pendingOutgoingMachMessage is set to true.
+
+        * Platform/IPC/Connection.h:
+        (IPC::Connection::m_isInitializingSendSource): Added.
+        * Platform/IPC/mac/ConnectionMac.mm:
+        (IPC::Connection::platformInvalidate): Set m_isInitializingSendSource to false.
+        (IPC::Connection::sendMessage): Assert that m_isInitializingSendSource is false.
+        (IPC::Connection::platformCanSendOutgoingMessages const): Return false if m_isInitializingSendSource
+        is set to true.
+        (IPC::Connection::sendOutgoingMessage): Assert that m_isInitializingSendSource is false.
+        (IPC::Connection::initializeSendSource): Set m_isInitializingSendSource to true temporarily until
+        dispatch_source_set_registration_handler's callback is called. Resume and send any pending outgoing
+        messages.
+        (IPC::Connection::resumeSendSource): Extracted from initializeSendSource.
+
 2018-04-05  Youenn Fablet  <youenn@apple.com>
 
         WebRTC data channel only applications require capture permissions for direct connections
index 8a25593..e568c22 100644 (file)
@@ -326,6 +326,7 @@ private:
     // Called on the connection queue.
     void receiveSourceEventHandler();
     void initializeSendSource();
+    void resumeSendSource();
 
     mach_port_t m_sendPort { MACH_PORT_NULL };
     dispatch_source_t m_sendSource { nullptr };
@@ -334,6 +335,7 @@ private:
     dispatch_source_t m_receiveSource { nullptr };
 
     std::unique_ptr<MachMessage> m_pendingOutgoingMachMessage;
+    bool m_isInitializingSendSource { false };
 
     OSObjectPtr<xpc_connection_t> m_xpcConnection;
 #elif OS(WINDOWS)
index a0edd4f..1f7b7b7 100644 (file)
@@ -132,6 +132,7 @@ void Connection::platformInvalidate()
     }
 
     m_pendingOutgoingMachMessage = nullptr;
+    m_isInitializingSendSource = false;
     m_isConnected = false;
 
     ASSERT(m_sendPort);
@@ -247,6 +248,7 @@ bool Connection::sendMessage(std::unique_ptr<MachMessage> message)
 {
     ASSERT(message);
     ASSERT(!m_pendingOutgoingMachMessage);
+    ASSERT(!m_isInitializingSendSource);
 
     // Send the message.
     kern_return_t kr = mach_msg(message->header(), MACH_SEND_MSG | MACH_SEND_TIMEOUT | MACH_SEND_NOTIFY, message->size(), 0, MACH_PORT_NULL, MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
@@ -273,12 +275,12 @@ bool Connection::sendMessage(std::unique_ptr<MachMessage> message)
 
 bool Connection::platformCanSendOutgoingMessages() const
 {
-    return !m_pendingOutgoingMachMessage;
+    return !m_pendingOutgoingMachMessage && !m_isInitializingSendSource;
 }
 
 bool Connection::sendOutgoingMessage(std::unique_ptr<Encoder> encoder)
 {
-    ASSERT(!m_pendingOutgoingMachMessage);
+    ASSERT(!m_pendingOutgoingMachMessage && !m_isInitializingSendSource);
 
     Vector<Attachment> attachments = encoder->releaseAttachments();
     
@@ -371,8 +373,15 @@ bool Connection::sendOutgoingMessage(std::unique_ptr<Encoder> encoder)
 void Connection::initializeSendSource()
 {
     m_sendSource = dispatch_source_create(DISPATCH_SOURCE_TYPE_MACH_SEND, m_sendPort, DISPATCH_MACH_SEND_DEAD | DISPATCH_MACH_SEND_POSSIBLE, m_connectionQueue->dispatchQueue());
+    m_isInitializingSendSource = true;
 
     RefPtr<Connection> connection(this);
+    dispatch_source_set_registration_handler(m_sendSource, [connection] {
+        if (!connection->m_sendSource)
+            return;
+        connection->m_isInitializingSendSource = false;
+        connection->resumeSendSource();
+    });
     dispatch_source_set_event_handler(m_sendSource, [connection] {
         if (!connection->m_sendSource)
             return;
@@ -386,9 +395,7 @@ void Connection::initializeSendSource()
 
         if (data & DISPATCH_MACH_SEND_POSSIBLE) {
             // FIXME: Figure out why we get spurious DISPATCH_MACH_SEND_POSSIBLE events.
-            if (connection->m_pendingOutgoingMachMessage)
-                connection->sendMessage(WTFMove(connection->m_pendingOutgoingMachMessage));
-            connection->sendOutgoingMessages();
+            connection->resumeSendSource();
             return;
         }
     });
@@ -401,6 +408,14 @@ void Connection::initializeSendSource()
     });
 }
 
+void Connection::resumeSendSource()
+{
+    ASSERT(!m_isInitializingSendSource);
+    if (m_pendingOutgoingMachMessage)
+        sendMessage(WTFMove(m_pendingOutgoingMachMessage));
+    sendOutgoingMessages();
+}
+
 static std::unique_ptr<Decoder> createMessageDecoder(mach_msg_header_t* header)
 {
     if (!(header->msgh_bits & MACH_MSGH_BITS_COMPLEX)) {