Bug 167489 - [WebRTC] Add support for libwebrtc data channel
Summary: [WebRTC] Add support for libwebrtc data channel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 143211
  Show dependency treegraph
 
Reported: 2017-01-26 21:46 PST by youenn fablet
Modified: 2017-01-30 09:38 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.50 KB, patch)
2017-01-27 11:03 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (21.52 KB, patch)
2017-01-27 16:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (21.52 KB, patch)
2017-01-27 16:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-01-26 21:46:54 PST
[WebRTC] Add support for libwebrtc data channel
Comment 1 youenn fablet 2017-01-27 11:03:37 PST
Created attachment 299945 [details]
Patch
Comment 2 Alex Christensen 2017-01-27 12:18:45 PST
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.
Comment 3 youenn fablet 2017-01-27 16:44:08 PST
Created attachment 299980 [details]
Patch
Comment 4 youenn fablet 2017-01-27 16:44:42 PST
Created attachment 299981 [details]
Patch
Comment 5 youenn fablet 2017-01-27 16:48:44 PST
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!
Comment 6 Radar WebKit Bug Importer 2017-01-27 17:41:43 PST
<rdar://problem/30245427>
Comment 7 WebKit Commit Bot 2017-01-30 09:38:06 PST
Comment on attachment 299981 [details]
Patch

Clearing flags on attachment: 299981

Committed r211371: <http://trac.webkit.org/changeset/211371>
Comment 8 WebKit Commit Bot 2017-01-30 09:38:10 PST
All reviewed patches have been landed.  Closing bug.