Closed Bug 1325049 Opened 3 years ago Closed 3 years ago

Global sharing indicator doesn't work correctly with e10s multi

Categories

(Firefox :: Device Permissions, defect, P1, major)

defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox54 --- verified
firefox55 --- verified

People

(Reporter: florian, Assigned: florian, NeedInfo)

References

Details

(Whiteboard: [e10s-multi:+])

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:
1. Enable e10s multi (set dom.ipc.processCount to 2 in about:config)
2. Load https://people-mozilla.org/~fqueze2/webrtc/ and share the camera.
3. Load http://mozilla.github.io/webrtc-landing/gum_test.html in a second tab, and share the camera.
4. Click the "Stop" button.
5. (Mac) Click the Camera icon in the menubar.

Expected result:
The menu should offer to control sharing for the first tab.

Actual result:
The menu has only one disable item, saying "Sharing Camera with 0 tabs".


This is because when receiving the "webrtc:UpdatingIndicators" message from any content process, webrtcUI.jsm clears the webrtcUI._streams array that contained info about active streams from all processes: http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/browser/modules/webrtcUI.jsm#217
Whiteboard: [e10s-multi:?]
E10s multi is on by default in Nightly.  Who can work on this?  Thanks
Flags: needinfo?(florian)
Florian is already looking at it, but we forgot to assign it.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
Attached patch Fix (obsolete) — Splinter Review
Attachment #8847597 - Flags: review?(felipc)
(In reply to Randell Jesup [:jesup] from comment #1)
> E10s multi is on by default in Nightly.  Who can work on this?  Thanks

It took me more time than I expected to attach a 'working' patch, because it took me a while to figure out how to reliably have 2 different content processes within my test.

From seeing the try results from comment 4, I think I'll need help to figure out the debug-only failures of my new test:
Assertion failure: [GFX1]: Texture deallocated too late during shutdown, at /home/worker/workspace/build/src/gfx/2d/Logging.h:518
Flags: needinfo?(florian) → needinfo?(rjesup)
Attached patch Fix v2Splinter Review
Fixed the eslint error, and disabled the test on e10s && debug (filed bug 1347625 to cover this assertion).
Attachment #8847680 - Flags: review?(felipc)
Attachment #8847597 - Attachment is obsolete: true
Attachment #8847597 - Flags: review?(felipc)
Comment on attachment 8847597 [details] [diff] [review]
Fix

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

(There should probably be a helper function to write tests that load tabs in different processes..)
Attachment #8847597 - Attachment is obsolete: false
Comment on attachment 8847597 [details] [diff] [review]
Fix

(gah, mid-air)
Attachment #8847597 - Attachment is obsolete: true
Attachment #8847680 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4e0318c18c37fc2fdf89cdd7cdf2398af0c7b56
Bug 1325049 - Fix the global webrtc sharing indicator to work with multiple content processes, r=felipe.
https://hg.mozilla.org/mozilla-central/rev/e4e0318c18c3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8847680 [details] [diff] [review]
Fix v2

I heard that we may turn on e10s-multi for some users in 54, if so I think we should uplift this fix for them.

Approval Request Comment
[Feature/Bug causing the regression]: e10s-multi
[User impact if declined]: WebRTC sharing indicators won't be reliable for users sharing devices in tabs running in different content processes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: Not really required, but would be nice if QE could do some exploration around this. STRs are in the initial bug description.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Acceptable for aurora in my option.
[Why is the change risky/not risky?]: The risk is reduced by relatively extensive test coverage for this area of the code.
[String changes made/needed]: none
Attachment #8847680 - Flags: approval-mozilla-aurora?
Comment on attachment 8847680 [details] [diff] [review]
Fix v2

Support e10s-multi experiment. Fix a WebRTC sharing indicator issue. Aurora54+.

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Attachment #8847680 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached video Recording #7.mp4
Hi, I tested this issue on Mac OS X 10.12 with FF Nightly 55.0a1(2017-03-19) and following the scenario from the description the issue is no longer reproducible. 

I did some exploratory and I found a bug following this scenario: 

You need to have 2 monitors to reproduce this. 

STR:
1.  Open Firefox on screen no.1
2. Load https://people-mozilla.org/~fqueze2/webrtc/ and share the camera.
3. Load http://mozilla.github.io/webrtc-landing/gum_test.html in a second tab, and share the camera.
4. Click the "Stop" button.
5. Click the Camera icon in the menubar.(from the second screen)

Expected result:
The menu should offer to control sharing for the first tab.

Actual result:

The control sharing for the first tab is open for ~1 sec and after that disappears. 

Please see the video recorder for a better understanding. 

Florian, please take a look at this, thanks.
Flags: needinfo?(brindusa.tot) → needinfo?(florian)
(In reply to ovidiu boca[:Ovidiu] from comment #14)
> Created attachment 8849018 [details]
> Recording #7.mp4
> 
> Hi, I tested this issue on Mac OS X 10.12 with FF Nightly 55.0a1(2017-03-19)
> and following the scenario from the description the issue is no longer
> reproducible. 
> 
> I did some exploratory and I found a bug following this scenario: 
> 
> You need to have 2 monitors to reproduce this. 
> 
> STR:
> 1.  Open Firefox on screen no.1
> 2. Load https://people-mozilla.org/~fqueze2/webrtc/ and share the camera.
> 3. Load http://mozilla.github.io/webrtc-landing/gum_test.html in a second
> tab, and share the camera.
> 4. Click the "Stop" button.
> 5. Click the Camera icon in the menubar.(from the second screen)
> 
> Expected result:
> The menu should offer to control sharing for the first tab.
> 
> Actual result:
> 
> The control sharing for the first tab is open for ~1 sec and after that
> disappears. 
> 
> Please see the video recorder for a better understanding. 
> 
> Florian, please take a look at this, thanks.

Please file a new bug for this, thanks!

From looking at the recording, it seems like the issue here is caused by opening the sharing indicator from the menu bar in the second screen. Can you reproduce if in the second tab you load any other page (ie. don't start a video stream from a second tab)?
Flags: needinfo?(florian)
Hi Florian, 
I filled a bug like you suggested, please see bug 1349141.

"Can you reproduce if in the second tab you load any other page (ie. don't start a video stream from a second tab)?"

I tried this scenario and I'm still able to reproduce it. 

What I also saw is that this issue is reproducible only on Mac OS X 10.12. I tried to reproduce the scenario from bug description, scenario from bug 1349141 and I couldn't.
Let's make sure this works as expected on Beta 54.
Flags: qe-verify+
I have reproduced this issue using Firefox 52.0a2 (2016.12.21) on Mac OS 10.11.
I can confirm this issue is fixed, I verified using Firefox 54.0b4 and 55.0a1 on Mac OS X 10.11.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1393761
You need to log in before you can comment on or make changes to this bug.