Null check callback in NetworkConnectionToWebProcess::didDeliverMessagePortMessages
Created attachment 384072 [details] Patch
Comment on attachment 384072 [details] Patch Do we understand why the callback can be null here?
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?
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.
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.
(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.
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.
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.
Created attachment 384185 [details] Patch
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.
Created attachment 384201 [details] Patch
http://trac.webkit.org/r252809
<rdar://problem/57442544>