1401786 - Move base::LaunchApp options into a LaunchOptions struct

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P2)

Description

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

Upstream Chromium does something useful with the arguments to LaunchApp: aside from the command line, they're all in a LaunchOptions struct.  This makes it easier to deal with platform-specific options (most of them are) and default values than with positional parameters.

Specifically, for bug 1401062, I used this to add a callback to use in place of fork() on Linux, and also so that the entire LaunchOptions could be handed off as a unit to a function in the sandboxing code to both set that option and add fd mappings and environment variables if needed.  It's also useful if you want to set up the options on one thread (e.g., the main thread, where prefs live) and then pass them off to another thread to do the actual launch.

Also, I notice that upstream Chromium has a lot more options than we do, in particular on Windows; I don't understand the details enough to be sure, but this might help clean up the sandboxing integration, and/or generally be useful for breaking up the huge blocks of platform-specific magic in GeckoChildProcessHost.

I already have a patch, and it works out to be a small net reduction in code size, which is usually good.

Comment 1

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

This doesn't strictly depend on removing the unused launch options, but it'd be a waste to refactor code that's known to be effectively dead/unnecessary.

Comment 2

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

Attachment: Bug 1401786 - Move base::LaunchApp options into a LaunchOptions struct, like upstream Chromium.

Review commit: https://reviewboard.mozilla.org/r/195698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195698/

Comment 3

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

Attachment: Bug 1401786 - Move the Linux sandboxing parts of GeckoChildProcessHost into security/sandbox.

Review commit: https://reviewboard.mozilla.org/r/195700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195700/

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8924411 [details]
Bug 1401786 - Move the Linux sandboxing parts of GeckoChildProcessHost into security/sandbox.

https://reviewboard.mozilla.org/r/195700/#review201066

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8924410 [details]
Bug 1401786 - Move base::LaunchApp options into a LaunchOptions struct, like upstream Chromium.

https://reviewboard.mozilla.org/r/195698/#review203888

::: ipc/chromium/src/base/process_util.h
(Diff revision 1)
> -// Before launching all FDs open in the parent process will be marked as
> -// close-on-exec.  |fds_to_remap| defines a mapping of src fd->dest fd to

We seem to have lose the close-on-exec part of the comment. Is that intentional?

Comment 6

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

(In reply to Bill McCloskey (:billm) from comment #5)
> ::: ipc/chromium/src/base/process_util.h
> (Diff revision 1)
> > -// Before launching all FDs open in the parent process will be marked as
> > -// close-on-exec.  |fds_to_remap| defines a mapping of src fd->dest fd to
> 
> We seem to have lose the close-on-exec part of the comment. Is that
> intentional?

Yes.  The behavior it's documenting is a bug; it was fixed on Linux in 2009 (https://codereview.chromium.org/100127), fixed on Mac recently (bug 1400061), and will be fixed on BSD (bug 1400051) as soon as I can get a working Rust toolchain on some BSD flavor or one of the BSD port maintainers picks it up, whichever comes first.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f53d4e3b5635
Move base::LaunchApp options into a LaunchOptions struct, like upstream Chromium. r=billm
https://hg.mozilla.org/integration/autoland/rev/d97cb5ef7531
Move the Linux sandboxing parts of GeckoChildProcessHost into security/sandbox. r=gcp

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f53d4e3b5635
https://hg.mozilla.org/mozilla-central/rev/d97cb5ef7531