1257243 - [e10s] Handle sessions starting tab sharing on non-remote tabs

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

From bug 1254102 comment 24:

> FWIW, the reverse behavior also exists: if you start a loop session while a
> chrome tab is selected, sending chrome works, but content tabs will be
> inaccessible.

I think we need to come up with some way around this, probably before we ship with e10s enabled.

Comment 1

[Ian Bicking (:ianbicking)]

Proposed fix: when we start Hello, if we detect the current tab is a non-remote tab (such as about:preferences), then before we start Hello we will open and activate a new tab, showing about:home.

Comment 2

[Mark Banner (:standard8)]

From an implementation perspective, MozLoopAPI#AddBrowserSharingListener already has a way to find out if the selected tab is a remote or non-remote, so we should be able to base a fix either in AddBrowserSharingListener or using some of the constructs there if its better to fix it elsewhere (like maybe when the user opens the room from the panel).

Comment 3

[Mike de Boer [:mikedeboer]]

This is all part of the wonderful possibility that we can open non-e10s windows during a browser session to start with.
Ideally we'd just rely on the pref value: is e10s enabled by default in our Fx host, yes or no? If yes, assume remote=true tabs/ browsers and show black screens for non-remote ones (like some about: pages).
If not, do the opposite.
I'd consider the 'is window remote?' check in MozLoopAPI a hack that ought to be deprecated. What if you want to test Loop in a non-e10s Fx version? Flip the pref and restart. That, in my opinion, is the path of least resistance and hours spent. ;-)

Comment 4

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> This is all part of the wonderful possibility that we can open non-e10s
> windows during a browser session to start with.

We're talking about tabs not windows...

> Ideally we'd just rely on the pref value: is e10s enabled by default in our
> Fx host, yes or no? If yes, assume remote=true tabs/ browsers and show black
> screens for non-remote ones (like some about: pages).

Unfortunately, if we start WebRTC tab sharing on a non-remote tab, then it sticks with that process - its all about the process in which tab sharing was started. So if we start with about:config, then all you'll ever be able to share is about:preferences, and most other about pages.

> That, in my opinion, is the path of least resistance and hours spent. ;-)

Mike, does the above clarify?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #4)
> We're talking about tabs not windows...

I know... I'll clarify below.

> Unfortunately, if we start WebRTC tab sharing on a non-remote tab, then it
> sticks with that process - its all about the process in which tab sharing
> was started. So if we start with about:config, then all you'll ever be able
> to share is about:preferences, and most other about pages.

That's how we do things right now, true. What I'm saying is that we need to make the decision depend on the global variable `gMultiProcessBrowser` as defined in browser.js, not on the first available browser element.

This way things will work like this:

 - Remote browser window, URLs will be loaded in remote browsers by default, unless it's an about: page that has to be loaded in a non-remote browser: Hello conversations are opened in a remote browser, so browser sharing will be capturing in the content process. Non-remote pages will not be captured, thus resulting in a black square being sent across.
 - Non-remote browser window, URLs will be loaded in non-remote browsers by default, including all about: pages: Hello conversations are opened in a non-remote browser, so browser sharing will be capturing in the main process. Remote pages will not be captured, thus resulting in a black square being sent across.

Comment 6

[GitHub Autolander Bot]

Attachment: [loop] fcampo:e10s-remote-tab-1257243 > mozilla:master

Comment 7

[Fernando Campo (:fcampo)]

Comment on attachment 8742302 [details] [review]
[loop] fcampo:e10s-remote-tab-1257243 > mozilla:master

Based on comment 5:
> That's how we do things right now, true. What I'm saying is that we need to make
> the decision depend on the global variable `gMultiProcessBrowser` as defined in
> browser.js, not on the first available browser element.
...changed the check on IsMultiProcessActive.

Also now we check for remote tab on room creation, open new about:home if it's not.
Check when opening existing room shouldn't be needed as the changes would extend (we shouldn't be able to open a room on a non-remote tab, so that url should never be stored in a room)

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8742302 [details] [review]
[loop] fcampo:e10s-remote-tab-1257243 > mozilla:master

This looks reasonable, and seems to work well. Lets try it. r=Standard8

Comment 9

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/2a3c743da4d0e55b7d8c5e2ba8922223c158014f