[e10s] Global indicator is dismissed after closing a loop call even though other active items are using the indicator at the same time

VERIFIED FIXED in Firefox 43

Status

()

Firefox
Device Permissions
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: bogdan_maris, Assigned: Felipe)

Tracking

37 Branch
Firefox 43
Points:
3
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(e10sm8+, firefox43 fixed)

Details

Attachments

(4 attachments)

STR:
1. Open Firefox
2. Join a Room (hello, loop) call. It is not needed for another person to join the call, only for the Global indicator to appear.
3. Visit https://people.mozilla.org/~fqueze2/webrtc/ff_gum_test.html
4. Share a selected Window (this can be reproduced with other sharing items)
5. Close the Room (hello, loop) call.


Expected results: The camera icon from global indicator is dismissed (in this case) but the indicator is still there.

Actual results: Global indicator is dismissed even though the window share is still active.

Notes:
1. This issue reproduces only with e10s enabled across all platforms: Windows, Linux and Mac.
Florian: does this look like a Loop bug or an e10s bug?
tracking-e10s: --- → ?
Flags: needinfo?(florian)
(In reply to Chris Peterson (:cpeterson) from comment #1)
> Florian: does this look like a Loop bug or an e10s bug?

Looks more like an e10s bug.

The problem is that http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentWebRTC.jsm is used from 2 different process: the child process when e10s on and gUM is used in tabs; the chrome process for Loop because the SocialAPI browser is not remote.

The 2 possible solutions are:
- Refactor the code in http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm to support more than one 'child' process.
- Make SocialAPI use remote browsers for chat windows.
Flags: needinfo?(florian)
fwiw, I have a bug to look at/use remote browsers for socialapi, however it's high effort and blocked by bug 1072980.
Maire, my read is that this is a problem with the GuM UI. Can you find an assignee?
tracking-e10s: ? → m6+
Flags: needinfo?(mreavy)
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> 
> The 2 possible solutions are:
> - Refactor the code in
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm
> to support more than one 'child' process.
> - Make SocialAPI use remote browsers for chat windows.

I'm concerned that the "refactor" option may be only a partial solution for the problem we really need to fix.  For example, the user will still not be able to use getUserMedia for a camera while Loop/Hello is active.

Mark/Florian/Shane -- What would it take to do the second option (make SocialAPI use remote browsers for chat windows)?   I believe we're going to *have* to do this as part of the sandboxing effort.
Flags: needinfo?(standard8)
Flags: needinfo?(mreavy)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(florian)
I have an exploratory patch for bug 1074553 but am blocked by bug 1072980.  I'll post the patch soon, may not have bandwidth for it until after holidays, we'll see.
Flags: needinfo?(mixedpuppy)
For Loop, I think using remote browsers/content would require bug 1048850 - that's something that will be potentially a bigger write than we'd want to uplift.
Flags: needinfo?(standard8)
(In reply to Mark Banner (:standard8) from comment #7)
> For Loop, I think using remote browsers/content would require bug 1048850 -
> that's something that will be potentially a bigger write than we'd want to
> uplift.

I don't think we are talking about uplifting anything here, this bug becomes a serious issue only if e10s is pref'ed on. Currently I think it's still only on central.
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> (In reply to Mark Banner (:standard8) from comment #7)
> > For Loop, I think using remote browsers/content would require bug 1048850 -
> > that's something that will be potentially a bigger write than we'd want to
> > uplift.
> 
> I don't think we are talking about uplifting anything here, this bug becomes
> a serious issue only if e10s is pref'ed on. Currently I think it's still
> only on central.

Yes, exactly.  I'm not suggesting we uplift this.  I'd like to fix it as soon as we can, and then let it ride the trains.  With that in mind, I think the second option is the better choice.  I'm wondering what the level of effort is for bug 1048850 and this bug.  (Bug 1048850 is currently marked "blocking-loop Fx37+".)
Flags: firefox-backlog+
Component: General → Device Permissions
(Assignee)

Updated

3 years ago
Assignee: nobody → felipc
Points: --- → 3
Flags: qe-verify+
(Assignee)

Updated

3 years ago
tracking-e10s: m6+ → ?

Updated

3 years ago
tracking-e10s: ? → m8+
(Assignee)

Updated

3 years ago
Assignee: felipc → nobody
Assignee: nobody → felipc
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1126231
(Assignee)

Comment 11

3 years ago
Created attachment 8646756 [details] [diff] [review]
Handle multiple processes

Simple handling of the indicator state of multiple processes.

What are the tests I should run and any ideas how feasible it is to write one for this bug?
Attachment #8646756 - Flags: review?(florian)
Comment on attachment 8646756 [details] [diff] [review]
Handle multiple processes

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

Thanks for the patch!

This seems to fix bug 1166450 too, which isn't really a surprise given you added a child-process-shutdown listener :).

Both before and after applying your patch, with a Linux debug build when I close the main browser window (which causes the child process to shutdown), I see a JS error from the child process in the terminal:
###!!! [Child][MessageChannel] Error: (msgtype=0x3A00E4,name=PContent::Msg_AsyncMessage) Closed channel: cannot send/recv

[Child 16979] WARNING: MsgDropped in ContentChild: file /home/florian/buildhg/mozilla-central/dom/ipc/ContentChild.cpp, line 1980
JavaScript error: resource:///modules/ContentWebRTC.jsm, line 189: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMessageSender.sendAsyncMessage]


Is there a notification similar to child-process-shutdown but fired on the content side, that we could use to remove the recording-device-events (and I guess the other 2) observer? If it's trivial, let's do it, if not we can file a follow-up, as it's not directly related.

(In reply to :Felipe Gomes from comment #11)

> What are the tests I should run and any ideas how feasible it is to write
> one for this bug?

You definitely want to ensure you are not regressing the browser/base/content/general/browser_devices_get_user_media* tests (they are fine on my linux debug local testing).

As for testing this bug... I think bug 1071623 is in our way, so it's probably best to fix that first. You would probably want to get ContentWebRTC.jsm loaded from several different processes, and check that ending the device share in one process doesn't remove the global indicator. We may be able to fake it by subscript-loading the ContentWebRTC.jsm file a few times in a single process, but the result would probably end up being different enough from the real e10s multi-process situation that I'm not sure we would still be testing anything useful.

::: browser/modules/webrtcUI.jsm
@@ +83,5 @@
> +        list.push(indicators.showScreenSharingIndicator);
> +    }
> +
> +    let precedence =
> +      ["Application", "Screen", "Window", "Browser", ""];

To match the current order at http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentWebRTC.jsm#209 the precedence order here should be: Screen, Window, Application, Browser.
Attachment #8646756 - Flags: review?(florian) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8646987 [details] [diff] [review]
Clear observers in child

> Is there a notification similar to child-process-shutdown but fired on the
> content side, that we could use to remove the recording-device-events (and I
> guess the other 2) observer? If it's trivial, let's do it, if not we can
> file a follow-up, as it's not directly related.

Yeah, there's something simple. I'm posting as a separate patch because I haven't tested it with a debug build (and with an opt build on Mac I don't get this warning.. although perhaps I should?). In any case, can you test it? If it works I'll fold it in the main patch, otherwise we can investigate in a separate bug.
Attachment #8646987 - Flags: review?(florian)
(Assignee)

Comment 14

3 years ago
> > +    let precedence =
> > +      ["Application", "Screen", "Window", "Browser", ""];
> 
> To match the current order at
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentWebRTC.
> jsm#209 the precedence order here should be: Screen, Window, Application,
> Browser.

Oh right. Thinking more about it, wouldn't it make sense for it to be Screen, Application, Window, Browser?
(In reply to :Felipe Gomes from comment #14)
> > > +    let precedence =
> > > +      ["Application", "Screen", "Window", "Browser", ""];
> > 
> > To match the current order at
> > http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentWebRTC.
> > jsm#209 the precedence order here should be: Screen, Window, Application,
> > Browser.
> 
> Oh right. Thinking more about it, wouldn't it make sense for it to be
> Screen, Application, Window, Browser?

We scratched our heads for a while on this. It would make more programing sense, but less UX sense. The reason is that if you are sharing an application + a window, saying you are sharing windows is true (an application share is a stream containing all the windows of the application in a single stream), but saying you are sharing applications (plural) isn't correct.
(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> (In reply to :Felipe Gomes from comment #14)
> > > > +    let precedence =
> > > > +      ["Application", "Screen", "Window", "Browser", ""];
> > > 
> > > To match the current order at
> > > http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentWebRTC.
> > > jsm#209 the precedence order here should be: Screen, Window, Application,
> > > Browser.
> > 
> > Oh right. Thinking more about it, wouldn't it make sense for it to be
> > Screen, Application, Window, Browser?
> 
> We scratched our heads for a while on this. It would make more programing
> sense, but less UX sense. The reason is that if you are sharing an
> application + a window, saying you are sharing windows is true (an
> application share is a stream containing all the windows of the application
> in a single stream), but saying you are sharing applications (plural) isn't
> correct.

I actually would like this to be covered by tests, because it's so against obvious logic when looking at the code that I expect someone to break it someday. Unfortunately there's currently zero test coverage for the screen/app/window sharing UI :-(.
Comment on attachment 8646987 [details] [diff] [review]
Clear observers in child

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

This fixes the JS error on my linux debug build. Thanks!
Attachment #8646987 - Flags: review?(florian) → review+
(Assignee)

Comment 19

3 years ago
Created attachment 8647118 [details] [diff] [review]
Folded patch for checkin, r=florian

Green on try, comment 15
Attachment #8647118 - Flags: review+
Attachment #8647118 - Flags: checkin?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8647118 - Flags: checkin? → checkin+
On an updated fx-team build (opt, Mac), I see this JS error in my terminal at shutdown:
JavaScript error: resource:///modules/ContentWebRTC.jsm, line 35: ReferenceError: handleRequest is not defined

Any idea of what could be causing it?
https://hg.mozilla.org/mozilla-central/rev/b6de7762bd17
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(Assignee)

Comment 23

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> On an updated fx-team build (opt, Mac), I see this JS error in my terminal
> at shutdown:
> JavaScript error: resource:///modules/ContentWebRTC.jsm, line 35:
> ReferenceError: handleRequest is not defined
> 
> Any idea of what could be causing it?

Function name changed in http://hg.mozilla.org/mozilla-central/rev/bd7d66caa793
(Assignee)

Comment 24

3 years ago
Created attachment 8647596 [details] [diff] [review]
Simple follow-up
Attachment #8647596 - Flags: review?(florian)
Attachment #8647596 - Flags: review?(florian) → review+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1166450
(Assignee)

Comment 26

3 years ago
Comment on attachment 8647596 [details] [diff] [review]
Simple follow-up

Don't want to waste infra resources on pushing just this patch, marking it for checkin-needed
Attachment #8647596 - Flags: checkin?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Comment on attachment 8647596 [details] [diff] [review]
Simple follow-up

Please make sure your patches contain commit messages when requesting checkin in the future.
Attachment #8647596 - Flags: checkin? → checkin+
Duplicate of this bug: 1071627
Confirming that this issue is no longer reproducible with latest 44.0a2 and 45.0a1 (from 2015-11-08), under Windows 7 64-bit and Ubuntu 14.04 64-bit.
Under Mac OS X 10.11.1, I hit crash bug 1222970 when Firefox is closed while the global indicator is visible. Felipe, any ideas?
Flags: needinfo?(felipc)
(Assignee)

Comment 32

3 years ago
Unrelated to this bug, but I investigated that and posted my findings in bug 1222970.
Flags: needinfo?(felipc)

Comment 33

3 years ago
Verified fixed with latest 44.0a2 and 45.0a1 (from 2015-11-18), across platforms [1]

[1] Windows 7 64-bit, Mac OS X 10.8.5 and Ubuntu 14.04 32-bit
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.