Bug 158940

Created attachment 281658 [details] Proposed patch

(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).

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 this.@localStreams.slice(); 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

(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 this.@localStreams.slice(); > > 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.

Clickable links for the previous post [2] http://webkit.org/b/158979 [3] http://webkit.org/b/158778

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

Created attachment 281899 [details] Updated patch

(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).

Comment on attachment 281899 [details] Updated patch Thank you Eric and Youenn for reviewing

Comment on attachment 281899 [details] Updated patch Clearing flags on attachment: 281899 Committed r202376: <http://trac.webkit.org/changeset/202376>

All reviewed patches have been landed. Closing bug.