Bug 173739 – LibWebRTCSocketClient should not destroy its socket within signalClose callback

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[youenn fablet]

LibWebRTCSocketClient should not destroy its socket within signalClose callback

[youenn fablet]

rdar://problem/32922877

[youenn fablet]

Created attachment 313666 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 313666 [details]
Patch

Clearing flags on attachment: 313666

Committed r218759: <http://trac.webkit.org/changeset/218759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.