Potential problem with E10S preference handling in the child process

VERIFIED FIXED in Firefox 48

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

unspecified
mozilla50
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox48 fixed, firefox49 fixed, firefox50 verified)

Details

Attachments

(1 attachment)

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.
Created attachment 8765610 [details]
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/
Attachment #8765610 - Flags: review?(jmathies)
Summary: Potential problem with E10S preference handing in the child process → Potential problem with E10S preference handling in the child process
tracking-e10s: --- → ?
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.
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

2 years ago
(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.

Updated

2 years ago
Attachment #8765610 - Flags: review?(jmathies) → review+

Comment 5

2 years ago
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

2 years ago
(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.

Updated

2 years ago
tracking-e10s: ? → +
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/
(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.
Assignee: nobody → milan

Comment 9

2 years ago
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ee6220f83eb
If we're not in the parent process, E10S must be on. r=jimm

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ee6220f83eb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Milan, looks like we need to uplift this bug to fix bug 1288751. Could you fill the uplift request? Thanks
Flags: needinfo?(milan)
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]:
Flags: needinfo?(milan)
Attachment #8765610 - Flags: approval-mozilla-beta?
Attachment #8765610 - Flags: approval-mozilla-aurora?
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
Attachment #8765610 - Flags: approval-mozilla-release+
Attachment #8765610 - Flags: approval-mozilla-beta?
Attachment #8765610 - Flags: approval-mozilla-beta+
Attachment #8765610 - Flags: approval-mozilla-aurora?
Attachment #8765610 - Flags: approval-mozilla-aurora+

Updated

2 years ago
Blocks: 1288751

Updated

2 years ago
Depends on: 1290089

Comment 15

2 years ago
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
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.