Closed Bug 1440356 Opened 2 years ago Closed 2 years ago

Sharing indicators missing/incorrect when sharing devices in multiple frames

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 --- verified

People

(Reporter: florian, Assigned: pehrsons)

References

Details

(Keywords: regression, sec-low)

Attachments

(4 files, 2 obsolete files)

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.
Flags: needinfo?
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.
Flags: needinfo? → needinfo?(apehrson)
Summary: Sharing indicators missing/incorrect when sharing different kinds of devices in frames → Sharing indicators missing/incorrect when sharing the microphone
This sounds like bug 1438538. Could you confirm with latest nightly?
Flags: needinfo?(apehrson) → needinfo?(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.
Flags: needinfo?(florian)
Summary: Sharing indicators missing/incorrect when sharing the microphone → Sharing indicators missing/incorrect when sharing devices in multiple frames
Assignee: nobody → apehrson
Status: NEW → ASSIGNED
Rank: 5
Priority: -- → P1
Keywords: sec-low
Attachment #8953459 - Attachment description: bug_1440356_1.patch → Rewrite IterateWindowListeners to use lambdas
I'll let padenot review this c++ heavy code instead. It's somewhat in his area.
Attachment #8953459 - Attachment is obsolete: true
Attachment #8953459 - Flags: review?(jhofmann)
Attachment #8953551 - Flags: review?(padenot)
I had mis-attached the wrong file previously, thank you padenot for noticing.
Attachment #8953463 - Attachment is obsolete: true
Attachment #8953463 - Flags: review?(jhofmann)
Attachment #8953918 - Flags: review?(jhofmann)
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.
tracking as new regression.
Attachment #8953551 - Flags: review?(padenot) → review+
Attachment #8953468 - Flags: review?(jhofmann) → review+
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?
Flags: needinfo?(apehrson)
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.
This doesn't have to be hidden.
Blocks: 1408294
Group: core-security
Attached patch TestSplinter Review
I attached a test!
Flags: needinfo?(apehrson)
Attachment #8955227 - Flags: review?(florian)
Attachment #8953918 - Flags: review?(jhofmann) → review+
Comment on attachment 8955227 [details] [diff] [review]
Test

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

Thanks!
Attachment #8955227 - Flags: review?(florian) → review+
Pushed by pehrsons@gmail.com:
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
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?
Flags: needinfo?(apehrson)
(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?
Flags: needinfo?(apehrson) → needinfo?(oana.botisan)
(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.
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.
Flags: needinfo?(oana.botisan)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.