Closed Bug 780553 Opened 10 years ago Closed 9 years ago

Providing an incorrect combination of parameters (video: true, picture: true) to getUserMedia - unexpected success, picture taken

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla17

People

(Reporter: jsmith, Assigned: Infinity)

Details

(Whiteboard: [getUserMedia], [blocking-gum+], [qa!])

Attachments

(2 files, 3 obsolete files)

Attached file 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?
Same problem also occurs in the combination of {picture: true, audio: true}.
Generally appears to happen if picture: true is in the dictionary no matter what, even if incorrect params come with it.
Whiteboard: [getUserMedia], [blocking-gum+]
Assignee: nobody → junky.argonaut
Attached patch Patchv1 (obsolete) — Splinter Review
Anant, could you have a look at it and tell me if it is ok?
Attachment #650630 - Flags: review?(anant)
Attached patch Patchv2 (obsolete) — Splinter Review
Is this fine?
Attachment #650630 - Attachment is obsolete: true
Attachment #650630 - Flags: review?(anant)
Attachment #650640 - Flags: review?(anant)
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.
Attachment #650640 - Flags: review?(anant)
Attached patch patchv3 (obsolete) — Splinter Review
Fixed the indentation and early return.
Attachment #650640 - Attachment is obsolete: true
Attachment #650667 - Flags: review?(anant)
Attached patch patchv4Splinter Review
Havn't tested it though
This must be the highest number of reviews to code ratio...sorry about that
Attachment #650667 - Attachment is obsolete: true
Attachment #650667 - Flags: review?(anant)
Attachment #650671 - Flags: review?(anant)
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! :)
Attachment #650671 - Flags: review?(anant) → review+
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/88160f960ef8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Anant, the patch that landed is not the patch that was reviewed, and is not correct.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+]
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
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 :)
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
Yeah cool....its only for the getUserMedia stuff right.
Thanks for the push btw...waiting to pull my patch :P
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.
Severity: normal → critical
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+]
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [backout-needed]
https://hg.mozilla.org/mozilla-central/rev/b3b06948b953
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [getUserMedia], [blocking-gum+], [backout-needed] → [getUserMedia], [blocking-gum+], [qa+]
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.
Severity: critical → normal
Status: RESOLVED → VERIFIED
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+], [qa!]
Flags: in-testsuite-
I think a mochitest would be good to have here.
Flags: in-testsuite- → in-testsuite?
(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.
Flags: in-testsuite? → in-testsuite-
In this case it would need to be a manual test.
Flags: in-moztrap?(jsmith)
(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.
Flags: in-moztrap?(jsmith)
You need to log in before you can comment on or make changes to this bug.