1282584 - Potential problem with E10S preference handling in the child process

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We evaluate the preference(s) to determine if we should do E10S or not once, then use that value throughout the lifetime of the process.

However, if we start in E10S mode, disable E10S (about:config), then get to a situation where all the child processes are gone (stay with about:blank tab only, for example), we will re-evaluate the preference in the child process and start reporting that E10S is off.

Not sure if this is a problem, or if we're worried about it, thought I'd mention it.

Starting in Core::General, doesn't really belong in Preferences.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1282584: If we're not in the parent process, E10S must be on.

Review commit: https://reviewboard.mozilla.org/r/60894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60894/

Comment 2

[George Wright (:gw280) (needinfo me!)]

We'd probably want to check XRE_IsContentProcess() rather than !XRE_IsParentProcess() so that we don't hit plugins and so on. I don't know how that'd play out with plugin processes in e10s though.

A further thing to consider: we use content processes in non-e10s for thumbnailing I believe.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - I'm not exactly clear on all the combinations of values returned from XRE_IsContentProcess() and XRE_IsParentProcess().  Is there a documentation somewhere?

Comment 4

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #0)
> We evaluate the preference(s) to determine if we should do E10S or not once,
> then use that value throughout the lifetime of the process.
> 
> However, if we start in E10S mode, disable E10S (about:config), then get to
> a situation where all the child processes are gone (stay with about:blank
> tab only, for example), we will re-evaluate the preference in the child
> process and start reporting that E10S is off.
> 
> Not sure if this is a problem, or if we're worried about it, thought I'd
> mention it.
> 
> Starting in Core::General, doesn't really belong in Preferences.

If you're mucking around in about:config with a pref like remote tabs, I think a restart is implied.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8765610 [details]
Bug 1282584: If we're not in the parent process, E10S must be on.

https://reviewboard.mozilla.org/r/60894/#review57932

::: toolkit/xre/nsAppRunner.cpp:4872
(Diff revision 1)
>    if (gBrowserTabsRemoteAutostartInitialized) {
>      return gBrowserTabsRemoteAutostart;
>    }
>    gBrowserTabsRemoteAutostartInitialized = true;
>  
> +  if (!XRE_IsParentProcess()) {

I think this is ok. We have the thumbnail process which runs without e10s, but I don't see any reason why it would call into this check.

Comment 6

[Jim Mathies [:jimm]]

(In reply to George Wright (:gw280) (:gwright) from comment #2)
> We'd probably want to check XRE_IsContentProcess() rather than
> !XRE_IsParentProcess() so that we don't hit plugins and so on. I don't know
> how that'd play out with plugin processes in e10s though.
> 
> A further thing to consider: we use content processes in non-e10s for
> thumbnailing I believe.

I like this idea. Like thumbnailing I don't see an issue here with plugin processes, but this is a nice safe way to avoid any pitfalls.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8765610 [details]
Bug 1282584: If we're not in the parent process, E10S must be on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60894/diff/1-2/

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #7)
> Comment on attachment 8765610 [details]
> Bug 1282584: If we're not in the parent process, E10S must be on.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/60894/diff/1-2/

This version checks XRE_IsContentProcess instead.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ee6220f83eb
If we're not in the parent process, E10S must be on. r=jimm

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ee6220f83eb

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Milan, looks like we need to uplift this bug to fix bug 1288751. Could you fill the uplift request? Thanks

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8765610 [details]
Bug 1282584: If we're not in the parent process, E10S must be on.

Approval Request Comment
[Feature/regressing bug #]: See bug 1288751 for the real reason.
[User impact if declined]: No scrolling with the latest Win 10 preview.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Extremely low (he said confidently.)  The content process existence implies E10S.
[String/UUID change made/needed]:

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8765610 [details]
Bug 1282584: If we're not in the parent process, E10S must be on.

Thanks Milan! 
Let's take it for 48 rc2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a0c3fd01451
https://hg.mozilla.org/releases/mozilla-beta/rev/958cee08361a
https://hg.mozilla.org/releases/mozilla-release/rev/c1de04f39fa9

Comment 15

[arni2033]

This fixed the following bug:

Summary:
[e10s] Firefox disables scrolling for newly launched content processes if accessibility tool was used

STR_1:
1. Open Options in a new tab, close other tabs and other Firefox windows
2. Press Win+"=" to use Windows Magnifier. Use any other accessibility tool if you use another OS
3. Switch to another application (Notepad++), then back to Firefox
4. Open this url:   data:text/html,<style>body{height:10000px}
5. Rotate mouse wheel down on the page from Step 5

AR:  No visible action
ER:  Page should scroll down

Fix range:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b52d27f7628fd1ba34395eed42f7e00988fb9790&tochange=3ee6220f83eb6da54b906a3d3375168d6c3c0db2