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

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Bogdan Maris, Desktop QA]

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.

Comment 1

[Chris Peterson [:cpeterson]]

Florian: does this look like a Loop bug or an e10s bug?

Comment 2

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

fwiw, I have a bug to look at/use remote browsers for socialapi, however it's high effort and blocked by bug 1072980.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Maire, my read is that this is a problem with the GuM UI. Can you find an assignee?

Comment 5

[Maire Reavy [: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.

Comment 6

[Shane Caraveo (:mixedpuppy)]

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.

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[Florian Quèze [:florian]]

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

Comment 9

[Maire Reavy [:mreavy]]

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

Comment 11

[:Felipe Gomes (needinfo for replies!)]

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

Comment 12

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

Comment 13

[:Felipe Gomes (needinfo for replies!)]

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

Comment 14

[:Felipe Gomes (needinfo for replies!)]

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

Comment 15

[:Felipe Gomes (needinfo for replies!)]

sent to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=548551119f87

Comment 16

[Florian Quèze [:florian]]

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

Comment 17

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

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!

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Attachment: Folded patch for checkin, r=florian

Green on try, comment 15

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b6de7762bd17

Comment 21

[Florian Quèze [:florian]]

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?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6de7762bd17

Comment 23

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Attachment: Simple follow-up

Comment 26

[:Felipe Gomes (needinfo for replies!)]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Please make sure your patches contain commit messages when requesting checkin in the future.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e87f635d3540

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e87f635d3540

Comment 31

[Bogdan Maris, Desktop QA]

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?

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Unrelated to this bug, but I investigated that and posted my findings in bug 1222970.

Comment 33

[Ada [:adalucinet]]

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