Closed
Bug 773986
Opened 13 years ago
Closed 10 years ago
Unknown media type in 1st parameter of mozGetUserMedia() call should throw a NS_ERROR_DOM_NOT_SUPPORTED_ERR
Categories
(Core :: WebRTC, defect, P4)
Core
WebRTC
Tracking
()
RESOLVED
DUPLICATE
of bug 1161433
backlog | webrtc/webaudio+ |
People
(Reporter: jsmith, Unassigned)
References
Details
Attachments
(3 files, 1 obsolete file)
946 bytes,
text/html
|
Details | |
8.97 KB,
patch
|
Ms2ger
:
feedback-
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
Details | Diff | Splinter Review |
Steps:
Sample code used is attached. Important part of it is below:
navigator.mozGetUserMedia({Video: true}, gotStream, noStream);
I purposely capitalized incorrectly to Video, instead of video, to see if I get an error callback (or something in the error console) indicating that the 1st parameter was incorrect.
Expected:
I'd expect an error callback to made to include an error indicating that the 1st parameter is incorrect. An error code is also sufficient.
Actual:
No error callback is made. Nothing shows up in the error console.
Reporter | ||
Comment 1•13 years ago
|
||
Checking the getUserMedia specification in http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastream, a SyntaxError should have been thrown here. No error being thrown seems like a bug then.
Reporter | ||
Comment 2•13 years ago
|
||
Looks like generally we aren't doing good on error conditions on the API. Just tried {} - no error callback fired as a result.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+]
Comment 3•13 years ago
|
||
{} is valid input, both audio and video default to false, which means no media is requested.
{Video:true} is not valid input however, and we should pass a NOT_SUPPORTED_ERR to the errorCallback.
BTW you should be looking at http://dev.w3.org/2011/webrtc/editor/getusermedia.html#navigatorusermedia, not spec for the MediaStream constructor - that's a completely different thing.
Updated•12 years ago
|
QA Contact: junky.argonaut
Updated•12 years ago
|
Assignee: nobody → junky.argonaut
QA Contact: junky.argonaut → jsmith
Comment 4•12 years ago
|
||
All of the code in http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#355 simply returns an nsresult indicating an error if anything goes wrong. You'll need to explicitly invoke the error callback instead.
Comment 5•12 years ago
|
||
Sorry about the delay.
I havn't changed nsIDOMNavigatorUserMedia.idl and MediaManager.h
Could you tell me if this is the right way to solve the issue in MediaManager.cpp?
Attachment #656429 -
Flags: feedback?(josh)
Attachment #656429 -
Flags: feedback?(anant)
Comment 6•12 years ago
|
||
Comment on attachment 656429 [details] [diff] [review]
Patchv0
Ok, there are a few things I want to point out in this patch that should be done differently.
* You'll want to add the [implicit_jscontext] annotation to the IDL definition at http://mxr.mozilla.org/mozilla-central/source/dom/media/nsIDOMNavigatorUserMedia.idl#36; this will make the mozGetUserMedia function take a JSContext* argument which you can pass to GetUserMedia and avoid the whole ScriptGlobalObject dance you're doing.
* The patch doesn't correctly deal with turning the strings into ids right now. It also won't build :) You should use the code at http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#501 as your model: you want to have an array of the properties of the object you're examining (eg. you'll have an array of ["foo", "bar", "video"]), and check whether each element in that array is present in your array of expected names. You'll want to get an nsDependentJSString of the property name, and you can compare that to your nsStrings.
Once you make those changes, I think we'll be in a better position to comment on any remaining issues.
Attachment #656429 -
Flags: feedback?(josh)
Comment 7•12 years ago
|
||
This is handled by WebIDL. No error callback should be called, and no exception should be thrown. If you want something in the error console, that should be handled in the binding code.
Comment 8•12 years ago
|
||
I assume to leverage WebIDL here, we'd have to move it to WebIDL from xpidl?
Also please define 'handled'. If "handled" means "ignored", well then it already does that. :-) And depending on the spec (dictionary vs object, etc), that might be correct. Anant?
Comment 9•12 years ago
|
||
When this patch is applied Nightly crashes on running the attached test case.
Could you guys tell me why that happens and help me solve this problem?
Attachment #656429 -
Attachment is obsolete: true
Attachment #656429 -
Flags: feedback?(anant)
Attachment #658187 -
Flags: feedback?(anant)
Attachment #658187 -
Flags: feedback?(Ms2ger)
Comment 10•12 years ago
|
||
Comment on attachment 658187 [details] [diff] [review]
Patchv1
Review of attachment 658187 [details] [diff] [review]:
-----------------------------------------------------------------
[scriptable, uuid(8a26205e-e8f7-4372-bd15-97ff982b4c1c)]
interface nsIMediaStreamOptions : nsISupports
{
readonly attribute boolean audio;
readonly attribute boolean video;
readonly attribute boolean picture;
readonly attribute DOMString camera;
};
should be
dictionary MediaStreamOptions
{
boolean audio;
boolean video;
boolean picture;
DOMString camera;
};
and you should add it to <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dictionary_helper_gen.conf>, so you can do something like <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMMutationObserver.cpp#533>. In any case, you shouldn't be enumerating the properties.
Attachment #658187 -
Flags: feedback?(Ms2ger) → feedback-
Comment 11•12 years ago
|
||
We want the first parameter to be a jsval, not an object or dictionary. The reason is that the values can be any track type, not just audio, video or camera. That's the reason for enumerating the properties, but we should be enumerating a jsval instead of nsIMediaStreamOptions.
nsIMediaStreamOptions is a hack I used to get something quick up and running, let's get rid of it :)
Comment 12•12 years ago
|
||
(In reply to Anant Narayanan [:anant] from comment #11)
> We want the first parameter to be a jsval, not an object or dictionary. The
> reason is that the values can be any track type, not just audio, video or
> camera. That's the reason for enumerating the properties, but we should be
> enumerating a jsval instead of nsIMediaStreamOptions.
Don't understand a word of what you're saying here.
Comment 13•12 years ago
|
||
navigator.mozGetUserMedia({foo:true, bar:true}); is valid input. We need to enumerate the JS object first and then recognize the "track" types we support, where each object property is a track type. Currently, we only support "video", "audio" and "camera", but we may add some later.
The algorithm in the spec requires us to enumerate each property of the first parameter and invoke the error callback once we encounter a track type that we don't support.
In terms of what that means for this patch, nsIMediaStreamOptions should be removed from the IDL. I'm also arguing that enumerating the properties of the jsval is desired behaviour.
Comment 14•12 years ago
|
||
(In reply to Anant Narayanan [:anant] from comment #13)
> navigator.mozGetUserMedia({foo:true, bar:true}); is valid input. We need to
> enumerate the JS object first and then recognize the "track" types we
> support, where each object property is a track type. Currently, we only
> support "video", "audio" and "camera", but we may add some later.
>
> The algorithm in the spec requires us to enumerate each property of the
> first parameter and invoke the error callback once we encounter a track type
> that we don't support.
>
> In terms of what that means for this patch, nsIMediaStreamOptions should be
> removed from the IDL. I'm also arguing that enumerating the properties of
> the jsval is desired behaviour.
Then the algorithm in the spec is wrong. This was all discussed to death on public-script-coord, but apparently the people in webrtc prefer sitting in their own silo and ignoring what happens elsewhere in the platform.
Comment 15•12 years ago
|
||
The goal is to be flexible enough that we can allow new track types without having to change the spec. Could you link to the relevant discussion (or give me a rough time-frame so I can search) on public-script-coord? The WebRTC spec is far from done, we can definitely still make changes.
Comment 16•12 years ago
|
||
Fixed the crash thanks to :jdm. Not a problem with the patch, I hadn't built toolkit/library.
Have to work on what Ms2ger has commented on now.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum-]
Comment 17•12 years ago
|
||
Comment on attachment 658187 [details] [diff] [review]
Patchv1
Clearing flag, the patch has bitrotted and we need to resolve the dictionary vs object issue anyway.
Attachment #658187 -
Flags: feedback?(anant)
Comment 19•12 years ago
|
||
The summary here is very vague! Lets get it updated so it can be found by querying Bugzilla. I have already attached a mochitest update for a possible fix on bug 815582, but I'm not sure about the implementation details given the non-finished discussion on this bug. Once it's clear I can attach an updated fix for the test, and hope we can get it fixed in the code soon.
(In reply to :Ms2ger from comment #14)
> Then the algorithm in the spec is wrong. This was all discussed to death on
> public-script-coord, but apparently the people in webrtc prefer sitting in
> their own silo and ignoring what happens elsewhere in the platform.
It would be great to know how we have to handle that. Ms2ger, mind giving us the requested information? Thanks.
Flags: needinfo?(Ms2ger)
Summary: Incorrect 1st parameter to mozGetUserMedia results in no error callback, nothing in the Error Console either → Unknown media type in 1st parameter of mozGetUserMedia() call should throw a NS_ERROR_DOM_NOT_SUPPORTED_ERR
Comment 20•12 years ago
|
||
Most dictionary consumers expect that extra properties would be ignored as WebIDL currently spec'ed. The semantic of MediaStreamOptions contradicts their expectations.
Maybe a new extended attribute need to be added to the WebIDL spec to satisfy WebRTC's demand. Until then, it would be fine to operate on jsval directly.
Comment 21•12 years ago
|
||
Mochitest from bug 815582 which demonstrates that we do not throw an unsupported error.
Comment 22•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #19)
It doesn't look like that :infinity is still working on this bug. Putting it back into the pool.
> (In reply to :Ms2ger from comment #14)
> > Then the algorithm in the spec is wrong. This was all discussed to death on
> > public-script-coord, but apparently the people in webrtc prefer sitting in
> > their own silo and ignoring what happens elsewhere in the platform.
>
> It would be great to know how we have to handle that. Ms2ger, mind giving us
> the requested information? Thanks.
Ms2ger, would you mind to give us the requested information?
Assignee: junky.argonaut → nobody
Comment 23•10 years ago
|
||
jib - is this bug still relevant?
backlog: --- → webRTC+
Rank: 45
Flags: needinfo?(Ms2ger) → needinfo?(jib)
Priority: -- → P4
QA Contact: jsmith
Whiteboard: [getUserMedia], [blocking-gum-]
Comment 24•10 years ago
|
||
OBE. Unknown dictionary members correctly get stripped by WebIDL binding code, which has worked for a long time. NotSupportedError is only thrown if this results in an empty dictionary.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(jib)
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•