Closed Bug 861716 Opened 7 years ago Closed 2 years ago

Multiple async calls to getUserMedia before user response causes earlier requests to be lost

Categories

(Firefox :: Device Permissions, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: jsmith, Assigned: mchiang)

References

Details

(Whiteboard: [getUserMedia][blocking-gum-])

Attachments

(5 files, 1 obsolete file)

Attached file Test Case
STR:

1. Load the attached test case

Expected:

If two async calls are made right after each other, the permission requests should be stacked right after each other. This means I'd expect behavior along the lines of:

1. First gUM call - async call starts, waits on perm prompt (first in queue), then completes callback
2. Second gUM call, - async call starts, waits in queue, then waits on perm prompt, then completes callback

Actual:

In my case, the first gUM call never happened - the JS code was completely ignored. Trying other cases, I've generally noticed we run into trouble with multiple async calls stacked in a row. We should be queueing the requests, not throwing them away.
Whiteboard: [getUserMedia]
This testcase shouldn't work the way you expect - the getUserMedia spec tells us to merge requests made in the same iteration (before the next stable state); in particular to avoid complexities about having to specify constraints for multiple streams of the same type.  For an audio and a video request, it should merge them (but fire both success callbacks, with no required ordering).

We don't support multiple streams of the same type in one UI requester, so right now we'd have to present the requests serially in violation of the intent of the spec (you had assumed serial was the spec requirement).  We could support merging audio and video requests, but don't do so currently.

The bug right now is "losing" the first request - no success or error callbacks.  The preferred result would be to process the second result serially; we must at least return the one of the requests with an error.

This losing of the first request has nothing to do with same-state merging; this happens even if the second request comes 10 seconds later - it replaces the doorhanger with a new one, without sending the current getUserMedia request back with an error (and without stacking them for serial resolution).

