1440356 - Sharing indicators missing/incorrect when sharing devices in multiple frames

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Florian Quèze [:florian]]

STRs:
1. Load data:text/html,<iframe src="https://mozilla.github.io/webrtc-landing/gum_test.html"></iframe><iframe src="https://mozilla.github.io/webrtc-landing/gum_test.html"></iframe>

2. In the second frame, click "Audio" and accept the prompt.
3. In the first frame, click "Video" and accept the prompt.

After step 2, I sometimes have no sharing indicator displayed at all (neither the global one nor the tab specific one).

After step 3, I usually have a microphone sharing indicator, despite the web page having access to both the microphone and camera.

mozregression says:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3018e377678c88dd85fd6dec8bda445a9e894141&tochange=93a8b769cb7a76aa8eb1c0b182c84d5bdf2fcb71

Marking as security sensitive for now and requesting tracking for 60 because having a page getting a stream from a device without any sharing indicator is scary.

Comment 1

[Florian Quèze [:florian]]

There seem to be some intermittent component to this, and I'm not sure this has anything to do with frames. The steps in comment 0 let me reproduce consistently, but I can also often reproduce with these simpler steps:
1. Load https://mozilla.github.io/webrtc-landing/gum_test.html
2. Click "Audio" and accept the prompt.

I sometimes see no sharing indicator at all after doing this.

Sharing "Video" or "Video + Audio" seems to reliably bring the sharing indicators. So this seems to have something to do with a gUM request getting only a microphone stream.

Comment 2

[Andreas Pehrson [:pehrsons]]

This sounds like bug 1438538. Could you confirm with latest nightly?

Comment 3

[Florian Quèze [:florian]]

(In reply to Andreas Pehrson [:pehrsons] from comment #2)
> This sounds like bug 1438538. Could you confirm with latest nightly?

Applying the patch from bug 1438538 fixes the STRs from comment 1. When using the more complicated case with frames from comment 0, there is still a problem: it seems we show the sharing indicators only for the second frame when both frames have an active stream.

Eg. if frame1 shares audio+video, I have the correct sharing indicators, then if I start sharing video only in frame2, the audio sharing indicator disappears.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Rewrite IterateWindowListeners to use lambdas

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Combine CaptureState for all child windows when calculating overall state

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Only call the window callback for found listeners

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Rewrite IterateWindowListeners to use lambdas

I'll let padenot review this c++ heavy code instead. It's somewhat in his area.

Comment 8

[Andreas Pehrson [:pehrsons]]

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

and with the last update (making the unused var `DebugOnly`):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=734a75a493c0bf3bde1f5629887645a797d29f38

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Combine CaptureState for all child windows when calculating overall state

I had mis-attached the wrong file previously, thank you padenot for noticing.

Comment 10

[Andreas Pehrson [:pehrsons]]

The issue here is that we reset the full capture state on each window listener iteration, i.e., UI would only know about the state of the last child window. This is fixed by the patch "Combine CaptureState for all child windows when calculating overall state".

Then since I touched IterateWindowListeners which seems written ages ago (passing a void ptr to be mutated in the static callback), and we didn't ship this regression further than Nightly, I took the liberty of rewriting it first to use lambdas that can capture references to allow for the callback to be inlined.

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

tracking as new regression.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8953918 [details] [diff] [review]
Combine CaptureState for all child windows when calculating overall state

This patch works fine for me but I think we should really have an automated test for the iframe example that Florian built. Considering that this is just a regression in 60, I think we would still have time to write it in this bug. It's probably not really important whether we test the platform APIs or through checking UI in a browser chrome mochitest.

Andreas, what do you think?

Comment 13

[Florian Quèze [:florian]]

A slight variation of https://searchfox.org/mozilla-central/rev/056a4057575029f58eb69ae89bc7b6c3c11e2e28/browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js#133-135 should be enough to have this case covered. It's actually bad luck that this bug breaks just the corner case that wasn't already covered by the test.

Comment 14

[Daniel Veditz [:dveditz]]

This doesn't have to be hidden.

Comment 15

[Johann Hofmann [:johannh]]

Attachment: Test

I attached a test!

Comment 16

[Johann Hofmann [:johannh]]

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

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8955227 [details] [diff] [review]
Test

Review of attachment 8955227 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e75a851a23
Rewrite IterateWindowListeners to use lambdas. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/71c5ae05d2ab
Combine CaptureState for all child windows when calculating overall state. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c554a92554
Only call the window callback for found listeners. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e29db472c7
Add a test for frames overriding WebRTC sharing UI. r=florian

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f3e75a851a23
https://hg.mozilla.org/mozilla-central/rev/71c5ae05d2ab
https://hg.mozilla.org/mozilla-central/rev/c1c554a92554
https://hg.mozilla.org/mozilla-central/rev/20e29db472c7

Comment 20

[Oana Botisan, Desktop QA]

I managed to reproduce the issue only on Windows 10 x64 using a version of Nightly from 2018-02-22. 
I partially followed the steps from comment 1, because I couldn't reproduce the issue with the steps from comment 0(The video was not played back, but the sound indicators were present).

 1. Load https://mozilla.github.io/webrtc-landing/gum_test.html
 2. Click "Audio & Video" and accept the prompt.

On the older version of Nightly I didn't get any audio indicators (I unmuted the video) and no video playback.

I retested everything on the same platform using latest Nightly 61.0a1 and beta 60.0b6. The issue is not reproducing. The sound indicators are displayed as soon as the "Mute" is unchecked (and the video is displayed as well). 

Are this results relevant? Could we consider this bug fixed?

Comment 21

[Andreas Pehrson [:pehrsons]]

(In reply to Oana Botisan from comment #20)
> I managed to reproduce the issue only on Windows 10 x64 using a version of
> Nightly from 2018-02-22. 
> I partially followed the steps from comment 1, because I couldn't reproduce
> the issue with the steps from comment 0(The video was not played back, but
> the sound indicators were present).
> 
>  1. Load https://mozilla.github.io/webrtc-landing/gum_test.html
>  2. Click "Audio & Video" and accept the prompt.
> 
> On the older version of Nightly I didn't get any audio indicators (I unmuted
> the video) and no video playback.
> 
> I retested everything on the same platform using latest Nightly 61.0a1 and
> beta 60.0b6. The issue is not reproducing. The sound indicators are
> displayed as soon as the "Mute" is unchecked (and the video is displayed as
> well). 
> 
> Are this results relevant? Could we consider this bug fixed?

No, your step 1 doesn't do iframes which is crucial to reproducing this. The repro you mention is likely of some other bug.

I can repro fine per comment 0, what's the issue you're facing?

Comment 22

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] from comment #21)
> No, your step 1 doesn't do iframes which is crucial to reproducing this. The
> repro you mention is likely of some other bug.

Perhaps bug 1438538.

Comment 23

[Oana Botisan, Desktop QA]

I finally managed to reproduce the bug using the steps from comment 0 using Nightly from 2018-02-22 on Windows 10 x64. I only got the audio indicator and no video incicator nor image for the video iframe. 

I retested everything using latest Nightly 61.0a1 and beta 60.0b11 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The bug is not reproducing anymore. Both the microphone and the video indicators are displayed in tab and global.