Improve process launch handle sharing API.

Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship between this and handlers_to_inherit is clarified. Extra comments are added about inheritance of the standard handles.

The rest of this change is related fallout:

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were removed. These were previously added for Windows XP support when all handles needed to be shared (XP didn't support a list). When XP support was removed here: https://codereview.chromium.org/1801963002 the CHECK was retained even though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2950153002
Cr-Commit-Position: refs/heads/master@{#489181}
Committed: https://chromium.googlesource.com/chromium/src/+/3c98c7d3c8b37108ed1dc1a72ce5d3106eccb6c6

[brettw, 2017-06-21 19:44:27]

Description was changed from

==========
Improve process launch handle sharing API.

BUG=
==========

to

==========
Improve process launch handle sharing API.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[brettw, 2017-06-21 19:47:01]

Description was changed from

==========
Improve process launch handle sharing API.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Improve process launch handle sharing API.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[brettw, 2017-06-21 19:47:32]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 19:47:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 20:40:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 20:40:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[brettw, 2017-06-21 22:34:13]

Description was changed from

==========
Improve process launch handle sharing API.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Improve process launch handle sharing API.

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[brettw, 2017-06-21 22:38:23]

Description was changed from

==========
Improve process launch handle sharing API.

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change results in fallout from this change:

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[brettw, 2017-06-21 22:48:33]

Description was changed from

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change results in fallout from this change:

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change is related fallout:

Remove support for XP in browser_watcher.

Name base::HandleToInheritVector only defined on Posix. Rename
content::FileDescriptorInfo to content::PosixFileDesciptorInfo and make sure
it's only used on Posix (required by the base change). This class was already
mostly only used on Posix so the practical difference is very small except for
the name change.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[brettw, 2017-06-21 23:15:20]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 23:15:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 23:27:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 23:27:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-06-21 23:30:36]

Mojo fix

[brettw, 2017-06-21 23:30:38]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 23:30:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 23:44:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 23:44:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-22 00:31:26]

Fixes

[brettw, 2017-06-22 00:31:27]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 00:31:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 00:47:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 00:47:32]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-22 01:21:49]

Fixes

[brettw, 2017-06-22 01:21:50]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 01:22:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 01:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 01:37:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-22 01:58:48]

Test launcher

[brettw, 2017-06-22 01:58:48]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 01:59:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 02:20:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 02:20:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-06-26 17:53:20]

Services fix + merge

[brettw, 2017-06-26 17:53:21]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 17:53:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 18:18:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 18:18:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2017-06-26 18:29:04]

Mac

[brettw, 2017-06-26 18:29:05]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 18:29:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 18:55:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 18:55:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2017-06-26 20:50:43]

Fix

[brettw, 2017-06-26 20:50:44]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 20:51:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 20:54:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 20:54:42]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-26 21:00:29]

brettw@chromium.org changed reviewers:
+ wfh@chromium.org

[brettw, 2017-06-26 21:00:30]

Sorry, Justin suggested you for this.

Don't be scared by the number of files, since many of the changes are trivial.

However, the non-search-and-replace changes need careful review.

One option is to have a consistent name for the type and variable in
LaunchOptions. This would have saved a couple of ifdefs for code that just
passes through to another place. A couple of palces in the code like Mojo have
their own typedefs for this. But I was worried the behavior here might be too
different between platforms (and any time you use it you'll need an ifdef
anyway, since posix has a pair of ints, and Windows is a HANDLE. Let me know
what you think.

[grt (UTC plus 2), 2017-06-27 08:01:44]

a few drive-by nits from a quick surface scan of launch.h and launch_win. not
adding myself as a reviewer since these aren't earth shattering comments.

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h#...
base/process/launch.h:19: #include "base/optional.h"
unused?

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h#...
base/process/launch.h:84: enum InheritMode {
nit: enum class?

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h#...
base/process/launch.h:88: INHERIT_SPECIFIC,
nit: kFoo is now preferred
(https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5AqwVQt-Etg/dis...)

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch_wi...
File base/process/launch_win.cc (right):

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch_wi...
base/process/launch_win.cc:215: bool inherit_handles = options.inherit_mode ==
LaunchOptions::INHERIT_ALL;
optional nit: bool -> BOOL to match the type of the bInheritHandles arg to
CreateProcess

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch_wi...
base/process/launch_win.cc:218: DCHECK(options.inherit_mode ==
LaunchOptions::INHERIT_SPECIFIC);
nit: DCHECK_EQ

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch_wi...
base/process/launch_win.cc:247: inherit_handles = true;
optional nit: true -> TRUE

[Will Harris, 2017-06-27 09:26:49]

First impression, without even looking at this, can the CL be split into a
"non-behavior changing" CL, and a second "behavior changing" CL? Also, it would
be nice to have green trybots?

Otherwise/in addition, I can look later today, but this CL is yuuuge.

[brettw, 2017-06-28 21:29:12]

On 2017/06/27 09:26:49, Will Harris (UTC TZ) wrote:
> First impression, without even looking at this, can the CL be split into a
> "non-behavior changing" CL, and a second "behavior changing" CL? Also, it
would
> be nice to have green trybots?

The only behavior changing things are in chrome/test/base/mojo_test_connector.cc
and services/service_manager/runner/host/service_process_launcher.cc and both of
these are test code not in production, and the behavior is to not CHECK in
official builds. So it should be safe :) I needed to change these call sites
anyway.

[brettw, 2017-06-28 21:42:05]

Merge + grt review

[brettw, 2017-06-28 21:42:07]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2017-06-28 21:42:22]