So given this, it's a UI bug.  I'll update the testcase to show the non-same-stable-state case.  There's still an issue with not merging an audio and a video request in the same stable state (from the same source) before sending it to the UI which we do not handle currently on the gUM side, we'll send serial requests to the UI.
Assignee: nobody → dao
Component: WebRTC → General
Product: Core → Firefox
QA Contact: jsmith
Summary: Multiple async calls to getUserMedia in a row can't be executed - certain async calls are lost → Multiple async calls to getUserMedia before user response causes earlier requests to be lost
Duplicate of this bug: 864110
Dao: any chance this will be addressed before the next uplift?  There's also now a probable dup with iframes (bug 868095)
Flags: needinfo?(dao)
(In reply to Randell Jesup [:jesup] from comment #4)
> Dao: any chance this will be addressed before the next uplift?

nope...
Flags: needinfo?(dao)
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum-]
So I could deny the first request when the prompt gets removed due to the second prompt showing. However, there are other situations where prompts get removed, like when the requesting document goes away. I probably shouldn't send getUserMedia:response:deny in that case?
Flags: needinfo?(rjesup)
Duplicate of this bug: 920656
Assignee: dao → nobody
Component: General → Device Permissions
Flags: firefox-backlog+
Flags: needinfo?(rjesup)
(In reply to Dão Gottwald [:dao] from comment #6)

> there are other situations where prompts get
> removed, like when the requesting document goes away.

This specific case (for frames) has been handled in bug 1114433. When the whole tab goes away, notification icons are dismissed automatically so we have nothing to do.
Duplicate of this bug: 1173859
Hi guys,

I have the same problem (like as in the first message there), when I try to call 2 'getUserMedia' requests (with different constraints):

navigator.mozGetUserMedia({audio: true}, function(stream) {
    console.log("First (audio) call made.");
}, function(err) {
    console.log("First (audio) error made.");
});

navigator.mozGetUserMedia({video: true}, function(stream) {
    console.log("Second (video) call made.");
}, function(err) {
    console.log("Second (video) error made.");
});

But I don't receive second callback.
Have you any updates for this issue?
Flags: needinfo?(rjesup)
forwarding NI
Should be easy to test with NSPR_LOG_MODULES=mediamanager:4 logging turned on
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
Flags: needinfo?(florian)
Duplicate of this bug: 1201781
(In reply to Yury Tarasevich from comment #10)
> But I don't receive second callback.

What version did you try? See Bug 1201781 comment 1.
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #13)
> (In reply to Yury Tarasevich from comment #10)
> > But I don't receive second callback.
> 
> What version did you try? See Bug 1201781 comment 1.

I use FireFox 40.0.3 on Windows 7.

Also in example from https://bugzilla.mozilla.org/show_bug.cgi?id=1201781#c1 I see only one log notice: "Video Success!". Nothing about audio stream.
Flags: needinfo?(jib)
And when I try your example in comment 10 I get "Second (video) call made.", so I'm seeing consistent results, given that the two examples are almost identical. But you're seeing inconsistent results. Here's how I tested comment 10: http://jsfiddle.net/jib1/v8qrLtq3

In any case, both are symptoms of a lack of handling sequential requests, and it is possible there's a race condition here, so I'm comfortable marking these as duplicates.
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> And when I try your example in comment 10 I get "Second (video) call made.",
> so I'm seeing consistent results, given that the two examples are almost
> identical. But you're seeing inconsistent results. Here's how I tested
> comment 10: http://jsfiddle.net/jib1/v8qrLtq3
> 
> In any case, both are symptoms of a lack of handling sequential requests,
> and it is possible there's a race condition here, so I'm comfortable marking
> these as duplicates.

I have checked all our situations once again (comment 10, attachment for bug: https://bugzilla.mozilla.org/attachment.cgi?id=737326&action=edit and jsfiddle example: http://jsfiddle.net/jib1/v8qrLtq3/) and should say that in all those cases I have the same behavior - only the last callback was fired.

Sorry for inconvenience above (in comment 10 I also receive second callback).

But I still would like to know status for this issue.
Have you any updates?
See Also: → 1224766
This bug is also present if you use the `navigator.mediaDevices.getUserMedia` interface:

    var p1 = navigator.mediaDevice.getUserMedia({ audio: true, video: true });
    var p2 = navigator.mediaDevice.getUserMedia({ audio: true, video: true });

    // After allowing, p1 will forever be in state "pending":
    //
    // >> p1
    // Promise { <state>: "pending" }
    //
    // >> p2
    // Promise { <state>: "fulfilled", <value>: LocalMediaStream }
Duplicate of this bug: 1276365
Flags: needinfo?(florian)
Priority: -- → P3
Duplicate of this bug: 868095
Duplicate of this bug: 1300089
Duplicate of this bug: 1303185
With bug 1270572 landed, I think we can queue requests as they come in, because the user will see at most two prompts, no matter how many requests there were:

 * Once the user grants video, all queued video requests are granted as well.
 * Once the user grants audio, all queued audio requests are granted as well.
 * Once the user rejects video, all queued video requests are rejected as well.
 * Once the user rejects audio, all queued audio requests are rejected as well.
 * Queued audio+video prompts might be skipped also, or reduced to audio- or video-only as needed.

Merging requests is problematic IMHO, as this could be used for permission escalation. I think we're better off with a straight queue. The merging advice in the spec is non-normative now anyway.

However, never resolving or rejecting the promise (i.e. "not working") is likely not spec-compliant.
Three prompts if we count screensharing. I just checked and bug 1270572 applies there as well.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #23)
> Three prompts if we count screensharing. I just checked and bug 1270572
> applies there as well.

A lot more if using specific constraints, as bug 1270572 will only grant access automatically if it's the same device. But I think it's still ok to fix this with a queue, as the first time a user says no is the end of it.
I've also observed this issue when calling gUM multiple times in quick succession (which is potentially not unusual for a full-mesh type application.) The problem for me is that only one of the gUM requests resolves/succeeds and I don't get any response at all for the others.

This seems to have a long history already. My expectation would be that I get prompted for permissions for each gUM request (for example, if I don't select to remember my choice) but I'd be OK with grouping multiple gUM requests into one UI permission request if that is what the spec suggests to do. However, I would certainly expect that regardless of how the permissions requests are handled (unless the permissions request is never responded to...), all of the gUM requests would resolve/call back.
What about adding a queue in MediaManager and let the behavior the same as these gUMs are called one-by-one?
As Florian mentioned, it may show a lot more prompts because bug 1270572 only grant the unprompted access if the existing device fulfills the constraint of 2nd gUM.
However, considering the complexity of merging request, this change can fix the non-resolved/rejected issue with minimal cost.
Comment on attachment 8861954 [details]
Bug 861716 - queue a gUM request if there has been a pending one.

https://reviewboard.mozilla.org/r/133962/#review137180

Thanks for catching this! Looks like we were accumulating callIds on successful gUM calls until page navigation. Hopefully benign provided uuid collisions are rare.
Attachment #8861954 - Flags: review?(jib) → review+
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review137194

Thanks Munro for stepping up and providing a fix!

Ideally though I think this logic belongs in the JS permission code, and is for the Device Permissions team to handle, even though it is tempting! See my concerns below though. Florian what do you think? Would this be simple to do in JS instead?

::: dom/media/MediaManager.cpp:2383
(Diff revision 2)
> +        if (!Preferences::GetBool("media.navigator.permission.force") && array->Length() > 1) {
> +          // there is at least 1 pending gUM request
> +          // For the scarySources test case, always send the request
> +          self->mPendingGUMRequest.AppendElement(req.forget());
> +        } else {
> -        obs->NotifyObservers(req, "getUserMedia:request", nullptr);
> +          obs->NotifyObservers(req, "getUserMedia:request", nullptr);
> -      }
> +        }

I'm a bit nervous approving this, as MediaManager handles multiple outstanding callIds per window today, vending requests for different devices and kinds.

As you know, persistent permissions (which are per-kind today, maybe per-device tomorrow?) are handled entirely in the JS permission code.

Wouldn't queueing here potentially mean a microphone request for which the user has previously granted persistent permission, might get blocked behind a camera request for which the user has NOT granted persistent permission, and vice versa?

Similarly would a screensharing request block both cam and mic gUM requests even when users have granted full persistent permission?

Also, with the work you just added in bug 861716, wouldn't a request for a different device block all re-requests for gUM where the site already currently has access?
Flags: needinfo?(florian)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #31)

> Ideally though I think this logic belongs in the JS permission code, and is
> for the Device Permissions team to handle, even though it is tempting!

If you have reasonable code doing it in MediaManager, that's fine with me.

> Florian what do you think? Would this be simple to
> do in JS instead?

The general idea seems simple enough to implement, but I would need to think more about potential edge cases. If we add a queue of notifications, we will need to think about how to discard notifications from the queue when they are no longer relevant (eg. if you have in the queue notifications for gum requests for an iframe, and that iframe is removed before we are done processing the previous requests of the queue). Not discarding them would likely leak the iframe. I would need to think a bit more about it and look at the code to see if it's straightforward or tricky to handle.

> Wouldn't queueing here potentially mean a microphone request for which the
> user has previously granted persistent permission, might get blocked behind
> a camera request for which the user has NOT granted persistent permission,
> and vice versa?
> 
> Similarly would a screensharing request block both cam and mic gUM requests
> even when users have granted full persistent permission?

Yes to both, and I think that's also the behavior we would implement if we were to write the implementation in JS.

> Also, with the work you just added in bug 861716, wouldn't a request for a
> different device block all re-requests for gUM where the site already
> currently has access?

Seems likely; and I don't think it's a problem.
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> > Wouldn't queueing here potentially mean a microphone request for which the
> > user has previously granted persistent permission, might get blocked behind
> > a camera request for which the user has NOT granted persistent permission,
> > and vice versa?
> > 
> > Similarly would a screensharing request block both cam and mic gUM requests
> > even when users have granted full persistent permission?
> 
> Yes to both, and I think that's also the behavior we would implement if we
> were to write the implementation in JS.

I would think in JS we could queue only the prompt() part. That way, persistent permissions and other rule-based auto-granting  wouldn't get blocked.

> > Also, with the work you just added in bug 861716, wouldn't a request for a
> > different device block all re-requests for gUM where the site already
> > currently has access?
> 
> Seems likely; and I don't think it's a problem.

Again, if we queued at the prompt() it wouldn't block.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #34)
> (In reply to Florian Quèze [:florian] [:flo] from comment #33)
> > > Wouldn't queueing here potentially mean a microphone request for which the
> > > user has previously granted persistent permission, might get blocked behind
> > > a camera request for which the user has NOT granted persistent permission,
> > > and vice versa?
> > > 
> > > Similarly would a screensharing request block both cam and mic gUM requests
> > > even when users have granted full persistent permission?
> > 
> > Yes to both, and I think that's also the behavior we would implement if we
> > were to write the implementation in JS.
> 
> I would think in JS we could queue only the prompt() part. That way,
> persistent permissions and other rule-based auto-granting  wouldn't get
> blocked.

We currently intentionally block auto-granting until the tab is selected in a focused window. This is currently done by using some of the prompting code, and returning early just before we display a popup notification. So if you want to avoid blocking requests that could be auto-granted using existing persistent permissions, this would probably need a custom code path specifically for it. This is doable, but I think outside the scope of this bug.
By "prompt" I guess I meant "we display a popup notification". Should we open a new bug and block this one on it?

I don't see how we get there if we queue in MediaManager.
Florian convinced me that taking this patch is the best short-term solution.

We should probably add a test of the MediaManager queue though, as I suspect our test coverage here is poor.

This would need manual testing as well.
Assignee: nobody → mchiang
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review142106

Looks good! I think it should be extended to cover two more cases however. Let me know if you want to do that in this patch or a new one and I'll r+ this one.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:12
(Diff revision 3)
> +
> +var gTests = [
> +
> +{
> +  desc: "getUserMedia queue request",
> +  run: function* checkAudioVideoWhileLiveTracksExist_camera() {

Rename function.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:27
(Diff revision 3)
> +    Assert.deepEqual((yield getMediaCaptureState()), {video: true},
> +                     "expected camera to be shared");
> +    yield indicator;
> +    yield checkSharingUI({audio: false, video: true});
> +
> +    yield promise;
> +    yield expectObserverCalled("getUserMedia:request");
> +    checkDeviceSelectors(true, false);
> +
> +    yield promiseMessage(permissionError, () => {
> +      activateSecondaryAction(kActionDeny);

This appears to test queueing deny audio behind allow video, which is great. Maybe a brief comment to that effect?

But I think we need code coverage for the two other places SendPendingGUMRequest is called as well, e.g.:

 1. A test queueing allow video behind deny audio, and
 2. A test queueing allow audio behind allow video with error

For the second one, you can provoke the NotReadable error test-kludge in our fake camera with:

    gUM({ video: { deviceId: 'bad device' }, fake: true })

From https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/dom/media/tests/mochitest/test_getUserMedia_constraints.html#50-51

Lastly, would it make sense to test explicitly that queueing e.g. allow video behind allow video does not prompt twice?

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:53
(Diff revision 3)
> +add_task(async function test() {
> +  await runTests(gTests);

Any reason not to embrace async/await for new tests?

Seems like we could easily add a runAsyncTests copy of runTests [1] with

 - await Task.spawn(testCase.run(browser));
 + await testCase.run(browser);

and it should just work. Florian, would this lose anything?

[1] https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/browser/base/content/test/webrtc/head.js#535
Attachment #8861955 - Flags: review?(jib) → review-
Flags: needinfo?(florian)
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review142106

> This appears to test queueing deny audio behind allow video, which is great. Maybe a brief comment to that effect?
> 
> But I think we need code coverage for the two other places SendPendingGUMRequest is called as well, e.g.:
> 
>  1. A test queueing allow video behind deny audio, and
>  2. A test queueing allow audio behind allow video with error
> 
> For the second one, you can provoke the NotReadable error test-kludge in our fake camera with:
> 
>     gUM({ video: { deviceId: 'bad device' }, fake: true })
> 
> From https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/dom/media/tests/mochitest/test_getUserMedia_constraints.html#50-51
> 
> Lastly, would it make sense to test explicitly that queueing e.g. allow video behind allow video does not prompt twice?

I will add three tests for all above mentioned scenarios.

> Any reason not to embrace async/await for new tests?
> 
> Seems like we could easily add a runAsyncTests copy of runTests [1] with
> 
>  - await Task.spawn(testCase.run(browser));
>  + await testCase.run(browser);
> 
> and it should just work. Florian, would this lose anything?
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/browser/base/content/test/webrtc/head.js#535

I will leave this to another bug.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #41)

>  - await Task.spawn(testCase.run(browser));
>  + await testCase.run(browser);
> 
> and it should just work. Florian, would this lose anything?

This change was done as part of bug 1353542.
Flags: needinfo?(florian)
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

Found test failure on linux. Cancel review.
Attachment #8861955 - Flags: review?(jib)
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review142636

Some issues with testQueuingAllowVideoBehindAllowVideo, the other tests lgtm, modulo the test failures.

Also, with bug 1353542 landed we'll need to switch to async/await in order to land.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:23
(Diff revisions 3 - 4)
> +    let recordingPromises = [promiseObserverCalled("recording-device-events"),
> +                             promiseObserverCalled("recording-device-events")];
> +
> +    let responsePromises = [promiseObserverCalled("getUserMedia:response:allow"),
> +                            promiseObserverCalled("getUserMedia:response:allow")];

Calling promiseObserverCalled twice will just register two listeners that will both be satisfied by the same event, so having two doesn't do anything different from having one.

Instead, I think we need to interspese these checks somehow with the requests, to capture the difference before and after this patch, if possible.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:29
(Diff revisions 3 - 4)
> +    promise = promiseMessage("ok");
> +    yield promiseMessage("ok", () => {
> +      PopupNotifications.panel.firstChild.button.click();
> +    });

Same problem here I think. These will resolve at the same time from a single event as far as I can tell.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:46
(Diff revisions 3 - 4)
> +    SitePermissions.remove(null, "screen", gBrowser.selectedBrowser);
> +    SitePermissions.remove(null, "camera", gBrowser.selectedBrowser);
> +    SitePermissions.remove(null, "microphone", gBrowser.selectedBrowser);

Is it common to leave these even when we don't deny any permissions in this test?
Attachment #8861955 - Flags: review-
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review142636

> Calling promiseObserverCalled twice will just register two listeners that will both be satisfied by the same event, so having two doesn't do anything different from having one.
> 
> Instead, I think we need to interspese these checks somehow with the requests, to capture the difference before and after this patch, if possible.

The behavior of "queuing allow video behind allow video" doesn't change before and after this patch.
Even without this queuing patch, the 2nd gUM can still get resolve.
The only difference is the timing. With this patch, the 2nd gUM resolve will come later.
But I don't know how to catch such kind of timing difference in mochitest.
(In reply to Munro Mengjue Chiang [:mchiang] from comment #49)
> Even without this queuing patch, the 2nd gUM can still get resolve.
both 1st and 2nd gUM can still get resolve.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144406

All good, except I think I found a similar problem to before. Would like to see it again if you find a fix.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:98
(Diff revisions 4 - 5)
> +    if (AppConstants.platform == "linux") {
> +      todo(false, "Bug 1365850");
> +      return;

So windows opt build is fine?

::: browser/base/content/test/webrtc/head.js:205
(Diff revisions 4 - 5)
> +    for (let i = 0 ; i < aCount ; i++) {
> -    mm.sendAsyncMessage("Test:WaitForObserverCall", aTopic);
> +      mm.sendAsyncMessage("Test:WaitForObserverCall", aTopic);
> +    }

Looking at "Test:WaitForObserverCall" [1] I think this just registers multiple independent observers for the same topic, which will all be satisfied by a single notification, no matter how high aCount is.

[1] https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/browser/base/content/test/webrtc/get_user_media_content_script.js#99-111
Attachment #8861955 - Flags: review?(jib) → review-
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144406

> So windows opt build is fine?

Yes, windows opt/debug builds are fine.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144406

> Yes, windows opt/debug builds are fine.

Ah, my bad, I confused the two tests.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144740

Lgtm.
Attachment #8861955 - Flags: review?(jib) → review+
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144748

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:19
(Diff revision 6)
> +  desc: "test queueing deny audio behind allow video",
> +  run: async function testQueuingDenyAudioBehindAllowVideo() {
> +    let promise = promisePopupNotificationShown("webRTC-shareDevices");
> +    await promiseRequestDevice(false, true);
> +    await promiseRequestDevice(true, false);
> +    await promise;

I think you want a checkDeviceSelectors call after each await of a webRTC-shareDevices notification promise.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:46
(Diff revision 6)
> +    });
> +
> +    await expectObserverCalled("getUserMedia:response:deny");
> +    SitePermissions.remove(null, "screen", gBrowser.selectedBrowser);
> +    SitePermissions.remove(null, "camera", gBrowser.selectedBrowser);
> +    SitePermissions.remove(null, "microphone", gBrowser.selectedBrowser);

You only need one of these 3 lines.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:54
(Diff revision 6)
> +    await closeStream();
> +  }
> +},
> +
> +{
> +  desc: "test queueing allow video behind deny audio",

Should we also test queueing audio+video behind deny audio (or deny video)?

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:88
(Diff revision 6)
> +    await indicator;
> +    await checkSharingUI({audio: false, video: true});
> +
> +    SitePermissions.remove(null, "screen", gBrowser.selectedBrowser);
> +    SitePermissions.remove(null, "camera", gBrowser.selectedBrowser);
> +    SitePermissions.remove(null, "microphone", gBrowser.selectedBrowser);

Same here.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144904

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:69
(Diff revision 7)
> +      activateSecondaryAction(kActionDeny);
> +    });
> +
> +    await expectObserverCalled("getUserMedia:response:deny");
> +
> +    await promise;

Still no checkDeviceSelectors call here.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:116
(Diff revision 7)
> +      PopupNotifications.panel.firstChild.button.click();
> +    });
> +
> +    await expectObserverCalled("getUserMedia:response:allow");
> +
> +    await promise;

nor here (checkDeviceSelectors)

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:146
(Diff revision 7)
> +    await promiseRequestDevice(true, true);
> +    await promise;
> +    await expectObserverCalled("getUserMedia:request");
> +    checkDeviceSelectors(true, false);
> +
> +    await promiseMessage(permissionError, () => {

Can we check that this permissionError message is received twice?

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_queue_request.js:13
(Diff revision 7)
> +  desc: "test queueing allow video behind allow video",
> +  run: async function testQueuingAllowVideoBehindAllowVideo() {
> +    let promise = promisePopupNotificationShown("webRTC-shareDevices");
> +    await promiseRequestDevice(false, true);
> +    await promiseRequestDevice(false, true);
> +    await promise;

checkDeviceSelectors?
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review144904

> Can we check that this permissionError message is received twice?

Could you give some advices regarding how to do this?
If I create two promise to monitor the event, both of them will get resolved when the first event arrives.
But If I create the second promise immediately after the first promise get resolved, it will still miss the 2nd event and lead to timeout.
(In reply to Munro Mengjue Chiang [:mchiang] from comment #64)
> Comment on attachment 8861955 [details]
> Bug 861716 - add a mochitest for gUM request queue in MediaManager.
> 
> https://reviewboard.mozilla.org/r/133964/#review144904
> 
> > Can we check that this permissionError message is received twice?
> 
> Could you give some advices regarding how to do this?
> If I create two promise to monitor the event, both of them will get resolved
> when the first event arrives.
> But If I create the second promise immediately after the first promise get
> resolved, it will still miss the 2nd event and lead to timeout.

We may need to introduce an additional string inside the messages that would uniquely identify each request.

Or we may need a way to count messages.
(In reply to Florian Quèze [:florian] [:flo] from comment #65)
> We may need to introduce an additional string inside the messages that would
> uniquely identify each request.
> 
> Or we may need a way to count messages.

I think it is good to have a new method to monitor events which come multiple times.
It would be also useful for the test case of unprompted access and temporarily block gUM feature.
Shall we create another bug for this?
(In reply to Munro Mengjue Chiang [:mchiang] from comment #66)

> It would be also useful for the test case of unprompted access and
> temporarily block gUM feature.
> Shall we create another bug for this?

Improving other tests that aren't related to this bug should be handled in another bug, yes. Testing that queuing works the way we expect (and that the webpage receives rejections for both requests) should be done here.
(In reply to Florian Quèze [:florian] [:flo] from comment #65)
> We may need to introduce an additional string inside the messages that would
> uniquely identify each request.
> 
> Or we may need a way to count messages.

To introduce an additional string, I suppose it involve changes in dom.
It seems has a huge effort and beyond my scope.

If we just want to ensure there are two concatenate rejects, I have a simpler way to do it.
Add a new pref, and when the pref is on, we just send another dom error message, like NotSupportedError, instead of NotAllowedError for the 2nd gUM.

What do you think?
Please ignore comment 68.
We can customize the message in MediaStreamError.
The message is sent from http://searchfox.org/mozilla-central/rev/15edcfd962be2c8cfdd0b8a96102150703bd4ac5/browser/base/content/test/webrtc/get_user_media.html#42
It's trivial to add a prefix (add an additional optional parameter to the requestDevice function called requestName or something like that).
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review145090

::: browser/base/content/test/webrtc/get_user_media.html:31
(Diff revisions 7 - 8)
> +var gResolveCounter = 0;
> +var gRejectCounter = 0;
> +
> +function requestDevice(aAudio, aVideo, aShare, aBadDevice = false, aCounterTarget = 1) {
> +  gResolveCounter = 0;
> +  gRejectCounter = 0;

These 2 global variables are scary. If the first requestDevice call's promise resolves before the second call, you'll never get to the expected count and we'll have an intermittent failure.

What was the problem with the suggestion I gave (comment 70) of adding an optional name for each request so that we could wait for the various messages independently?


The 'aCounterTarget' parameter name is also not explicit, there's no way to guess what it is for without reading the code.
Attachment #8861955 - Flags: review?(florian) → review-
(In reply to Florian Quèze [:florian] [:flo] from comment #73)
> What was the problem with the suggestion I gave (comment 70) of adding an
> optional name for each request so that we could wait for the various
> messages independently?

We can add additional string into the message. But the two promiseMessage will still get resolved when the first message arrives.

https://searchfox.org/mozilla-central/source/browser/base/content/test/webrtc/get_user_media_content_script.js#124
(In reply to Munro Mengjue Chiang [:mchiang] from comment #74)
> (In reply to Florian Quèze [:florian] [:flo] from comment #73)
> > What was the problem with the suggestion I gave (comment 70) of adding an
> > optional name for each request so that we could wait for the various
> > messages independently?
> 
> We can add additional string into the message. But the two promiseMessage
> will still get resolved when the first message arrives.
> 
> https://searchfox.org/mozilla-central/source/browser/base/content/test/
> webrtc/get_user_media_content_script.js#124

Ah. Then that code needs to also be aware of the prefix and resolve/remove the listener only when a message with the expected prefix has been received.
(In reply to Florian Quèze [:florian] [:flo] from comment #65)
> We may need to introduce an additional string inside the messages that would
> uniquely identify each request.
> 
> Or we may need a way to count messages.
Now I have found a way to count messages with the same string.
So we don't need to put the additional string.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review145460

I think the approach will work, but there's a mistake in the actual implementation.

::: browser/base/content/test/webrtc/head.js:261
(Diff revisions 7 - 9)
>      });
>      mm.sendAsyncMessage("Test:WaitForMessage");
>    });
>  }
>  
> +function promiseSpecificMessageReceived(aMessage, aNumOfOccurrence = 1) {

nit: you probably expect more than 1 occurences, so you would need an 's' after 'Occurence' in this parameter name. But I would just name it 'aCount' or 'aExpectedCount'.

::: browser/base/content/test/webrtc/head.js:262
(Diff revisions 7 - 9)
>      mm.sendAsyncMessage("Test:WaitForMessage");
>    });
>  }
>  
> +function promiseSpecificMessageReceived(aMessage, aNumOfOccurrence = 1) {
> +  return new Promise((resolve, reject) => {

nit: you don't use reject so you can simplify to 'new Promise(resolve => {'

::: browser/base/content/test/webrtc/get_user_media_content_script.js:119
(Diff revision 9)
> +    sendAsyncMessage("Test:MessageReceived", data);
> +  });
> +});
> +
> +addMessageListener("Test:StopWaitForMutipleMessages", () => {
> +  content.removeEventListener("message", ({data}) => {

This won't work, as you are creating a new function here; it won't remove the listener you added from Test:WaitForMutipleMessages

You need something like:

function messageListener({data}) {
  sendAsyncMessage("Test:MessageReceived", data);
}

addMessageListener("Test:WaitForMessage", () => {
  content.addEventListener("message", messageListener, {once: true});
});

...
Attachment #8861955 - Flags: review?(florian) → review-
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review145972

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:145
(Diff revision 10)
> +  run: async function testQueuingAllowVideoBehindDenyAudio() {
> +    let promise = promisePopupNotificationShown("webRTC-shareDevices");
> +    await promiseRequestDevice(true, false);
> +    await promiseRequestDevice(true, true);
> +    await promise;
> +    promise = promiseSpecificMessageReceived(permissionError, 2);

nit: move this line right before "activateSecondaryAction(kActionDeny);".

::: browser/base/content/test/webrtc/head.js:214
(Diff revision 10)
>                            function listener({data}) {
> -      is(data.count, 1, "expected notification " + aTopic);
> +      is(data.count, aCount, "expected notification " + aTopic);
>        mm.removeMessageListener("Test:ExpectObserverCalled:Reply", listener);
>        resolve();
>      });
> +    for (let i = 0 ; i < aCount ; i++) {

I don't trust this loop, you are sending this message several times to decrement the gObservedTopics counter in the content script, but it'll also cause a Test:ExpectObserverCalled:Reply message to be sent back each time... and we only wait for the first message here. The next expectObserverCalled call could randomly receive the second message, and cause intermittent failures.

Instead I think we could change this call to:
mm.sendAsyncMessage("Test:ExpectObserverCalled", {topic: aTopic, count: aCount});
and adapt the listener at http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/browser/base/content/test/webrtc/get_user_media_content_script.js#54
Attachment #8861955 - Flags: review?(florian) → review-
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review146040

::: browser/base/content/test/webrtc/get_user_media_content_script.js:51
(Diff revisions 10 - 11)
> -addMessageListener("Test:ExpectObserverCalled", ({data}) => {
> +addMessageListener("Test:ExpectObserverCalled", ({ data: { topic, count } }) => {
>    sendAsyncMessage("Test:ExpectObserverCalled:Reply",
> -                   {count: gObservedTopics[data]});
> -  if (data in gObservedTopics)
> -    --gObservedTopics[data];
> +                   {count: gObservedTopics[topic]});
> +  if (topic in gObservedTopics) {
> +    for (let i = 0 ; i < count ; i++) {
> +      --gObservedTopics[topic];

gObservedTopics[topic] -= count;
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review146042

::: browser/base/content/test/webrtc/get_user_media.html:40
(Diff revision 11)
>      opts.fake = true;
>    }
>  
> +  if (aVideo && aBadDevice) {
> +    opts.video = {
> +      deviceId: 'bad device'

Eslint errors on this, it should use double quotes.

::: browser/base/content/test/webrtc/get_user_media.html:46
(Diff revision 11)
> +    }
> +  }
> +
> +  if (aAudio && aBadDevice) {
> +    opts.audio = {
> +      deviceId: 'bad device'

same here.
Attachment #8861955 - Flags: review?(florian) → review-
Hi Florian,

May I know if you still have any concern?
And also please help have a full patch review considering this is not a big change.

Then I can address all the bugs in one revision and accelerate the progress.

Thanks again for the insightful advices.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review148032

This looks close to r+, but I would like more details about bug 1365850 and bug 1365852 before r+ing.

::: browser/base/content/test/webrtc/browser.ini:21
(Diff revision 12)
>  [browser_devices_get_user_media_unprompted_access.js]
>  [browser_devices_get_user_media_unprompted_access_in_frame.js]
>  [browser_devices_get_user_media_unprompted_access_tear_off_tab.js]
>  skip-if = (os == "win" && bits == 64) # win8: bug 1334752
> +[browser_devices_get_user_media_unprompted_access_queue_request.js]
> +skip-if = (os =="win" || os == "linux") && debug # bug 1365852

bug 1365852 doesn't look like an actionable bug report. There's no information at all in there about how often this is failing. Have you tried to debug this instead of filing an intermittent bug for a test that hasn't even landed yet?

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:1
(Diff revision 12)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Tests should be public domain, not MPL.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_queue_request.js:9
(Diff revision 12)
> +
> +const permissionError = "error: NotAllowedError: The request is not allowed " +
> +    "by the user agent or the platform in the current context.";
> +
> +const badDeviceError = "error: NotReadableError: Failed to allocate " +
> +    "videosource";

nit: wrap this line after the '=' to avoid breaking the string in two:

const badDeviceError =
  "error: ...

::: browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access_queue_request.js:18
(Diff revision 12)
> +    await promise;
> +    checkDeviceSelectors(false, true);
> +    promise = promiseSpecificMessageReceived("ok", 2);
> +    await expectObserverCalled("getUserMedia:request");
> +
> +    PopupNotifications.panel.firstChild.button.click();

nit: the 'promiseSpecificMessageReceived("ok", 2);' line should be right before this one.
Not re-using the 'promise' variable name would be better. 'promiseOK' would be more readable here.
Attachment #8861955 - Flags: review?(florian) → review-
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review150708

::: browser/modules/webrtcUI.jsm:1071
(Diff revision 13)
> -    if (!gIndicatorWindow)
> +    if (!gIndicatorWindow) {
>        gIndicatorWindow = getGlobalIndicator();
> -    else
> +    } else {
> +      try {
> -      gIndicatorWindow.updateIndicatorState();
> +        gIndicatorWindow.updateIndicatorState();
> +      } catch(e) {ok(false, e);}

'ok' won't be defined here, when I said to use ok(false, e) I thought it was test code; sorry about the confusion :(.
Comment on attachment 8861955 [details]
Bug 861716 - add a mochitest for gUM request queue in MediaManager.

https://reviewboard.mozilla.org/r/133964/#review151236

Looks good to me.

So is bug 1365850 fixed by adding the two 'opts.fake = true;'?
Attachment #8861955 - Flags: review?(florian) → review+
Comment on attachment 8875628 [details]
Bug 861716 - catch exception thrown from gIndicatorWindow.updateIndicatorState().

https://reviewboard.mozilla.org/r/147056/#review151238

I'm ok with catching the exception here to report it... but have you managed to see what exception we are catching to verify that we are not hiding a real bug?

You may need to add a SimpleTest.requestCompleteLog(); in the relevant test to have the full log available even when the test doesn't fail.

::: browser/modules/webrtcUI.jsm:1071
(Diff revision 1)
> -    if (!gIndicatorWindow)
> +    if (!gIndicatorWindow) {
>        gIndicatorWindow = getGlobalIndicator();
> -    else
> +    } else {
> +      try {
> -      gIndicatorWindow.updateIndicatorState();
> +        gIndicatorWindow.updateIndicatorState();
> +      } catch(err) {

eslint is unhappy about this line on your try push.
Attachment #8875628 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #97)
> So is bug 1365850 fixed by adding the two 'opts.fake = true;'?
Yes.

(In reply to Florian Quèze [:florian] [:flo] from comment #98)
> I'm ok with catching the exception here to report it... but have you managed
> to see what exception we are catching to verify that we are not hiding a
> real bug?
> 
> You may need to add a SimpleTest.requestCompleteLog(); in the relevant test
> to have the full log available even when the test doesn't fail.
I will add SimpleTest.requestCompleteLog() and submit a new try request to see if we can catch the exception.
(In reply to Munro Mengjue Chiang [:mchiang] from comment #99)

> > You may need to add a SimpleTest.requestCompleteLog(); in the relevant test
> > to have the full log available even when the test doesn't fail.
> I will add SimpleTest.requestCompleteLog() and submit a new try request to
> see if we can catch the exception.

For that try push you may want to also add a dump(`error in gIndicatorWindow.updateIndicatorState(): ${err.message}\n`) in the catch block just to be sure.
It seems we call updateIndicatorState before onload (init).
I will land the first three patches
Bug 861716 - queue a gUM request if there has been a pending one.
Bug 861716 - add a mochitest for gUM request queue in MediaManager
Bug 861716 - catch exception thrown from gIndicatorWindow.updateIndicatorState().

And address below issue in bug 1365852
Bug 861716 - ensure webrtcIndicator init() done before calling updateIndicatorState().
Attachment #8876214 - Attachment is obsolete: true
Attachment #8876214 - Flags: review?(florian)
Duplicate of this bug: 1365850
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80acbcf225a1
queue a gUM request if there has been a pending one. r=jib
https://hg.mozilla.org/integration/autoland/rev/4f5477c79f4f
add a mochitest for gUM request queue in MediaManager. r=florian,jib
https://hg.mozilla.org/integration/autoland/rev/95d89b5bcc9c
catch exception thrown from gIndicatorWindow.updateIndicatorState(). r=florian
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.