Bug 158940 – WebRTC: Add support for RTCPeerConnection legacy MediaStream-based API

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

[Adam Bergkvist]

The MediaStream-based API on RTCPeerConnection (addStream, removeStream, getLocalStreams, getRemoteStreams, getStreamById) has been removed from the WebRTC 1.0 specification, but is widely implemented and used. We should be able to support it with JS built-ins and some hooks down in native code.

[Adam Bergkvist]

Created attachment 281658 [details]
Proposed patch

[Adam Bergkvist]

(In reply to comment #1)
> Created attachment 281658 [details]
> Proposed patch

This won't build until the JS constructor is in place (http://webkit.org/b/158832).

[youenn fablet]

Comment on attachment 281658 [details]
Proposed patch

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

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:57
> +    return [e-mail]();

Array.slice may be corrupted by user scripts.
The same applies to find findIndex, push, forEach, splice methods.
This should be robustified.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:94
> +    stream.getTracks().forEach(track => this.@addTrack(track, stream));

Potentially, we know that this, track and stream are of the right type.
So we could replace if statements by assert statements in the binding generated code.
That said, this might be too early to do so.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:87
> +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().MediaStreamPrivateName(), JSMediaStream::getConstructor(vm, this), DontDelete | ReadOnly),

It might be good to add a keyword similar to PrivateAlso for exposing DOM constructors safely to JS builtins.
Ideally, we should not need to create the constructor but just pass a getter function that would create it if needed

[Adam Bergkvist]

(In reply to comment #3)
> Comment on attachment 281658 [details]
> Proposed patch

Thanks for reviewing Youenn.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281658&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:57
> > +    return [e-mail]();
> 
> Array.slice may be corrupted by user scripts.
> The same applies to find findIndex, push, forEach, splice methods.
> This should be robustified.

Looking at the array prototype implementation, we seem to have a private name for push, but the rest needs fixing. New bug: [1].

[1] http://webkit.org/b/158978

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:94
> > +    stream.getTracks().forEach(track => this.@addTrack(track, stream));
> 
> Potentially, we know that this, track and stream are of the right type.
> So we could replace if statements by assert statements in the binding
> generated code.
> That said, this might be too early to do so.

MediaStream.getTracks is actually part of the public API so we need a safe way to call it. New bug [2].

[2] webkit.org/b/158979

I agree that assertions would be sufficient for [PrivateOnly] bindings. The bug [3] suggests that one might want to use the type checking of a [PrivateOnly] function as part of a public API call, but that might be unlikely now when we have [PrivateAlso]. I wouldn't mind closing [3] as a WONTFIX.

[3] webkit.org/b/158778

> 
> > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:87
> > +        JSDOMGlobalObject::GlobalPropertyInfo(clientData.builtinNames().MediaStreamPrivateName(), JSMediaStream::getConstructor(vm, this), DontDelete | ReadOnly),
> 
> It might be good to add a keyword similar to PrivateAlso for exposing DOM
> constructors safely to JS builtins.
> Ideally, we should not need to create the constructor but just pass a getter
> function that would create it if needed

That would be useful indeed.

[Adam Bergkvist]

Clickable links for the previous post

[2] http://webkit.org/b/158979
[3] http://webkit.org/b/158778

[Adam Bergkvist]

*** Bug 158979 has been marked as a duplicate of this bug. ***

[Adam Bergkvist]

Created attachment 281899 [details]
Updated patch

[Adam Bergkvist]

(In reply to comment #4)
> MediaStream.getTracks is actually part of the public API so we need a safe
> way to call it. New bug [2].
> 
> [2] webkit.org/b/158979

This is included in the updated patch (closed bug:158979).

[Adam Bergkvist]

Comment on attachment 281899 [details]
Updated patch

Thank you Eric and Youenn for reviewing

[WebKit Commit Bot]

Comment on attachment 281899 [details]
Updated patch

Clearing flags on attachment: 281899

Committed r202376: <http://trac.webkit.org/changeset/202376>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.