Bug 151303 - WebRTC: Check type of this in RTCPeerConnection JS built-in functions
Summary: WebRTC: Check type of this in RTCPeerConnection JS built-in functions
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: Nobody
URL:
Keywords:
Depends on:
Blocks: 143211
  Show dependency treegraph
 
Reported: 2015-11-16 02:00 PST by Adam Bergkvist
Modified: 2016-06-16 23:23 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (8.74 KB, patch)
2016-06-15 13:45 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (12.43 KB, patch)
2016-06-16 01:33 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Bergkvist 2015-11-16 02:00:57 PST
The JS built-in functions should throw if this isn't of type RTCPeerConnection.
Comment 1 Adam Bergkvist 2016-06-15 13:45:19 PDT
Created attachment 281386 [details]
Proposed patch
Comment 2 Eric Carlson 2016-06-15 14:06:32 PDT
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"
Comment 3 youenn fablet 2016-06-15 14:32:04 PDT
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.
Comment 4 Adam Bergkvist 2016-06-16 00:17:24 PDT
(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.
Comment 5 youenn fablet 2016-06-16 01:01:06 PDT
(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(...)
Comment 6 Adam Bergkvist 2016-06-16 01:15:17 PDT
(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.
Comment 7 Adam Bergkvist 2016-06-16 01:33:14 PDT
Created attachment 281448 [details]
Updated patch
Comment 8 Adam Bergkvist 2016-06-16 01:34:58 PDT
(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
Comment 9 Adam Bergkvist 2016-06-16 01:47:04 PDT
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
Comment 10 youenn fablet 2016-06-16 08:10:04 PDT
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?
Comment 11 Adam Bergkvist 2016-06-16 10:50:59 PDT
(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.
Comment 12 Adam Bergkvist 2016-06-16 10:52:00 PDT
Comment on attachment 281448 [details]
Updated patch

Thanks for reviewing Youenn and Eric!
Comment 13 WebKit Commit Bot 2016-06-16 11:12:41 PDT
Comment on attachment 281448 [details]
Updated patch

Clearing flags on attachment: 281448

Committed r202130: <http://trac.webkit.org/changeset/202130>
Comment 14 WebKit Commit Bot 2016-06-16 11:12:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alex. Gouaillard 2016-06-16 22:38:43 PDT
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.
Comment 16 Adam Bergkvist 2016-06-16 23:23:41 PDT
(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!