Bug 204460 – Null check callback in NetworkConnectionToWebProcess::didDeliverMessagePortMessages

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Null check callback in NetworkConnectionToWebProcess::didDeliverMessagePortMessages

[Alex Christensen]

Created attachment 384072 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 384072 [details]
Patch

Do we understand why the callback can be null here?

[Chris Dumez]

Comment on attachment 384072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384072&action=review

> Source/WebKit/ChangeLog:5
> +        <rdar://57448618>

This does not look like a valid radar number?

[Chris Dumez]

Comment on attachment 384072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384072&action=review

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:992
> +    if (auto callback = m_messageBatchDeliveryCompletionHandlers.take(messageBatchIdentifier))

This should not be possible in theory, I think we need to find the root cause.

m_messageBatchDeliveryCompletionHandlers is only modified here and in takeAllMessagesForPort(). So either we're getting an unexpected didDeliverMessagePortMessages or the NetworkConnectionToWebProcess is stale.

[Chris Dumez]

Comment on attachment 384072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384072&action=review

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:992
>> +    if (auto callback = m_messageBatchDeliveryCompletionHandlers.take(messageBatchIdentifier))
> 
> This should not be possible in theory, I think we need to find the root cause.
> 
> m_messageBatchDeliveryCompletionHandlers is only modified here and in takeAllMessagesForPort(). So either we're getting an unexpected didDeliverMessagePortMessages or the NetworkConnectionToWebProcess is stale.

Maybe if the network process crashes in between the takeAllMessagesForPort() call and the didDeliverMessagePortMessages() one? Then it'd be a new NetworkConnectionToWebProcess. The WebProcess would send the DidDeliverMessagePortMessages IPC to the new network process, which does not know about the takeAllMessagesForPort request.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 384072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384072&action=review
> 
> >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:992
> >> +    if (auto callback = m_messageBatchDeliveryCompletionHandlers.take(messageBatchIdentifier))
> > 
> > This should not be possible in theory, I think we need to find the root cause.
> > 
> > m_messageBatchDeliveryCompletionHandlers is only modified here and in takeAllMessagesForPort(). So either we're getting an unexpected didDeliverMessagePortMessages or the NetworkConnectionToWebProcess is stale.
> 
> Maybe if the network process crashes in between the takeAllMessagesForPort()
> call and the didDeliverMessagePortMessages() one? Then it'd be a new
> NetworkConnectionToWebProcess. The WebProcess would send the
> DidDeliverMessagePortMessages IPC to the new network process, which does not
> know about the takeAllMessagesForPort request.

If that's what's doing on, it may be possible to write a test for this.

[Alex Christensen]

Comment on attachment 384072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384072&action=review

>> Source/WebKit/ChangeLog:5
>> +        <rdar://57448618>
> 
> This does not look like a valid radar number?

I have no idea where that came from.  Should be rdar://problem/57348618 and will update.

>>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:992
>>>> +    if (auto callback = m_messageBatchDeliveryCompletionHandlers.take(messageBatchIdentifier))
>>> 
>>> This should not be possible in theory, I think we need to find the root cause.
>>> 
>>> m_messageBatchDeliveryCompletionHandlers is only modified here and in takeAllMessagesForPort(). So either we're getting an unexpected didDeliverMessagePortMessages or the NetworkConnectionToWebProcess is stale.
>> 
>> Maybe if the network process crashes in between the takeAllMessagesForPort() call and the didDeliverMessagePortMessages() one? Then it'd be a new NetworkConnectionToWebProcess. The WebProcess would send the DidDeliverMessagePortMessages IPC to the new network process, which does not know about the takeAllMessagesForPort request.
> 
> If that's what's doing on, it may be possible to write a test for this.

I think that's what's going on and that was mentioned in https://bugs.webkit.org/show_bug.cgi?id=201299#c4 , but there doesn't seem to be a good way to call _terminateNetworkProcess exactly between these two calls.

[Chris Dumez]

Comment on attachment 384072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384072&action=review

>>>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:992
>>>>> +    if (auto callback = m_messageBatchDeliveryCompletionHandlers.take(messageBatchIdentifier))
>>>> 
>>>> This should not be possible in theory, I think we need to find the root cause.
>>>> 
>>>> m_messageBatchDeliveryCompletionHandlers is only modified here and in takeAllMessagesForPort(). So either we're getting an unexpected didDeliverMessagePortMessages or the NetworkConnectionToWebProcess is stale.
>>> 
>>> Maybe if the network process crashes in between the takeAllMessagesForPort() call and the didDeliverMessagePortMessages() one? Then it'd be a new NetworkConnectionToWebProcess. The WebProcess would send the DidDeliverMessagePortMessages IPC to the new network process, which does not know about the takeAllMessagesForPort request.
>> 
>> If that's what's doing on, it may be possible to write a test for this.
> 
> I think that's what's going on and that was mentioned in https://bugs.webkit.org/show_bug.cgi?id=201299#c4 , but there doesn't seem to be a good way to call _terminateNetworkProcess exactly between these two calls.

Then the ChangeLog should state so. Also maybe a comment in the code indicating why this can happen.

[Alex Christensen]

Created attachment 384185 [details]
Patch

[Chris Dumez]

Comment on attachment 384185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384185&action=review

> Source/WebKit/ChangeLog:10
> +        a web process may have an identifier that becomes stale.  In this case, we should not crsah again.

Typo: crash

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:1001
> +        ASSERT_NOT_REACHED();

Since we know this can happen in practice and we have a valid explanation for it, I do not think we should ASSERT.

[Alex Christensen]

Created attachment 384201 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r252809

[Radar WebKit Bug Importer]

<rdar://problem/57442544>