ASSERT(MACH_PORT_VALID(m_sendPort)) hit in IPC::Connection::initializeSendSource(): Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000657b2caa4 WTFCrash + 36 (Assertions.cpp:271) 1 com.apple.WebKit 0x0000000101660379 IPC::Connection::initializeSendSource() + 313 (ConnectionMac.mm:390) 2 com.apple.WebKit 0x000000010165f8a3 IPC::Connection::open() + 1171 (ConnectionMac.mm:206) 3 com.apple.WebKit 0x0000000101ef8f1a WebKit::WebInspectorUI::establishConnection(IPC::Attachment, unsigned long long, bool, unsigned int) + 346 (WebInspectorUI.cpp:82) 4 com.apple.WebKit 0x0000000101f0218f void IPC::callMemberFunctionImpl<WebKit::WebInspectorUI, void (WebKit::WebInspectorUI::*)(IPC::Attachment, unsigned long long, bool, unsigned int), std::__1::tuple<IPC::Attachment, unsigned long long, bool, unsigned int>, 0ul, 1ul, 2ul, 3ul>(WebKit::WebInspectorUI*, void (WebKit::WebInspectorUI::*)(IPC::Attachment, unsigned long long, bool, unsigned int), std::__1::tuple<IPC::Attachment, unsigned long long, bool, unsigned int>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 399 (HandleMessage.h:41) 5 com.apple.WebKit 0x0000000101f01dd0 void IPC::callMemberFunction<WebKit::WebInspectorUI, void (WebKit::WebInspectorUI::*)(IPC::Attachment, unsigned long long, bool, unsigned int), std::__1::tuple<IPC::Attachment, unsigned long long, bool, unsigned int>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(std::__1::tuple<IPC::Attachment, unsigned long long, bool, unsigned int>&&, WebKit::WebInspectorUI*, void (WebKit::WebInspectorUI::*)(IPC::Attachment, unsigned long long, bool, unsigned int)) + 96 (HandleMessage.h:47) 6 com.apple.WebKit 0x0000000101f00a2a void IPC::handleMessage<Messages::WebInspectorUI::EstablishConnection, WebKit::WebInspectorUI, void (WebKit::WebInspectorUI::*)(IPC::Attachment, unsigned long long, bool, unsigned int)>(IPC::Decoder&, WebKit::WebInspectorUI*, void (WebKit::WebInspectorUI::*)(IPC::Attachment, unsigned long long, bool, unsigned int)) + 330 (HandleMessage.h:127) 7 com.apple.WebKit 0x0000000101efffe5 WebKit::WebInspectorUI::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 133 (WebInspectorUIMessageReceiver.cpp:42) Saw this locally when running the layout tests.
This is probably happening because it's a dead mach port. That assertion is likely too strict (this can happen if the process its talking to has died, which need not be fatal). This should probably be weekend, but I want to review the code again.
<rdar://problem/39003606>
Code: ASSERT(MACH_PORT_VALID(m_sendPort)); mach_port_t sendPort = m_sendPort; dispatch_source_set_cancel_handler(m_sendSource, ^{ // Release our send right. mach_port_deallocate(mach_task_self(), sendPort); });
(In reply to Brent Fulgham from comment #1) > This is probably happening because it's a dead mach port. That assertion is > likely too strict (this can happen if the process its talking to has died, > which need not be fatal). > > This should probably be weekend, but I want to review the code again. lol. This should probably be WEAKENED.
It looks like the WebContent process is being asked to establish a connection to a dead mach port.
(In reply to Brent Fulgham from comment #5) > It looks like the WebContent process is being asked to establish a > connection to a dead mach port. I think I was running this when I saw the assertion yesterday: Tools/Scripts/run-webkit-tests LayoutTests/http/tests/xmlhttprequest/ LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/ Note that there was no test failure. However, on the results page, there was a WebContent crash.
(In reply to Chris Dumez from comment #6) > (In reply to Brent Fulgham from comment #5) > > It looks like the WebContent process is being asked to establish a > > connection to a dead mach port. > > I think I was running this when I saw the assertion yesterday: > Tools/Scripts/run-webkit-tests LayoutTests/http/tests/xmlhttprequest/ > LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/ > > Note that there was no test failure. However, on the results page, there was > a WebContent crash. Yeah -- I think this is a case where the WebInspector process has a connection to a WebContent process that has shut down. We shouldn't assert in that case, we should recover gracefully. We should also avoid establishing new connections to dead processes, so this patch will be a little larger than just changing the assertion.
Created attachment 336803 [details] Patch
Comment on attachment 336803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336803&action=review > Source/WebKit/Platform/IPC/Connection.h:138 > + static bool identifierIsDead(Identifier identifier) { return identifier.port == MACH_PORT_DEAD; } Do we ever need to distinguish null and dead? It does not look like it. If so, could we rename identifierIsNull() to identifierIdValid() and have it use MACH_PORT_VALID() internally? > Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:399 > + if (sendPort) I do not think this check is useful, is it? We copied m_sendPort into sendPort above and asserted that it was valid before that. There is no way for sendPort to check after that since it is a copy. If we want to be extra safe still, why not use if (MACH_PORT_VALID(sendPort)) ?
Comment on attachment 336803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336803&action=review >> Source/WebKit/Platform/IPC/Connection.h:138 >> + static bool identifierIsDead(Identifier identifier) { return identifier.port == MACH_PORT_DEAD; } > > Do we ever need to distinguish null and dead? It does not look like it. If so, could we rename identifierIsNull() to identifierIdValid() and have it use MACH_PORT_VALID() internally? Agreed. I'll change it. >> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:399 >> + if (sendPort) > > I do not think this check is useful, is it? We copied m_sendPort into sendPort above and asserted that it was valid before that. There is no way for sendPort to check after that since it is a copy. > If we want to be extra safe still, why not use if (MACH_PORT_VALID(sendPort)) ? Ok -- I'll remove it. The main reason I added it is that there is no reason to send mach_port_deallocate to a null port (though there IS value in sending it to a dead port if you used it to allocate a connection). So this was more about avoiding an unnecessary IPC call.
Committed r230084: <https://trac.webkit.org/changeset/230084>