Bug 184122 – REGRESSION(r230035): ASSERT(MACH_PORT_VALID(m_sendPort)) hit in IPC::Connection::initializeSendSource()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/39003606>

[Chris Dumez]

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);
    });

[Brent Fulgham]

(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.

[Brent Fulgham]

It looks like the WebContent process is being asked to establish a connection to a dead mach port.

[Chris Dumez]

(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.

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 336803 [details]
Patch

[Chris Dumez]

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)) ?

[Brent Fulgham]

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.

[Brent Fulgham]

Committed r230084: <https://trac.webkit.org/changeset/230084>