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

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect, P3)

Description

[Jason Smith [:jsmith]]

Attachment: 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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Testcase with delayed second getUserMedia() call

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Dao: any chance this will be addressed before the next uplift?  There's also now a probable dup with iframes (bug 868095)

Comment 5

[Dão Gottwald [:dao]]

(In reply to Randell Jesup [:jesup] from comment #4)
> Dao: any chance this will be addressed before the next uplift?

nope...

Comment 6

[Dão Gottwald [:dao]]

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?

Comment 8

[Florian Quèze [:florian]]

(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.

Comment 10

[Yury Tarasevich]

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?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

forwarding NI
Should be easy to test with NSPR_LOG_MODULES=mediamanager:4 logging turned on

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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.

Comment 14

[Yury Tarasevich]

(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.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 16

[Yury Tarasevich]

(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?

Comment 17

[mroberts]

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 }

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Three prompts if we count screensharing. I just checked and bug 1270572 applies there as well.

Comment 24

[Florian Quèze [:florian]]

(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.

Comment 25

[Rob Swain]

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.

Comment 26

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 27

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 861716 - queue a gUM request if there has been a pending one.

Review commit: https://reviewboard.mozilla.org/r/133962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133962/

Comment 28

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 861716 - add a mochitest for gUM request queue in MediaManager.

Review commit: https://reviewboard.mozilla.org/r/133964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133964/

Comment 29

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/1-2/

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 31

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Uh, I meant bug 1270572.

Comment 33

[Florian Quèze [: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.

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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.

Comment 35

[Florian Quèze [:florian]]

(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.

Comment 36

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 37

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 38

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8861954 [details]
Bug 861716 - queue a gUM request if there has been a pending one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133962/diff/1-2/

Comment 39

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/2-3/

Comment 40

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f9b9077fa8204adf42ef97de8062c47b05675d9&selectedJob=98674892

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 42

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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.

Comment 43

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8861954 [details]
Bug 861716 - queue a gUM request if there has been a pending one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133962/diff/2-3/

Comment 44

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/3-4/

Comment 45

[Florian Quèze [:florian]]

(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.

Comment 46

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sweet!

Comment 47

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Found test failure on linux. Cancel review.

Comment 48

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 49

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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.

Comment 50

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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 51

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8861954 [details]
Bug 861716 - queue a gUM request if there has been a pending one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133962/diff/3-4/

Comment 52

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/4-5/

Comment 53

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5aa71e8414efc65e29587b23a1a57223f6a4f0

Comment 54

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 55

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 56

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/5-6/

Comment 57

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0603f87188afd552f7458271fc17e24fa20895b

Comment 58

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 59

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

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

Lgtm.

Comment 60

[Florian Quèze [:florian]]

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 61

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/6-7/

Comment 62

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6569ced04157bbef575840f71ba7a7a1b3546b83&selectedJob=100700299

Comment 63

[Florian Quèze [:florian]]

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 64

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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.

Comment 65

[Florian Quèze [:florian]]

(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.

Comment 66

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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?

Comment 67

[Florian Quèze [:florian]]

(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.

Comment 68

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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?

Comment 69

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Please ignore comment 68.
We can customize the message in MediaStreamError.

Comment 70

[Florian Quèze [:florian]]

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 71

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/7-8/

Comment 72

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03f116fbfb7ccd2eb33583ac801c4395cac64683

Comment 73

[Florian Quèze [:florian]]

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.

Comment 74

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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

Comment 75

[Florian Quèze [:florian]]

(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.

Comment 76

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/8-9/

Comment 77

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d06181cc5fba45ae1fbc699e157b07815785cd9

Comment 78

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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 79

[Florian Quèze [:florian]]

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});
});

...

Comment 80

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/9-10/

Comment 81

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=538bdb75a360e30b9f0010485f4901c346f1efce

Comment 82

[Florian Quèze [:florian]]

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

Comment 83

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/10-11/

Comment 84

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61f360a3f4fc94daa4ff9cd26a94545831593824

Comment 85

[Florian Quèze [:florian]]

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 86

[Florian Quèze [:florian]]

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.

Comment 87

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/11-12/

Comment 88

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4898685d50fdd44d4543ff1d3ebe99c1f4be4737

Comment 89

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 90

[Florian Quèze [:florian]]

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.

Comment 91

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8861954 [details]
Bug 861716 - queue a gUM request if there has been a pending one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133962/diff/4-5/

Comment 92

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/12-13/

Comment 93

[Florian Quèze [:florian]]

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 94

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 861716 - catch exception thrown from gIndicatorWindow.updateIndicatorState().

Review commit: https://reviewboard.mozilla.org/r/147056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147056/

Comment 95

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/13-14/

Comment 96

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a431a2ed66282976ab5c3da537e35f12afb801

Comment 97

[Florian Quèze [:florian]]

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;'?

Comment 98

[Florian Quèze [:florian]]

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.

Comment 99

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(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.

Comment 100

[Florian Quèze [:florian]]

(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.

Comment 101

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Please refer to this log
https://treeherder.mozilla.org/logviewer.html#?job_id=105458768&repo=try&lineNumber=2803

error in gIndicatorWindow.updateIndicatorState(): gStringBundle is undefined

I think the exception is thrown from here:

http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/browser/base/content/webrtcIndicator.js#57

Comment 102

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

It seems we call updateIndicatorState before onload (init).

Comment 103

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 861716 - ensure webrtcIndicator init() done before calling updateIndicatorState().

Review commit: https://reviewboard.mozilla.org/r/147662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147662/

Comment 104

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/14-15/

Comment 105

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8875628 [details]
Bug 861716 - catch exception thrown from gIndicatorWindow.updateIndicatorState().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147056/diff/1-2/

Comment 106

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1d178ba9d18ef7edee3395cf286fc842209fd7b&selectedJob=105886411
eslint error is a false alarm.

Comment 107

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Comment 108

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8861954 [details]
Bug 861716 - queue a gUM request if there has been a pending one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133962/diff/5-6/

Comment 109

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133964/diff/15-16/

Comment 110

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8875628 [details]
Bug 861716 - catch exception thrown from gIndicatorWindow.updateIndicatorState().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147056/diff/2-3/

Comment 112

[Pulsebot]

Pushed by [e-mail]:
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

Comment 113

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/80acbcf225a1
https://hg.mozilla.org/mozilla-central/rev/4f5477c79f4f
https://hg.mozilla.org/mozilla-central/rev/95d89b5bcc9c