New patch up.

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h#...
base/process/launch.h:19: #include "base/optional.h"
On 2017/06/27 08:01:44, grt (UTC plus 2) wrote:
> unused?

Oh yeah, Optional was my first attempt :/

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h#...
base/process/launch.h:84: enum InheritMode {
On 2017/06/27 08:01:44, grt (UTC plus 2) wrote:
> nit: enum class?

Done.

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch.h#...
base/process/launch.h:88: INHERIT_SPECIFIC,
On 2017/06/27 08:01:44, grt (UTC plus 2) wrote:
> nit: kFoo is now preferred
>
(https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5AqwVQt-Etg/dis...)

Done.

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch_wi...
File base/process/launch_win.cc (right):

https://codereview.chromium.org/2950153002/diff/160001/base/process/launch_wi...
base/process/launch_win.cc:215: bool inherit_handles = options.inherit_mode ==
LaunchOptions::INHERIT_ALL;
I like keeping this a bool. At some point there will have to be a conversion
from a bool to an int (BOOL), and I prefer to do this as late as possible since
it reduces some screwups.

[commit-bot: I haz the power, 2017-06-28 21:42:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 22:19:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 22:19:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-29 00:02:50]

Fixes

[brettw, 2017-06-29 00:02:51]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 00:03:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 00:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 00:46:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[brettw, 2017-06-29 04:04:26]

Fix

[brettw, 2017-06-29 04:04:27]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 04:04:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 04:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 04:59:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Joe Mason, 2017-07-04 18:26:52]

joenotcharles@chromium.org changed reviewers:
+ joenotcharles@chromium.org

[Joe Mason, 2017-07-04 18:26:56]

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h#...
base/process/launch.h:42: #elif defined(OS_FUCHSIA)
Is OS_POSIX also defined when OS_FUCHSIA is? (I can't imagine OS_WIN overlaps
with OS_FUCHSIA...)

If so, this #elif will never be hit. But if not I don't see how the "#else  //
!defined(OS_WIN)" below can work since FileHandleMappingVector is used in that
block.

[Joe Mason, 2017-07-04 18:30:35]

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h#...
base/process/launch.h:39: typedef std::vector<std::pair<int, int>>
FileHandleMappingVector;
From the CL desc: "Name base::HandleToInheritVector only defined on Posix."

That seems to be the opposite of what the code says here. HandlesToInheritVector
is NOT defined on POSIX, but FileHandleMappingVector IS.

[brettw, 2017-07-05 19:12:24]

Description was changed from

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change is related fallout:

Remove support for XP in browser_watcher.

Name base::HandleToInheritVector only defined on Posix. Rename
content::FileDescriptorInfo to content::PosixFileDesciptorInfo and make sure
it's only used on Posix (required by the base change). This class was already
mostly only used on Posix so the practical difference is very small except for
the name change.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change is related fallout:

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[brettw, 2017-07-05 19:13:31]

Fix Mojo launcher, review comments

[brettw, 2017-07-05 19:13:33]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 19:13:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-07-05 19:13:51]

Thanks, new snap up.

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h#...
base/process/launch.h:39: typedef std::vector<std::pair<int, int>>
FileHandleMappingVector;
Will update the CL description. I put the vector definition back after the
FUCHSIA thing landed in the middle of this patch.

https://codereview.chromium.org/2950153002/diff/220001/base/process/launch.h#...
base/process/launch.h:42: #elif defined(OS_FUCHSIA)
Looks like it is, I reordered.

[commit-bot: I haz the power, 2017-07-05 19:41:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 19:41:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[brettw, 2017-07-05 20:05:20]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 20:05:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 23:15:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 23:15:02]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[brettw, 2017-07-24 18:54:49]

brettw@chromium.org changed reviewers:
+ jschuh@chromium.org

[jschuh, 2017-07-24 19:41:27]

