Fix field trials not working in utility processes.

Fix field trials not working in utility processes.

The simple change in utility_process_host_impl.cc wasn't enough, since utility processes can be launched elevated on Windows. In that cause, handle inheritence doesn't work since base::LaunchElevatedProcess doesn't (and can't) handle this. However, since the elevated process is by definition not sandboxed, have it duplicated the shared memory handle manually.

BUG=598073
Review-Url: https://codereview.chromium.org/2961333002
Cr-Commit-Position: refs/heads/master@{#483727}
Committed: https://chromium.googlesource.com/chromium/src/+/b18e53ad3d63f5c36792d7025600bc9b47c6f5d7

[jam, 2017-06-29 20:55:05]

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

[commit-bot: I haz the power, 2017-06-29 20:55:29]

Dry run: CQ is trying da patch.

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

[jam, 2017-06-29 22:00:08]

jam@chromium.org changed reviewers:
+ asvitkine@chromium.org, sergeyu@chromium.org

[jam, 2017-06-29 22:00:09]

sergeyu: remoting
asvitkine: the rest

[jam, 2017-06-29 22:00:40]

btw this was exposed by https://codereview.chromium.org/2960703003/

[commit-bot: I haz the power, 2017-06-29 22:31:24]

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

[commit-bot: I haz the power, 2017-06-29 22:31:25]

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

[jam, 2017-06-29 22:41:27]

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

[commit-bot: I haz the power, 2017-06-29 22:41:54]

Dry run: CQ is trying da patch.

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

[Sergey Ulanov, 2017-06-29 23:57:23]

remoting lgtm

https://codereview.chromium.org/2961333002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/2961333002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_main.cc:63: #endif
 // defined(OS_WIN) && defined(OFFICIAL_BUILD)

[commit-bot: I haz the power, 2017-06-30 00:13:04]

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

[commit-bot: I haz the power, 2017-06-30 00:13:07]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-30 05:19:27]

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

[commit-bot: I haz the power, 2017-06-30 05:19:36]

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-30 06:40:54]

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

[commit-bot: I haz the power, 2017-06-30 06:40:55]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-30 14:39:58]

jam@chromium.org changed reviewers:
+ thestig@chromium.org

[jam, 2017-06-30 14:39:59]

+thestig for base/process

https://codereview.chromium.org/2961333002/diff/20001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/2961333002/diff/20001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_main.cc:63: #endif
On 2017/06/29 23:57:23, Sergey Ulanov wrote:
>  // defined(OS_WIN) && defined(OFFICIAL_BUILD)

Done.

[Alexei Svitkine (slow), 2017-06-30 14:53:59]

lgtm

[(unused - use chromium), 2017-06-30 16:15:17]

thakis@google.com changed reviewers:
+ thakis@google.com

[(unused - use chromium), 2017-06-30 16:15:18]

base/process_info lgtm, some soapboxing below.

https://codereview.chromium.org/2961333002/diff/40001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2961333002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:1156: }
Having OS-dependent elevantion-level-dependent code in base/metrics seems a tad
questionable to me, but I guess a base/metrics/OWNER already approved this
part.. :-P

https://codereview.chromium.org/2961333002/diff/40001/base/process/process_in...
File base/process/process_info_win.cc (right):

