[webkit-reviews] review granted: [Bug 173739] LibWebRTCSocketClient should not destroy its socket within signalClose callback : [Attachment 313666] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 173739: LibWebRTCSocketClient should not destroy its socket within
signalClose callback
https://bugs.webkit.org/show_bug.cgi?id=173739

Attachment 313666: Patch

https://bugs.webkit.org/attachment.cgi?id=313666&action=review




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 313666
  --> https://bugs.webkit.org/attachment.cgi?id=313666
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.