LibWebRTCSocketClient should not destroy its socket within signalClose callback
rdar://problem/32922877
Created attachment 313666 [details] Patch
Comment on attachment 313666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313666&action=review > Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:141 > + m_rtcProvider.callOnRTCNetworkThread([socket = m_rtcProvider.takeSocket(m_identifier)] { }); How do we know this is sufficient lifespan? I don't think that anything really guarantees that the RTCNetworkThread doesn't wake up and do the destruction before the caller of LibWebRTCSocketClient::signalClose() has a chance to finish using the socket. It seems like the caller needs some way to keep 'socket' alive for as long as it needs to use it.
Comment on attachment 313666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313666&action=review r=me >> Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:141 >> + m_rtcProvider.callOnRTCNetworkThread([socket = m_rtcProvider.takeSocket(m_identifier)] { }); > > How do we know this is sufficient lifespan? I don't think that anything really guarantees that the RTCNetworkThread doesn't wake up and do the destruction before the caller of LibWebRTCSocketClient::signalClose() has a chance to finish using the socket. It seems like the caller needs some way to keep 'socket' alive for as long as it needs to use it. Looking further, I guess the only caller of LibWebRTCSocketClient::signalClose is the WebRTCSocket message handler, which calls this method and doesn't do anything else. We might have a number of queued up messages to be handled that need socket, but by putting the destruction in the message queue, we guarantee that those tasks are done by the time we are allowed to destroy the socket. So I'm confident this change is correct.
(In reply to Brent Fulgham from comment #4) > Comment on attachment 313666 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313666&action=review > > r=me > > >> Source/WebKit2/NetworkProcess/webrtc/LibWebRTCSocketClient.cpp:141 > >> + m_rtcProvider.callOnRTCNetworkThread([socket = m_rtcProvider.takeSocket(m_identifier)] { }); > > > > How do we know this is sufficient lifespan? I don't think that anything really guarantees that the RTCNetworkThread doesn't wake up and do the destruction before the caller of LibWebRTCSocketClient::signalClose() has a chance to finish using the socket. It seems like the caller needs some way to keep 'socket' alive for as long as it needs to use it. > > Looking further, I guess the only caller of > LibWebRTCSocketClient::signalClose is the WebRTCSocket message handler, > which calls this method and doesn't do anything else. > > We might have a number of queued up messages to be handled that need socket, > but by putting the destruction in the message queue, we guarantee that those > tasks are done by the time we are allowed to destroy the socket. > > So I'm confident this change is correct. Right, maybe I should beef up the change log?
Comment on attachment 313666 [details] Patch Clearing flags on attachment: 313666 Committed r218759: <http://trac.webkit.org/changeset/218759>
All reviewed patches have been landed. Closing bug.