Closed
Bug 780553
Opened 12 years ago
Closed 12 years ago
Providing an incorrect combination of parameters (video: true, picture: true) to getUserMedia - unexpected success, picture taken
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
VERIFIED
FIXED
mozilla17
People
(Reporter: jsmith, Assigned: Infinity)
Details
(Whiteboard: [getUserMedia], [blocking-gum+], [qa!])
Attachments
(2 files, 3 obsolete files)
2.57 KB,
text/html
|
Details | |
696 bytes,
patch
|
anant
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Same problem also occurs in the combination of {picture: true, audio: true}.
Reporter | ||
Comment 2•12 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•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+]
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → junky.argonaut
Assignee | ||
Comment 3•12 years ago
|
||
Anant, could you have a look at it and tell me if it is ok?
Attachment #650630 -
Flags: review?(anant)
Assignee | ||
Comment 4•12 years ago
|
||
Is this fine?
Attachment #650630 -
Attachment is obsolete: true
Attachment #650630 -
Flags: review?(anant)
Attachment #650640 -
Flags: review?(anant)
Comment 5•12 years ago
|
||
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•12 years ago
|
||
Fixed the indentation and early return.
Attachment #650640 -
Attachment is obsolete: true
Attachment #650667 -
Flags: review?(anant)
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
Comment 10•12 years ago
|
||
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•12 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.
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88160f960ef8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 13•12 years ago
|
||
Anant, the patch that landed is not the patch that was reviewed, and is not correct.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+]
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
Assignee | ||
Comment 18•12 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•12 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•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+]
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [backout-needed]
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3b06948b953
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+], [backout-needed] → [getUserMedia], [blocking-gum+], [qa+]
Reporter | ||
Comment 21•12 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•12 years ago
|
Severity: critical → normal
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+], [qa!]
Reporter | ||
Updated•12 years ago
|
Flags: in-testsuite-
Comment 22•12 years ago
|
||
I think a mochitest would be good to have here.
Flags: in-testsuite- → in-testsuite?
Reporter | ||
Comment 23•12 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-
Reporter | ||
Comment 25•12 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.
Description
•