Closed Bug 1257243 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

(e10s+)

RESOLVED FIXED
Iteration:
48.3 - Apr 25
Tracking Status
e10s + ---

People

(Reporter: standard8, Assigned: fcampo)

References

Details

(Whiteboard: [btpp-fix-now])

Attachments

(1 file)

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.
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.
Rank: 19
Priority: -- → P1
Whiteboard: [triage]
Blocks: 1258335
Rank: 19 → 9
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).
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. ;-)
(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?
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
Whiteboard: [btpp-fix-now]
Assignee: nobody → fernando.campo
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)
Attachment #8742302 - Flags: review?(standard8)
Attachment #8742302 - Flags: review?(dcritchley)
Attachment #8742302 - Flags: review?(crafuse)
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
Attachment #8742302 - Flags: review?(standard8)
Attachment #8742302 - Flags: review?(dcritchley)
Attachment #8742302 - Flags: review?(crafuse)
Attachment #8742302 - Flags: review+
https://github.com/mozilla/loop/commit/2a3c743da4d0e55b7d8c5e2ba8922223c158014f
Status: NEW → RESOLVED
Iteration: --- → 48.3 - Apr 25
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1265865
You need to log in before you can comment on or make changes to this bug.