Bug 124049

Summary: Checking for TypeError in RTCPeerConnection object creation
Product: WebKit Reporter: Thiago de Barros Lacerda <thiago.lacerda>
Component: WebCore Misc.Assignee: Thiago de Barros Lacerda <thiago.lacerda>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, hta, jer.noble, kondapallykalyan, rakuco, tommyw
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 124288    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Thiago de Barros Lacerda 2013-11-08 10:02:50 PST
If invalid parameters are passed on RTCPeerConnection creation we must throw a TypeError exception.
According to the spec it requires a Dictionary argument, the RTCConfiguration, which is mandatory.
Comment 1 Thiago de Barros Lacerda 2013-11-08 10:07:42 PST
Created attachment 216406 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2013-11-08 10:08:33 PST
(In reply to comment #1)
> Created an attachment (id=216406) [details]
> Patch

Eric, could you please help me to add it to xcode's build files?
Comment 3 Eric Carlson 2013-11-08 10:55:48 PST
Comment on attachment 216406 [details]
Patch

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

I will email you a patch to add the new file to the Xcode project.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:36
> +    CustomConstructor,
> +    CustomConstructor(Dictionary rtcIceServers, optional Dictionary mediaConstraints),

I don't think you need both of these.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:-36
> -    ConstructorCallWith=ScriptExecutionContext,

Why remove this when we still require the ScriptExecutionContext?

> LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:19
>  PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'}]}, null); did not throw exception.
>  PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'},{url:'stun:bar.com'}]}, null); did not throw exception.

We should also check that "username" is allowed.

> LayoutTests/fast/mediastream/RTCPeerConnection.html:13
>  <script>
>  description("Tests the RTCPeerConnection constructor.");
>  
> -shouldNotThrow("new webkitRTCPeerConnection(null);");
> -shouldNotThrow("new webkitRTCPeerConnection(null, null);");
> -shouldNotThrow("new webkitRTCPeerConnection(undefined);");
> -shouldNotThrow("new webkitRTCPeerConnection(undefined, undefined);");
> +shouldThrow("new webkitRTCPeerConnection(null);");
> +shouldThrow("new webkitRTCPeerConnection(null, null);");
> +shouldThrow("new webkitRTCPeerConnection(undefined);");
> +shouldThrow("new webkitRTCPeerConnection(undefined, undefined);");

Bonus points if you add some indentation to make this more readable.
Comment 4 Thiago de Barros Lacerda 2013-11-08 11:21:49 PST
(In reply to comment #3)
> (From update of attachment 216406 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216406&action=review
> 
> I will email you a patch to add the new file to the Xcode project.

Thanks

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:36
> > +    CustomConstructor,
> > +    CustomConstructor(Dictionary rtcIceServers, optional Dictionary mediaConstraints),
> 
> I don't think you need both of these.

OK, removing the first one.

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:-36
> > -    ConstructorCallWith=ScriptExecutionContext,
> 
> Why remove this when we still require the ScriptExecutionContext?

I was following the how other CustomConstructors do. And all CustomConstructors, do not have it. But I can put it back if there is no harm.

> 
> > LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:19
> >  PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'}]}, null); did not throw exception.
> >  PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'},{url:'stun:bar.com'}]}, null); did not throw exception.
> 
> We should also check that "username" is allowed.

True story.

> 
> > LayoutTests/fast/mediastream/RTCPeerConnection.html:13
> >  <script>
> >  description("Tests the RTCPeerConnection constructor.");
> >  
> > -shouldNotThrow("new webkitRTCPeerConnection(null);");
> > -shouldNotThrow("new webkitRTCPeerConnection(null, null);");
> > -shouldNotThrow("new webkitRTCPeerConnection(undefined);");
> > -shouldNotThrow("new webkitRTCPeerConnection(undefined, undefined);");
> > +shouldThrow("new webkitRTCPeerConnection(null);");
> > +shouldThrow("new webkitRTCPeerConnection(null, null);");
> > +shouldThrow("new webkitRTCPeerConnection(undefined);");
> > +shouldThrow("new webkitRTCPeerConnection(undefined, undefined);");
> 
> Bonus points if you add some indentation to make this more readable.

I think all indentation could be in a separated commit, so we won't get confused by some git blame, because there will be an explanation on the commit that it was just indenting.
Comment 5 Thiago de Barros Lacerda 2013-11-08 11:56:04 PST
Created attachment 216420 [details]
Patch
Comment 6 Eric Carlson 2013-11-08 12:27:52 PST
Comment on attachment 216420 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        * GNUmakefile.list.am:
> +        * Modules/mediastream/RTCPeerConnection.idl:
> +        * UseJSC.cmake:
> +        * WebCore.vcxproj/WebCore.vcxproj:
> +        * WebCore.vcxproj/WebCore.vcxproj.filters:
> +        * bindings/js/JSRTCPeerConnectionCustom.cpp: Added.
> +        (WebCore::JSRTCPeerConnectionConstructor::constructJSRTCPeerConnection):

You forgot to include the Xcode changes here.

> LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:15
> +PASS new webkitRTCPeerConnection({}); threw exception TypeError: Error creating RTCPeerConnection.
> +PASS new webkitRTCPeerConnection({}, ''); threw exception TypeError: Error creating RTCPeerConnection.
> +PASS new webkitRTCPeerConnection({}, null); threw exception TypeError: Error creating RTCPeerConnection.

It seems unfortunate to not have the same helpful exception error message here that we use when a non-object is passed for the Dictionary. "First argument of RTCPeerConnection must be a valid Dictionary containing a list of ice servers" is just as valid in this case. 

Could we use the same error message when "Dictionary mediaConstraints(exec, exec->argument(1))" returns a TypeError?
Comment 7 Eric Carlson 2013-11-08 12:36:39 PST
Comment on attachment 216420 [details]
Patch

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

> Source/WebCore/bindings/js/JSRTCPeerConnectionCustom.cpp:49
> +    Dictionary mediaConstraints(exec, exec->argument(1));
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());

Oops, this needs "if (exec->argumentCount() > 1)"
Comment 8 Eric Carlson 2013-11-08 12:38:04 PST
Comment on attachment 216420 [details]
Patch

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

>> LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:15
>> +PASS new webkitRTCPeerConnection({}, null); threw exception TypeError: Error creating RTCPeerConnection.
> 
> It seems unfortunate to not have the same helpful exception error message here that we use when a non-object is passed for the Dictionary. "First argument of RTCPeerConnection must be a valid Dictionary containing a list of ice servers" is just as valid in this case. 
> 
> Could we use the same error message when "Dictionary mediaConstraints(exec, exec->argument(1))" returns a TypeError?

Or rather, when "RTCPeerConnection::create()" returns a TYPE_MISMATCH_ERR
Comment 9 Thiago de Barros Lacerda 2013-11-08 15:06:57 PST
Created attachment 216446 [details]
Patch
Comment 10 WebKit Commit Bot 2013-11-08 15:34:34 PST
Comment on attachment 216446 [details]
Patch

Clearing flags on attachment: 216446

Committed r158964: <http://trac.webkit.org/changeset/158964>
Comment 11 WebKit Commit Bot 2013-11-08 15:34:37 PST
All reviewed patches have been landed.  Closing bug.