Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla17

Status

()

Core
WebRTC
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jsmith, Assigned: Infinity)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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?
(Reporter)

Comment 1

5 years ago
Same problem also occurs in the combination of {picture: true, audio: true}.
(Reporter)

Comment 2

5 years ago
Generally appears to happen if picture: true is in the dictionary no matter what, even if incorrect params come with it.
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+]
(Assignee)

Updated

5 years ago
Assignee: nobody → junky.argonaut
(Assignee)

Comment 3

5 years ago
Created attachment 650630 [details] [diff] [review]
Patchv1

Anant, could you have a look at it and tell me if it is ok?
Attachment #650630 - Flags: review?(anant)
(Assignee)

Comment 4

5 years ago
Created attachment 650640 [details] [diff] [review]
Patchv2

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)
(Assignee)

Comment 6

5 years ago
Created attachment 650667 [details] [diff] [review]
patchv3

Fixed the indentation and early return.
Attachment #650640 - Attachment is obsolete: true
Attachment #650667 - Flags: review?(anant)
(Assignee)

Comment 7

5 years ago
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
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/88160f960ef8
(Reporter)

Updated

5 years ago
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.
(Reporter)

Comment 11

5 years ago
(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
Last Resolved: 5 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.
(Reporter)

Updated

5 years ago
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 :)
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
(Assignee)

Comment 18

5 years ago
Yeah cool....its only for the getUserMedia stuff right.
Thanks for the push btw...waiting to pull my patch :P
(Reporter)

Comment 19

5 years ago
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
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+]
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [backout-needed]
https://hg.mozilla.org/mozilla-central/rev/b3b06948b953
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+], [backout-needed] → [getUserMedia], [blocking-gum+], [qa+]
(Reporter)

Comment 21

5 years ago
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.
(Reporter)

Updated

5 years ago
Severity: critical → normal
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+], [qa!]
(Reporter)

Updated

5 years ago
Flags: in-testsuite-
I think a mochitest would be good to have here.
Flags: in-testsuite- → in-testsuite?
(Reporter)

Comment 23

5 years ago
(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)
(Reporter)

Comment 25

5 years ago
(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.