Preallocated content process is launching before first paint

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: mconley, Assigned: gkrizsanits)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

Here's a slice from a start-up profile:

https://perfht.ml/2sj3xwY

The preallocation of the content process is occurring before first paint. We should probably defer that until sometime after first paint - perhaps done when the user is idle.
(Reporter)

Updated

2 years ago
Whiteboard: [qf]
(Assignee)

Comment 1

2 years ago
Right, I think I've made a mistake. So the intention was to do something like that:
http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/dom/ipc/ContentParent.cpp#2719

but then I seem to forgot to prevent PreallocatedProcessManagerImpl::Enable() to launch it earlier. I can take this.
Assignee: nobody → gkrizsanits

Comment 2

2 years ago
Bug 1348361 seems related too...
See Also: → 1348361

Updated

2 years ago
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 4

2 years ago
I've added some description in the patch comment. I think all the test failures from the last try run are known intermittents, and it was just an unlucky push. We'll see...
Attachment #8881827 - Flags: review?(mrbkap)
Comment on attachment 8881827 [details] [diff] [review]
ppm blocker. v1

Review of attachment 8881827 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/PreallocatedProcessManager.cpp
@@ +67,5 @@
>  
>    bool mEnabled;
>    bool mShutdown;
>    RefPtr<ContentParent> mPreallocatedProcess;
> +  std::set<uint64_t> mBlockers;

This should probably use a Mozilla datastructure.
Attachment #8881827 - Flags: review?(mrbkap) → review+

Comment 6

2 years ago
Pushed by gkrizsanits@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa05515c1c87
Block the preallocated process manager while a content process is being launched.
Backed out for frequently timing out in toolkit/components/extensions/test/xpcshell/test_ext_i18n.js on OS X 10.10 opt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7efddd72ba59c19ab21efc7b21c57cd13e123aa7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7efddd72ba59c19ab21efc7b21c57cd13e123aa7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=xpcshell+10.10
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110891540&repo=mozilla-inbound

15:35:19     INFO -  (xpcshell/head.js) | test run_next_test 2 finished (2)
15:35:19     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "DataError: Data provided to an operation does not meet requirements."]
15:35:19     INFO -  wrapMethods/cls.prototype[method]@resource://gre/modules/IndexedDB.jsm:103:47
15:35:19     INFO -  get@resource://gre/modules/ExtensionParent.jsm:1341:22
15:35:19     INFO -  async*parseManifest@resource://gre/modules/Extension.jsm:873:12
15:35:19     INFO -  loadManifest@resource://gre/modules/Extension.jsm:543:7
15:35:19     INFO -  async*loadManifest@resource://gre/modules/Extension.jsm:878:12
15:35:19     INFO -  _startup@resource://gre/modules/Extension.jsm:1055:42
15:35:19     INFO -  async*startup@resource://gre/modules/Extension.jsm:1031:27
15:35:19     INFO -  startup@resource://testing-common/ExtensionXPCShellUtils.jsm:281:27
15:35:19     INFO -  test_get_ui_language@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/tests/toolkit/components/extensions/test/xpcshell/test_ext_i18n.js:290:9
15:35:19     INFO -  async*asyncFunction@resource://gre/modules/Task.jsm:241:18
15:35:19     INFO -  Task_spawn@resource://gre/modules/Task.jsm:166:12
15:35:19     INFO -  _run_next_test@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:1544:9
15:35:19     INFO -  run@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:706:9
15:35:19     INFO -  _do_main@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:220:3
15:35:19     INFO -  _execute_test@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:549:5
15:35:19     INFO -  @-e:1:1
15:35:19     INFO -  "
15:35:19     INFO -  TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n.js | test_get_ui_language - [test_get_ui_language : 243] Got expected getUILanguage result in background - Expected: en_US, Actual: en_US - true == true
15:35:19     INFO -  TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n.js | test_get_ui_language - [test_get_ui_language : 243] Got expected getMessage result in background - Expected: en_US, Actual: en_US - true == true
15:35:19     INFO -  <<<<<<<
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 8

2 years ago
The failure you linked is in non-e10s mode where my code is inactive so I doubt it's related. And the backout did not seem to fix the problem. Was this the only reason for the backout? This is an important patch so if there isn't any other reason for the backout I'm going to repush it.
Flags: needinfo?(gkrizsanits) → needinfo?(aryx.bugmail)
This seems to have been unrelated, please reland it.
Flags: needinfo?(aryx.bugmail)

Comment 10

2 years ago
Pushed by gkrizsanits@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbcc5752dea8
Block the preallocated process manager while a content process is being launched. r=mrbkap

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbcc5752dea8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1380385
(Assignee)

Comment 12

2 years ago
Comment on attachment 8881827 [details] [diff] [review]
ppm blocker. v1

Approval Request Comment
[Feature/Bug causing the regression]: It's not a regression. It's an optimization on the preallocated process manager.

[User impact if declined]: We might start the preallocated content process at the wrong time, delaying firstpaint.

[Is this code covered by automated tests?]: The code is used by all browser tests, and some other test suits, so it's heavily tested. There is also a probe related to this feature, that would show regression if something were not OK. But it looks OK.

[Has the fix been verified in Nightly?]: Yes.

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Low risk.

[Why is the change risky/not risky?]: The patch does not do anything new, it just makes sure that we're more careful about when we start up the preallocated process. 

[String changes made/needed]: None.
Attachment #8881827 - Flags: approval-mozilla-beta?
(Assignee)

Comment 13

2 years ago
(Assignee)

Comment 14

2 years ago
This is the patch we talked about during the e10s-multi meeting.
Flags: needinfo?(lhenry)
Can you elaborate on the need for this patch in 55, for someone who was not at that meeting?
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 16

2 years ago
Let's just put this request on hold. I want to investigate Bug 1368036, which seems to be related.
Flags: needinfo?(lhenry)
Flags: needinfo?(gkrizsanits)
(Assignee)

Updated

2 years ago
Attachment #8881827 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.