https://codereview.chromium.org/2961333002/diff/40001/base/process/process_in...
base/process/process_info_win.cc:18: HANDLE GetCurrentProcessToken() {
We should probably make ScopedHandle movable and return ScopedHandles from
functions like this.

[jam, 2017-06-30 16:15:42]

The CQ bit was checked by jam@chromium.org

[jam, 2017-06-30 16:15:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2961333002/#ps40001
(title: "review comment")

[commit-bot: I haz the power, 2017-06-30 16:15:54]

CQ is trying da patch.

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

[jam, 2017-06-30 16:16:02]

Description was changed from

==========
Fix field trials not working in utility processes.

The simple change in utility_process_host_impl.cc wasn't enough, since utility
processes can be launched elevated on Windows. In that cause, handle inheritence
doesn't work since base::LaunchElevatedProcess doesn't (and can't) handle this.
However, since the elevated process is by definition not sandboxed, have it
duplicated the shared memory handle manually.

BUG=598073
==========

to

==========
Fix field trials not working in utility processes.

The simple change in utility_process_host_impl.cc wasn't enough, since utility
processes can be launched elevated on Windows. In that cause, handle inheritence
doesn't work since base::LaunchElevatedProcess doesn't (and can't) handle this.
However, since the elevated process is by definition not sandboxed, have it
duplicated the shared memory handle manually.

BUG=598073
==========

[jam, 2017-06-30 16:16:02]

jam@chromium.org changed reviewers:
- thestig@chromium.org

[jam, 2017-06-30 16:16:49]

https://codereview.chromium.org/2961333002/diff/40001/base/process/process_in...
File base/process/process_info_win.cc (right):

https://codereview.chromium.org/2961333002/diff/40001/base/process/process_in...
base/process/process_info_win.cc:18: HANDLE GetCurrentProcessToken() {
On 2017/06/30 16:15:18, (unused - use chromium) wrote:
> We should probably make ScopedHandle movable and return ScopedHandles from
> functions like this.

yep agreed, i checked first to see if it's moveable but as you say it's not

[Alexei Svitkine (slow), 2017-06-30 16:20:37]

https://codereview.chromium.org/2961333002/diff/40001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2961333002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:1156: }
On 2017/06/30 16:15:18, (unused - use chromium) wrote:
> Having OS-dependent elevantion-level-dependent code in base/metrics seems a
tad
> questionable to me, but I guess a base/metrics/OWNER already approved this
> part.. :-P

I agree this is not ideal. But I don't have a suggestion of how this could be
done better. Do you have a suggestion?

[chromium-reviews, 2017-06-30 16:21:58]

On Fri, Jun 30, 2017 at 12:20 PM, <asvitkine@chromium.org> wrote:

>
> https://codereview.chromium.org/2961333002/diff/40001/
> base/metrics/field_trial.cc
> File base/metrics/field_trial.cc (right):
>
> https://codereview.chromium.org/2961333002/diff/40001/
> base/metrics/field_trial.cc#newcode1156
> base/metrics/field_trial.cc:1156: }
> On 2017/06/30 16:15:18, (unused - use chromium) wrote:
> > Having OS-dependent elevantion-level-dependent code in base/metrics
> seems a tad
> > questionable to me, but I guess a base/metrics/OWNER already approved
> this
> > part.. :-P
>
> I agree this is not ideal. But I don't have a suggestion of how this
> could be done better. Do you have a suggestion?
>

No. (I only read the base/ part of this CL and didn't try to understand the
overall picture.)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-06-30 16:28:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498839342199780,
"parent_rev": "0304a310bb32a007c09041384cd58f5a38d18e47", "commit_rev":
"b18e53ad3d63f5c36792d7025600bc9b47c6f5d7"}

[commit-bot: I haz the power, 2017-06-30 16:28:45]

Description was changed from

==========
Fix field trials not working in utility processes.

The simple change in utility_process_host_impl.cc wasn't enough, since utility
processes can be launched elevated on Windows. In that cause, handle inheritence
doesn't work since base::LaunchElevatedProcess doesn't (and can't) handle this.
However, since the elevated process is by definition not sandboxed, have it
duplicated the shared memory handle manually.

BUG=598073
==========

to

==========
Fix field trials not working in utility processes.

The simple change in utility_process_host_impl.cc wasn't enough, since utility
processes can be launched elevated on Windows. In that cause, handle inheritence
doesn't work since base::LaunchElevatedProcess doesn't (and can't) handle this.
However, since the elevated process is by definition not sandboxed, have it
duplicated the shared memory handle manually.

BUG=598073

Review-Url: https://codereview.chromium.org/2961333002
Cr-Commit-Position: refs/heads/master@{#483727}
Committed:
https://chromium.googlesource.com/chromium/src/+/b18e53ad3d63f5c36792d7025600...
==========

[commit-bot: I haz the power, 2017-06-30 16:28:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b18e53ad3d63f5c36792d7025600...