Closed
Bug 1282584
Opened 9 years ago
Closed 9 years ago
Potential problem with E10S preference handling in the child process
Categories
(Core :: General, defect)
Tracking
()
VERIFIED
FIXED
mozilla50
People
(Reporter: milan, Assigned: milan)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
jimm
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Summary: Potential problem with E10S preference handing in the child process → Potential problem with E10S preference handling in the child process
Updated•9 years ago
|
tracking-e10s:
--- → ?
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8765610 -
Flags: review?(jmathies) → review+
![]() |
||
Comment 5•9 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•9 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•9 years ago
|
Assignee | ||
Comment 7•9 years ago
|
||
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/
Assignee | ||
Comment 8•9 years ago
|
||
(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 | ||
Updated•9 years ago
|
Assignee: nobody → milan
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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 11•9 years ago
|
||
Milan, looks like we need to uplift this bug to fix bug 1288751. Could you fill the uplift request? Thanks
Flags: needinfo?(milan)
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
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
status-firefox48:
--- → fixed
status-firefox49:
--- → fixed
Comment 15•9 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
You need to log in
before you can comment on or make changes to this bug.
Description
•