Unknown media type in 1st parameter of mozGetUserMedia() call should throw a NS_ERROR_DOM_NOT_SUPPORTED_ERR

RESOLVED DUPLICATE of bug 1161433

Status

()

P4
normal
Rank:
45
RESOLVED DUPLICATE of bug 1161433
7 years ago
4 years ago

People

(Reporter: jsmith, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 642256 [details]
Misspelled (on purpose) call to mozGetUserMedia

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

7 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

7 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

7 years ago
Whiteboard: [getUserMedia], [blocking-gum+]
{} 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.
QA Contact: junky.argonaut
Assignee: nobody → junky.argonaut
QA Contact: junky.argonaut → jsmith
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.
Created attachment 656429 [details] [diff] [review]
Patchv0

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 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)
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.
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?
Created attachment 658187 [details] [diff] [review]
Patchv1

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

7 years ago
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum-]
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)
(Reporter)

Updated

6 years ago
Duplicate of this bug: 815582
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
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.
Created attachment 687029 [details] [diff] [review]
mochitest v1

Mochitest from bug 815582 which demonstrates that we do not throw an unsupported error.
(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
jib - is this bug still relevant?
backlog: --- → webRTC+
Rank: 45
Flags: needinfo?(Ms2ger) → needinfo?(jib)
Priority: -- → P4
QA Contact: jsmith
Whiteboard: [getUserMedia], [blocking-gum-]
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
Last Resolved: 4 years ago
Flags: needinfo?(jib)
Resolution: --- → DUPLICATE
Duplicate of bug: 1161433
You need to log in before you can comment on or make changes to this bug.