WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 124049
Checking for TypeError in RTCPeerConnection object creation
https://bugs.webkit.org/show_bug.cgi?id=124049
Summary
Checking for TypeError in RTCPeerConnection object creation
Thiago de Barros Lacerda
Reported
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.
Attachments
Patch
(35.72 KB, patch)
2013-11-08 10:07 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Patch
(42.03 KB, patch)
2013-11-08 11:56 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Patch
(46.41 KB, patch)
2013-11-08 15:06 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Thiago de Barros Lacerda
Comment 1
2013-11-08 10:07:42 PST
Created
attachment 216406
[details]
Patch
Thiago de Barros Lacerda
Comment 2
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?
Eric Carlson
Comment 3
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.
Thiago de Barros Lacerda
Comment 4
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.
Thiago de Barros Lacerda
Comment 5
2013-11-08 11:56:04 PST
Created
attachment 216420
[details]
Patch
Eric Carlson
Comment 6
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?
Eric Carlson
Comment 7
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)"
Eric Carlson
Comment 8
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
Thiago de Barros Lacerda
Comment 9
2013-11-08 15:06:57 PST
Created
attachment 216446
[details]
Patch
WebKit Commit Bot
Comment 10
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
>
WebKit Commit Bot
Comment 11
2013-11-08 15:34:37 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug