Bug 167489 – [WebRTC] Add support for libwebrtc data channel

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

[WebRTC] Add support for libwebrtc data channel

[youenn fablet]

Created attachment 299945 [details]
Patch

[Alex Christensen]

Comment on attachment 299945 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        ntroducing LibWebRTCDataChannelHandler as the integration layer between WebCore (RTCDataChannel)

Introducing

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:-68
> -    m_handler->setClient(this);

Does moving this accomplish anything?

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:52
> +    return m_channel->Send({rtc::CopyOnWriteBuffer(text.utf8().data(), text.length()), false});

The CString returned by utf8() goes out of scope immediately.  Will that cause a problem with the lifetime of CopyOnWriteBuffer?  Will it ever read after the CString has been freed?

> Source/WebCore/platform/mediastream/RTCDataChannelHandlerClient.h:34
> +class RTCDataChannelHandlerClient : public RefCounted<RTCDataChannelHandlerClient> {

This should be ThreadSafeRefCounted because we are calling ref and deref from different threads.

[youenn fablet]

Created attachment 299980 [details]
Patch

[youenn fablet]

Created attachment 299981 [details]
Patch

[youenn fablet]

Thanks for the review.

> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:-68
> > -    m_handler->setClient(this);
> 
> Does moving this accomplish anything?

The call to setClient triggers taking a ref of the channel at libwebrtc stack.
If done within constructor, it will happen before the pointer being adopted.

> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:52
> > +    return m_channel->Send({rtc::CopyOnWriteBuffer(text.utf8().data(), text.length()), false});
> 
> The CString returned by utf8() goes out of scope immediately.  Will that
> cause a problem with the lifetime of CopyOnWriteBuffer?  Will it ever read
> after the CString has been freed?

CopyOnWrite is doing its own copy of the data.

> > Source/WebCore/platform/mediastream/RTCDataChannelHandlerClient.h:34
> > +class RTCDataChannelHandlerClient : public RefCounted<RTCDataChannelHandlerClient> {
> 
> This should be ThreadSafeRefCounted because we are calling ref and deref
> from different threads.

Right!

[Radar WebKit Bug Importer]

<rdar://problem/30245427>

[WebKit Commit Bot]

Comment on attachment 299981 [details]
Patch

Clearing flags on attachment: 299981

Committed r211371: <http://trac.webkit.org/changeset/211371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.