Bug 151303 – WebRTC: Check type of this in RTCPeerConnection JS built-in functions

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

[Adam Bergkvist]

The JS built-in functions should throw if this isn't of type RTCPeerConnection.

[Adam Bergkvist]

Created attachment 281386 [details]
Proposed patch

[Eric Carlson]

Comment on attachment 281386 [details]
Proposed patch

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

> LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:9
> +            description("Verify that the RTCPeerConnection JS built-in methods checks calling 'this'");

Nit: "checks" -> "check"

[youenn fablet]

Would you be able to add 2 tests:
- Changing a RTCPeerConnection object prototype, then calling one of RTCPeerConnection function on the object.
- Setting an object to the RTCPeerConnection prototype and calling one of the methods.

Ideally we should not rely on the object prototype itself.

[Adam Bergkvist]

(In reply to comment #3)
> Would you be able to add 2 tests:
> - Changing a RTCPeerConnection object prototype, then calling one of
> RTCPeerConnection function on the object.
> - Setting an object to the RTCPeerConnection prototype and calling one of
> the methods.
> 
> Ideally we should not rely on the object prototype itself.

The current @isRTCPeerConnection() cannot deal with the cases you describe Youenn. My initial thinking is to add a JSBuiltinConstructor where I can initialize the @operations field and use that one as a probe. Right now the existence for @operations is checked before every use, so a JS built-in constructor would be useful anyhow.

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #3)
> > Would you be able to add 2 tests:
> > - Changing a RTCPeerConnection object prototype, then calling one of
> > RTCPeerConnection function on the object.
> > - Setting an object to the RTCPeerConnection prototype and calling one of
> > the methods.
> > 
> > Ideally we should not rely on the object prototype itself.
> 
> The current @isRTCPeerConnection() cannot deal with the cases you describe
> Youenn. My initial thinking is to add a JSBuiltinConstructor where I can
> initialize the @operations field and use that one as a probe. Right now the
> existence for @operations is checked before every use, so a JS built-in
> constructor would be useful anyhow.

That would probably cover the cases.

In the future we might want to make checks more uniform, for instance something like:
if (!(object instanceof @RTCPeerConnection))
   throw new @TypeError(...)

[Adam Bergkvist]

(In reply to comment #5)
> In the future we might want to make checks more uniform, for instance
> something like:
> if (!(object instanceof @RTCPeerConnection))
>    throw new @TypeError(...)

That would be nice.

[Adam Bergkvist]

Created attachment 281448 [details]
Updated patch

[Adam Bergkvist]

(In reply to comment #2)
> Comment on attachment 281386 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281386&action=review
> 
> > LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:9
> > +            description("Verify that the RTCPeerConnection JS built-in methods checks calling 'this'");
> 
> Nit: "checks" -> "check"

Fixed

[Adam Bergkvist]

I would prefer to not add a JS built-in constructor in this change so I've added expected failures that can be converted to passing tests when [1] is resolved. Which in turns depend on [2].

[1] https://bugs.webkit.org/show_bug.cgi?id=158831
[2] https://bugs.webkit.org/show_bug.cgi?id=158832

[youenn fablet]

Comment on attachment 281448 [details]
Updated patch

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

> LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:52
> +            window.successfullyParsed = true;

With testharness.js, these 10 lines would no longer be needed.

Promise-based test handling by testharness.js is really nice.
I would tend to use those here. You can have a look in fetch or streams tests.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:38
> +        return @Promise.@reject(new @TypeError("Function should be called on an RTCPeerConnection"));

This message is consistent with other built-ins which is good.
But it is not with binding generated messages (something like "Can only call RTCPeerConnection.createOffer on instances of RTCPeerConnection").

Maybe we should add something like @createBadThisFunctionError taking interface and function name as input to have the same messages in both places.

Or maybe making consistent is not all that important?

[Adam Bergkvist]

(In reply to comment #10)
> Comment on attachment 281448 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281448&action=review
> 
> > LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:52
> > +            window.successfullyParsed = true;
> 
> With testharness.js, these 10 lines would no longer be needed.
> 
> Promise-based test handling by testharness.js is really nice.
> I would tend to use those here. You can have a look in fetch or streams
> tests.

I might do that and see if we can contribute tests to the W3C test suite.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:38
> > +        return @Promise.@reject(new @TypeError("Function should be called on an RTCPeerConnection"));
> 
> This message is consistent with other built-ins which is good.
> But it is not with binding generated messages (something like "Can only call
> RTCPeerConnection.createOffer on instances of RTCPeerConnection").
> 
> Maybe we should add something like @createBadThisFunctionError taking
> interface and function name as input to have the same messages in both
> places.
> 
> Or maybe making consistent is not all that important?

I think that's a good idea.

[Adam Bergkvist]

Comment on attachment 281448 [details]
Updated patch

Thanks for reviewing Youenn and Eric!

[WebKit Commit Bot]

Comment on attachment 281448 [details]
Updated patch

Clearing flags on attachment: 281448

Committed r202130: <http://trac.webkit.org/changeset/202130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex. Gouaillard]

Adam, I am one of the maintainer of the w3c webrtc tests, so you can send the test my way for commit, I have access.

[Adam Bergkvist]

(In reply to comment #15)
> Adam, I am one of the maintainer of the w3c webrtc tests, so you can send
> the test my way for commit, I have access.

Got it!