1487287 - Move process launch off the I/O thread and into a thread pool

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Process launch can take nontrivial amounts of time, for reasons that aren't all under our control, which is why we're trying to move it off the main thread for responsiveness.  However, the parent process I/O thread mediates all IPDL communication with child processes, so blocking it is also not ideal.

It looks like what actually needs to run on the I/O thread is, at most, the creation of the channel; the launch itself should be able to run on any thread.

This doesn't need its own thread pool or dedicated threads, just a place to send runnables that isn't in contention with anything important, but giving this its own thread pool may be the easiest thing to do with the infrastructure we currently have.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Actually, on Windows we do need a dedicated thread, and specifically *a* thread: https://searchfox.org/mozilla-central/rev/43969de34f5d4b113133d090f024e5eed7a82af0/security/sandbox/chromium/sandbox/win/src/broker_services.cc#323

(Whereas on Mac, we'll probably want thread pool size >1 because of the Mach IPC setup.)

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Also, AutoSetProfilerEnvVarsForChildProcess will break if there are overlapping launches on multiple threads (and in general it's preferred to pass strings to LaunchApp instead of side-effecting the parent process).

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1487287 - Set profiler env vars in child processes without side-effecting the parent process.

We can directly set environment variables for the child process on
all platforms now, instead of changing the parent's environment and
inheriting the changes.  This simplifies memory management, but more
importantly it's necessary for thread safety to allow launching
processes from a thread pool.

Depends on D8944

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1487287 - Move child process launch off the I/O thread.

Launching processes takes enough time that we should avoid blocking the
parent process's IPC I/O thread for it; it's less bad for responsiveness
than blocking the main thread, but it's not good.

On Windows we need to use a dedicated thread, because the sandbox isn't
thread-safe and it asserts that the same thread is used for every
launch.  Otherwise, a thread pool is used.

Depends on D8945

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fa5e81ad801
Set profiler env vars in child processes without side-effecting the parent process. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8b1f88d7bfeb
Move child process launch off the I/O thread. r=mccr8

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

This was backed out as part of the big async launch patch stack, and I believe it's responsible for both of the failures that were seen:

1. This means that the I/O thread can be woken up for OnChannel{Connected,Error} before the launch thread has finished the launch and set the state to PROCESS_CREATED.  I added some assertions about the state in bug 1446161, inspired by review comments, which caught this.

2. Web Replay launches content processes from the middleman process, which doesn't initialize SharedThreadPool.  This should be simple to fix.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Also, RegisterChildCrashAnnotationFileDescriptor isn't thread-safe.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1487287 - Fix thread-safety of crash reporter pid map.

Depends on D8945

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

https://phabricator.services.mozilla.com/D8946 needs re-review for the changes I made; I tried to set that in Phabricator but I don't know how well it worked.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e818109ab4e1
Set profiler env vars in child processes without side-effecting the parent process. r=mstange
https://hg.mozilla.org/integration/autoland/rev/5a5e1801bb36
Fix thread-safety of crash reporter pid map. r=gsvelto

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0cf88b1fe5b
Move child process launch off the I/O thread. r=mccr8

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e818109ab4e1
https://hg.mozilla.org/mozilla-central/rev/5a5e1801bb36
https://hg.mozilla.org/mozilla-central/rev/a0cf88b1fe5b

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Backed out the main patch (D8946) for causing bug 1519145.

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/191057a8eccf
Backed out changeset a0cf88b1fe5b

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Bug 1487287 - Synchronize GeckoChildProcessHost destruction with launching.

In order to enable asynchronous launch, destruction of
GeckoChildProcessHost (and its subclasses) has to be delayed until after
launching (or anything else that might be made asynchronous in the
future) has completed, to prevent use-after-free. However, there are
other dependencies on process hosts always being destroyed on the I/O
thread, so refcounting would be difficult to use.

Instead, GeckoChildProcessHost now may not be destroyed directly, but
must go through a method that handles the scheduling.

There are also some minor cleanups to the affected headers (removed
duplicate access modifiers, and made PluginProcessParent final).

Depends on D18010

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/242953b250a5
Synchronize GeckoChildProcessHost destruction with launching. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/073aa330041c
Move child process launch off the I/O thread. r=mccr8

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/242953b250a5
https://hg.mozilla.org/mozilla-central/rev/073aa330041c

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Are you intending uplift to beta 66? I notice the target milestone is set to 66 here though this landed on 67 nightly. Maybe a little complicated for uplift though.

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The flags probably got set like that because this got backed out twice and needed some time to fix. It's a risky change, and it's for a performance improvement that's probably not significant pre-Fission (and not well quantified in general), so no uplift.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Perfect, that's what I like to hear right now in beta. Thanks.