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)

Unspecified
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
e10s + ---
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- verified

People

(Reporter: milan, Assigned: milan)

References

Details

Attachments

(1 file)

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.
Summary: Potential problem with E10S preference handing in the child process → Potential problem with E10S preference handling in the child process
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?
(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.
Attachment #8765610 - Flags: review?(jmathies) → review+
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.
(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 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
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
Status: NEW → RESOLVED
Closed: 9 years ago
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+
Blocks: 1288751
Depends on: 1290089
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.

Attachment

General

Created:
Updated:
Size: