Closed Bug 1401786 Opened 2 years ago Closed 2 years ago

Move base::LaunchApp options into a LaunchOptions struct

Categories

(Core :: IPC, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: sb+)

Attachments

(2 files)

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.
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.
Depends on: 1316153, 1401790
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Whiteboard: sb+
Priority: P3 → P2
Comment on attachment 8924411 [details]
Bug 1401786 - Move the Linux sandboxing parts of GeckoChildProcessHost into security/sandbox.

https://reviewboard.mozilla.org/r/195700/#review201066
Attachment #8924411 - Flags: review?(gpascutto) → review+
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?
Attachment #8924410 - Flags: review?(wmccloskey) → review+
(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.
Pushed by jedavis@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f53d4e3b5635
https://hg.mozilla.org/mozilla-central/rev/d97cb5ef7531
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.