Last Comment Bug 780553 - Providing an incorrect combination of parameters (video: true, picture: true) to getUserMedia - unexpected success, picture taken
: Providing an incorrect combination of parameters (video: true, picture: true)...
Status: VERIFIED FIXED
[getUserMedia], [blocking-gum+], [qa!]
:
Product: Core
Classification: Components
Component: WebRTC (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nagarjuna Varma [:Infinity]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-05 22:35 PDT by Jason Smith [:jsmith]
Modified: 2012-11-25 22:30 PST (History)
5 users (show)
jsmith: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test Case (2.57 KB, text/html)
2012-08-05 22:35 PDT, Jason Smith [:jsmith]
no flags Details
Patchv1 (1.64 KB, patch)
2012-08-09 11:38 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Review
Patchv2 (790 bytes, patch)
2012-08-09 12:23 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Review
patchv3 (682 bytes, patch)
2012-08-09 13:44 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Review
patchv4 (696 bytes, patch)
2012-08-09 14:01 PDT, Nagarjuna Varma [:Infinity]
anant: review+
Details | Diff | Review

Description Jason Smith [:jsmith] 2012-08-05 22:35:58 PDT
Created attachment 649187 [details]
Test Case

Steps:

Load the attached test case in the browser - important part worth noting is:

navigator.mozGetUserMedia({video: true, picture: true}, gotStream, noStream);

Expected:

An error should be thrown to indicate that an incorrect combination parameters was provided to mozGetUserMedia.

Actual:

A success callback is made unexpectedly. A picture is also taken via the camera.

Additional Notes:

We need an automated API test for this. Do we have infrastructure up yet for doing API automation?
Comment 1 Jason Smith [:jsmith] 2012-08-05 22:39:59 PDT
Same problem also occurs in the combination of {picture: true, audio: true}.
Comment 2 Jason Smith [:jsmith] 2012-08-05 22:41:43 PDT
Generally appears to happen if picture: true is in the dictionary no matter what, even if incorrect params come with it.
Comment 3 Nagarjuna Varma [:Infinity] 2012-08-09 11:38:47 PDT
Created attachment 650630 [details] [diff] [review]
Patchv1

Anant, could you have a look at it and tell me if it is ok?
Comment 4 Nagarjuna Varma [:Infinity] 2012-08-09 12:23:51 PDT
Created attachment 650640 [details] [diff] [review]
Patchv2

Is this fine?
Comment 5 Anant Narayanan [:anant] 2012-08-09 13:15:13 PDT
Comment on attachment 650640 [details] [diff] [review]
Patchv2

>     if (mPicture) {
>+      if (mAudio || mVideo) {
>+        NS_DispatchToMainThread(new ErrorCallbackRunnable(
>+        mError, NS_LITERAL_STRING("NOT_SUPPORTED_ERR"), mWindowID
>+      ));
>+      }
>+      else
>+      {
>       SendPicture();
>       return NS_OK;
>+      }

Looks good! Couple of nits: the indentation is off, and I prefer early returns to keep the nesting level low.
Comment 6 Nagarjuna Varma [:Infinity] 2012-08-09 13:44:52 PDT
Created attachment 650667 [details] [diff] [review]
patchv3

Fixed the indentation and early return.
Comment 7 Nagarjuna Varma [:Infinity] 2012-08-09 14:01:24 PDT
Created attachment 650671 [details] [diff] [review]
patchv4

Havn't tested it though
This must be the highest number of reviews to code ratio...sorry about that
Comment 8 Anant Narayanan [:anant] 2012-08-09 14:02:37 PDT
Comment on attachment 650671 [details] [diff] [review]
patchv4

No need to apologize, my first bugfix at Mozilla took 9 review cycles and it was a one line patch! :)
Comment 10 Anant Narayanan [:anant] 2012-08-09 14:54:02 PDT
Jason, when you test this the expected behaviour is for the failure callback to receive the "NOT_SUPPORTED_ERR" error message. In this test case you attached here, the text says that the test verifies if the camera feed appears but that is not expected to happen.
Comment 11 Jason Smith [:jsmith] 2012-08-09 15:00:18 PDT
(In reply to Anant Narayanan [:anant] from comment #10)
> Jason, when you test this the expected behaviour is for the failure callback
> to receive the "NOT_SUPPORTED_ERR" error message. In this test case you
> attached here, the text says that the test verifies if the camera feed
> appears but that is not expected to happen.

Ack. Copy and paste fail on my part. Disregard the text in the HTML source.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-08-09 19:56:12 PDT
https://hg.mozilla.org/mozilla-central/rev/88160f960ef8
Comment 13 :Ms2ger 2012-08-10 01:48:03 PDT
Anant, the patch that landed is not the patch that was reviewed, and is not correct.
Comment 14 Anant Narayanan [:anant] 2012-08-10 01:56:29 PDT
Do you mean the patch is wrong, or that it is incorrect to commit a patch different that the one reviewed? I added a comment to it when I merged the patch locally.
Comment 15 :Ms2ger 2012-08-10 03:18:15 PDT
No, I meant that the reviewed patch was correct, and the landed one wasn't. In particular, SendPicture() is now called if mPicture is null.
Comment 16 Anant Narayanan [:anant] 2012-08-10 08:42:54 PDT
Good catch, thank you Ms2ger! I totally fail at merging patches :(

I did not do any more reviews because the patch is now the equivalent to the original one that was reviewed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b06948b953
Comment 17 Anant Narayanan [:anant] 2012-08-10 08:45:36 PDT
Arjun, for future reference, it might be better to create patches based on mozilla-inbound rather than alder for getUserMedia related bugs so that there are fewer chances of merge conflicts that require manual resolution (alder is subtly different in many ways, especially in MediaManager.cpp).

This was clearly a result of bad judgement on my part, but just making sure we take a few more precautions so it doesn't happen again :)
Comment 18 Nagarjuna Varma [:Infinity] 2012-08-10 10:32:19 PDT
Yeah cool....its only for the getUserMedia stuff right.
Thanks for the push btw...waiting to pull my patch :P
Comment 19 Jason Smith [:jsmith] 2012-08-11 14:03:04 PDT
Is this patch backed out? I'm seeing a problem on Nightly right now where every single valid request to a MediaStream always tries to take a picture, including on Anant's test page (https://people.mozilla.com/~anarayanan/gum_test.html). As of right now, you cannot do any playback of video or audio through gUM right now.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-08-11 19:56:34 PDT
https://hg.mozilla.org/mozilla-central/rev/b3b06948b953
Comment 21 Jason Smith [:jsmith] 2012-08-11 23:57:15 PDT
I'll re-test this with the new mozilla central bits and see if the issue I hit in comment 19 still reproduces or not.
Comment 22 Henrik Skupin (:whimboo) 2012-11-25 21:52:08 PST
I think a mochitest would be good to have here.
Comment 23 Jason Smith [:jsmith] 2012-11-25 21:57:19 PST
(In reply to Henrik Skupin (:whimboo) from comment #22)
> I think a mochitest would be good to have here.

Not possible because testing this requires real devices. Using the faked devices isn't going to catch the codepaths in relation to this bug (which btw, doesn't work right now if you do incorrect combinations with fake: true, which is a separate bug - bug 798983). The associated bug is already flagged with in-testsuite? as a result already.
Comment 24 Henrik Skupin (:whimboo) 2012-11-25 22:11:20 PST
In this case it would need to be a manual test.
Comment 25 Jason Smith [:jsmith] 2012-11-25 22:30:42 PST
(In reply to Henrik Skupin (:whimboo) from comment #24)
> In this case it would need to be a manual test.

Right. I've already called this out in the test plan here - https://wiki.mozilla.org/Platform/Features/Camera_API_-_Phase_2_%28getUserMedia%29/Test_Plan that was updated when I verified this.

I'll probably port a bunch of stuff there to MozTrap at some time soon (only basic smoke tests are in MozTrap right now).

No need to track explicitly, only cause it's in the test cases in the test plan that will get up in MozTrap explicitly soon.

Note You need to log in before you can comment on or make changes to this bug.