Lgtm on all the Windows stuff with some nits. I think you're inheriting with
kAll in the one spot I cited where you shouldn't need to, and please file a bug
on kAll inheritance and add the TODOs in the places I called out.

https://codereview.chromium.org/2950153002/diff/240001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2950153002/diff/240001/base/process/launch.h#...
base/process/launch.h:98: // over-sharing.
Please replace this with a warning that this is deprecated, should not be used
in any new code, and a pointer to a bug for fixing it.

https://codereview.chromium.org/2950153002/diff/240001/chrome/installer/util/...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/2950153002/diff/240001/chrome/installer/util/...
chrome/installer/util/user_experiment.cc:139: options.inherit_mode =
base::LaunchOptions::Inherit::kAll;
Leave a TODO here.

https://codereview.chromium.org/2950153002/diff/240001/chrome/installer/util/...
chrome/installer/util/user_experiment.cc:242: options.inherit_mode =
base::LaunchOptions::Inherit::kAll;
Leave a TODO here.

https://codereview.chromium.org/2950153002/diff/240001/components/browser_wat...
File components/browser_watcher/window_hang_monitor_win_unittest.cc (right):

https://codereview.chromium.org/2950153002/diff/240001/components/browser_wat...
components/browser_watcher/window_hang_monitor_win_unittest.cc:365:
options.inherit_mode = base::LaunchOptions::Inherit::kAll;
Leave a TODO here.

https://codereview.chromium.org/2950153002/diff/240001/mojo/edk/test/multipro...
File mojo/edk/test/multiprocess_test_helper.cc (right):

https://codereview.chromium.org/2950153002/diff/240001/mojo/edk/test/multipro...
mojo/edk/test/multiprocess_test_helper.cc:141: options.inherit_mode =
base::LaunchOptions::Inherit::kAll;
Why is this one not using the inheritance list?

https://codereview.chromium.org/2950153002/diff/240001/net/test/spawned_test_...
File net/test/spawned_test_server/local_test_server_win.cc (right):

https://codereview.chromium.org/2950153002/diff/240001/net/test/spawned_test_...
net/test/spawned_test_server/local_test_server_win.cc:128:
launch_options.inherit_mode = base::LaunchOptions::Inherit::kAll;
Leave a TODO here.

[brettw, 2017-07-24 23:35:12]

Review comments and merge

[brettw, 2017-07-24 23:35:13]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-07-24 23:35:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/2950153002/#ps260001
(title: "Review comments and merge")

[commit-bot: I haz the power, 2017-07-24 23:35:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-07-24 23:35:30]

https://codereview.chromium.org/2950153002/diff/240001/mojo/edk/test/multipro...
File mojo/edk/test/multiprocess_test_helper.cc (right):

https://codereview.chromium.org/2950153002/diff/240001/mojo/edk/test/multipro...
mojo/edk/test/multiprocess_test_helper.cc:141: options.inherit_mode =
base::LaunchOptions::Inherit::kAll;
On 2017/07/24 19:41:27, jschuh wrote:
> Why is this one not using the inheritance list?

Thanks!

[commit-bot: I haz the power, 2017-07-24 23:40:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-24 23:40:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-07-24 23:59:32]

Merge

[brettw, 2017-07-24 23:59:33]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-07-24 23:59:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/2950153002/#ps280001
(title: "Merge")

[commit-bot: I haz the power, 2017-07-24 23:59:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-25 01:44:23]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1500940773040960,
"parent_rev": "d0b99c320408e55020e11e76dac56597156074da", "commit_rev":
"3c98c7d3c8b37108ed1dc1a72ce5d3106eccb6c6"}

[commit-bot: I haz the power, 2017-07-25 01:44:38]

Description was changed from

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change is related fallout:

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Improve process launch handle sharing API.

Makes handles_to_inherit and fds_to_remap in base::LaunchOptions to be regular
data members instead of pointers. This simplifies all users of these objects.

On Windows, inherit_handles boolean is changed to an enum, and the relationship
between this and handlers_to_inherit is clarified. Extra comments are added
about inheritance of the standard handles.

The rest of this change is related fallout:

Remove support for XP in browser_watcher.

Some blocks of CHECK(false) that were inside OFFICIAL_BUILD blocks in Mojo were
removed. These were previously added for Windows XP support when all handles
needed to be shared (XP didn't support a list). When XP support was removed
here: https://codereview.chromium.org/1801963002 the CHECK was retained even
though it was explicitly set to only share a whitelist of handles.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2950153002
Cr-Commit-Position: refs/heads/master@{#489181}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c98c7d3c8b37108ed1dc1a72ce5...
==========

[commit-bot: I haz the power, 2017-07-25 01:44:41]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/3c98c7d3c8b37108ed1dc1a72ce5...