Bug 124049 – Checking for TypeError in RTCPeerConnection object creation

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

[Thiago de Barros Lacerda]

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.

[Thiago de Barros Lacerda]

Created attachment 216406 [details]
Patch

[Thiago de Barros Lacerda]

(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?

[Eric Carlson]

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.

[Thiago de Barros Lacerda]

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

[Thiago de Barros Lacerda]

Created attachment 216420 [details]
Patch

[Eric Carlson]

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?

[Eric Carlson]

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)"

[Eric Carlson]

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

[Thiago de Barros Lacerda]

Created attachment 216446 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 216446 [details]
Patch

Clearing flags on attachment: 216446

Committed r158964: <http://trac.webkit.org/changeset/158964>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.