Add enrolled to domain bit to crash keys in renderer process.

Add enrolled to domain bit to crash keys in renderer process.

This is a follow up to 2514483002 which adds this flag to the browser process.

BUG=660868
Review-Url: https://codereview.chromium.org/2618623003
Cr-Commit-Position: refs/heads/master@{#442690}
Committed: https://chromium.googlesource.com/chromium/src/+/80353b52b8d21e3481f6e1b69eec86f3399bc5cf

[Georges Khalil, 2017-01-05 21:37:21]

Patchset #2 (id:20001) has been deleted

[Georges Khalil, 2017-01-05 21:37:26]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2017-01-05 21:40:27]

Patchset #1 (id:40001) has been deleted

[Georges Khalil, 2017-01-05 21:40:59]

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

[commit-bot: I haz the power, 2017-01-05 21:41:24]

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-01-05 21:52:01]

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

[commit-bot: I haz the power, 2017-01-05 21:52:03]

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...)

[Georges Khalil, 2017-01-06 14:37:12]

Patchset #1 (id:60001) has been deleted

[Georges Khalil, 2017-01-06 14:37:36]

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

[commit-bot: I haz the power, 2017-01-06 14:37:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Georges Khalil, 2017-01-06 15:25:44]

Description was changed from

==========
Add enrolled to domain bit to crash keys in renderer process.

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

to

==========
Add enrolled to domain bit to crash keys in renderer process.

This is a follow up to 2514483002 which adds this flag to the browser process.

BUG=660868
==========

[Georges Khalil, 2017-01-06 15:26:28]

georgesak@chromium.org changed reviewers:
+ esprehn@chromium.org

[Georges Khalil, 2017-01-06 15:26:29]

PTAL.

[commit-bot: I haz the power, 2017-01-06 15:32:00]

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

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

Dry run: This issue passed the CQ dry run.

[esprehn, 2017-01-06 21:12:38]

esprehn@chromium.org changed reviewers:
+ nasko@chromium.org

[esprehn, 2017-01-06 21:12:38]

lgtm, but I think you want nasko@ or jam@ to look at this too.

[esprehn, 2017-01-06 21:14:01]

On 2017/01/06 at 21:12:38, esprehn wrote:
> lgtm, but I think you want nasko@ or jam@ to look at this too.

Actually why is this specific to windows too? We have enterprise management on
CrOS as well. I'm surprised there isn't a generic "IsEnterpriseManaged()"
somewhere that covers both Windows and CrOS. Then you could filter by OS in the
crash reports for just windows.

[Georges Khalil, 2017-01-08 22:24:32]

On 2017/01/06 21:14:01, esprehn wrote:
> On 2017/01/06 at 21:12:38, esprehn wrote:
> > lgtm, but I think you want nasko@ or jam@ to look at this too.
> 
> Actually why is this specific to windows too? We have enterprise management on
> CrOS as well. I'm surprised there isn't a generic "IsEnterpriseManaged()"
> somewhere that covers both Windows and CrOS. Then you could filter by OS in
the
> crash reports for just windows.

Thanks for the review.

There's no generic function, unfortunately. We're only targeting Windows as a
start, but I'll expand it to CrOS as well. I also opened a bug for creating a
common API (crbug.com/679221).

nasko@, PTAL?

[nasko, 2017-01-09 23:12:35]

https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
content/renderer/render_process_impl.cc:168: base::win::IsEnrolledToDomain() ?
"yes" : "no");
Shouldn't this be logged only once per Chrome instance? I don't think it makes
sense to log it for each renderer process, as domain membership changes on
Windows require reboot, therefore the probability of having two renderer process
be started across change in domain members is very low.

Also, don't we want to annotate browser process crashes with this info as well?
If so, this is much later than we'd like it to be. Why not look at the browser
process initialization and capture it there?

[Georges Khalil, 2017-01-10 14:49:51]

On 2017/01/09 23:12:35, nasko wrote:
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> File content/renderer/render_process_impl.cc (right):
> 
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> content/renderer/render_process_impl.cc:168: base::win::IsEnrolledToDomain() ?
> "yes" : "no");
> Shouldn't this be logged only once per Chrome instance? I don't think it makes
> sense to log it for each renderer process, as domain membership changes on
> Windows require reboot, therefore the probability of having two renderer
process
> be started across change in domain members is very low.
> 
> Also, don't we want to annotate browser process crashes with this info as
well?
> If so, this is much later than we'd like it to be. Why not look at the browser
> process initialization and capture it there?

[Georges Khalil, 2017-01-10 14:52:25]

On 2017/01/10 14:49:51, Georges Khalil wrote:
> On 2017/01/09 23:12:35, nasko wrote:
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > File content/renderer/render_process_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > content/renderer/render_process_impl.cc:168: base::win::IsEnrolledToDomain()
?
> > "yes" : "no");
> > Shouldn't this be logged only once per Chrome instance? I don't think it
makes
> > sense to log it for each renderer process, as domain membership changes on
> > Windows require reboot, therefore the probability of having two renderer
> process
> > be started across change in domain members is very low.
> > 
> > Also, don't we want to annotate browser process crashes with this info as
> well?
> > If so, this is much later than we'd like it to be. Why not look at the
browser
> > process initialization and capture it there?

We do already log it in the browser process:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_win.c...

However, this doesn't seem to propagate to the renderer processes. From my
understanding, it needs to be added here as well, as each process sets those
bits independently. If there's a way to set it once and it propagates to all
processes, then I'll be glad to use that instead.

[nasko, 2017-01-10 15:25:15]

On 2017/01/10 14:52:25, Georges Khalil wrote:
> On 2017/01/10 14:49:51, Georges Khalil wrote:
> > On 2017/01/09 23:12:35, nasko wrote:
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > File content/renderer/render_process_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > content/renderer/render_process_impl.cc:168:
base::win::IsEnrolledToDomain()
> ?
> > > "yes" : "no");
> > > Shouldn't this be logged only once per Chrome instance? I don't think it
> makes
> > > sense to log it for each renderer process, as domain membership changes on
> > > Windows require reboot, therefore the probability of having two renderer
> > process
> > > be started across change in domain members is very low.
> > > 
> > > Also, don't we want to annotate browser process crashes with this info as
> > well?
> > > If so, this is much later than we'd like it to be. Why not look at the
> browser
> > > process initialization and capture it there?
> 
> We do already log it in the browser process:
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_win.c...
> 
> However, this doesn't seem to propagate to the renderer processes. From my
> understanding, it needs to be added here as well, as each process sets those
> bits independently. If there's a way to set it once and it propagates to all
> processes, then I'll be glad to use that instead.

That makes sense. Does the method work successfully in the renderer process? I
couldn't find its implementation to check :(. I would imagine we don't allow the
Win32 API calls needed to determine the state, so hopefully it is passed from
the browser to the renderer somehow. Any pointers?

[Georges Khalil, 2017-01-10 18:27:24]

On 2017/01/10 15:25:15, nasko wrote:
> On 2017/01/10 14:52:25, Georges Khalil wrote:
> > On 2017/01/10 14:49:51, Georges Khalil wrote:
> > > On 2017/01/09 23:12:35, nasko wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > > File content/renderer/render_process_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > > content/renderer/render_process_impl.cc:168:
> base::win::IsEnrolledToDomain()
> > ?
> > > > "yes" : "no");
> > > > Shouldn't this be logged only once per Chrome instance? I don't think it
> > makes
> > > > sense to log it for each renderer process, as domain membership changes
on
> > > > Windows require reboot, therefore the probability of having two renderer
> > > process
> > > > be started across change in domain members is very low.
> > > > 
> > > > Also, don't we want to annotate browser process crashes with this info
as
> > > well?
> > > > If so, this is much later than we'd like it to be. Why not look at the
> > browser
> > > > process initialization and capture it there?
> > 
> > We do already log it in the browser process:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_win.c...
> > 
> > However, this doesn't seem to propagate to the renderer processes. From my
> > understanding, it needs to be added here as well, as each process sets those
> > bits independently. If there's a way to set it once and it propagates to all
> > processes, then I'll be glad to use that instead.
> 
> That makes sense. Does the method work successfully in the renderer process? I
> couldn't find its implementation to check :(. I would imagine we don't allow
the
> Win32 API calls needed to determine the state, so hopefully it is passed from
> the browser to the renderer somehow. Any pointers?

Yeah it works, I have tested it :)

IsEnrolledToDomain calls the Win32 API IsOS(OS_DOMAINMEMBER), which works fine
in our sandbox, from the tests I did (we have VMs setup with domain controllers
for this type of testing).

We could pass it from the browser to the renderers, but that API call is very
quick, I don't think it's worth it.

WDYT?

[nasko, 2017-01-10 18:43:58]

On 2017/01/10 18:27:24, Georges Khalil wrote:
> On 2017/01/10 15:25:15, nasko wrote:
> > On 2017/01/10 14:52:25, Georges Khalil wrote:
> > > On 2017/01/10 14:49:51, Georges Khalil wrote:
> > > > On 2017/01/09 23:12:35, nasko wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > > > File content/renderer/render_process_impl.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > > > content/renderer/render_process_impl.cc:168:
> > base::win::IsEnrolledToDomain()
> > > ?
> > > > > "yes" : "no");
> > > > > Shouldn't this be logged only once per Chrome instance? I don't think
it
> > > makes
> > > > > sense to log it for each renderer process, as domain membership
changes
> on
> > > > > Windows require reboot, therefore the probability of having two
renderer
> > > > process
> > > > > be started across change in domain members is very low.
> > > > > 
> > > > > Also, don't we want to annotate browser process crashes with this info
> as
> > > > well?
> > > > > If so, this is much later than we'd like it to be. Why not look at the
> > > browser
> > > > > process initialization and capture it there?
> > > 
> > > We do already log it in the browser process:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_win.c...
> > > 
> > > However, this doesn't seem to propagate to the renderer processes. From my
> > > understanding, it needs to be added here as well, as each process sets
those
> > > bits independently. If there's a way to set it once and it propagates to
all
> > > processes, then I'll be glad to use that instead.
> > 
> > That makes sense. Does the method work successfully in the renderer process?
I
> > couldn't find its implementation to check :(. I would imagine we don't allow
> the
> > Win32 API calls needed to determine the state, so hopefully it is passed
from
> > the browser to the renderer somehow. Any pointers?
> 
> Yeah it works, I have tested it :)

Awesome! Thanks for confirming!
 
> IsEnrolledToDomain calls the Win32 API IsOS(OS_DOMAINMEMBER), which works fine
> in our sandbox, from the tests I did (we have VMs setup with domain
controllers
> for this type of testing).

Sweet! This is great news!

> We could pass it from the browser to the renderers, but that API call is very
> quick, I don't think it's worth it.
> 
> WDYT?

No need to make it more complex. As long as the API works in the renderer, it is
fine. Is there any test that will fail if it starts failing to work inside the
sandbox? It will be ideal to have one.

LGTM

[Georges Khalil, 2017-01-10 19:09:43]

On 2017/01/10 18:43:58, nasko wrote:
> On 2017/01/10 18:27:24, Georges Khalil wrote:
> > On 2017/01/10 15:25:15, nasko wrote:
> > > On 2017/01/10 14:52:25, Georges Khalil wrote:
> > > > On 2017/01/10 14:49:51, Georges Khalil wrote:
> > > > > On 2017/01/09 23:12:35, nasko wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > > > > File content/renderer/render_process_impl.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2618623003/diff/80001/content/renderer/render...
> > > > > > content/renderer/render_process_impl.cc:168:
> > > base::win::IsEnrolledToDomain()
> > > > ?
> > > > > > "yes" : "no");
> > > > > > Shouldn't this be logged only once per Chrome instance? I don't
think
> it
> > > > makes
> > > > > > sense to log it for each renderer process, as domain membership
> changes
> > on
> > > > > > Windows require reboot, therefore the probability of having two
> renderer
> > > > > process
> > > > > > be started across change in domain members is very low.
> > > > > > 
> > > > > > Also, don't we want to annotate browser process crashes with this
info
> > as
> > > > > well?
> > > > > > If so, this is much later than we'd like it to be. Why not look at
the
> > > > browser
> > > > > > process initialization and capture it there?
> > > > 
> > > > We do already log it in the browser process:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_win.c...
> > > > 
> > > > However, this doesn't seem to propagate to the renderer processes. From
my
> > > > understanding, it needs to be added here as well, as each process sets
> those
> > > > bits independently. If there's a way to set it once and it propagates to
> all
> > > > processes, then I'll be glad to use that instead.
> > > 
> > > That makes sense. Does the method work successfully in the renderer
process?
> I
> > > couldn't find its implementation to check :(. I would imagine we don't
allow
> > the
> > > Win32 API calls needed to determine the state, so hopefully it is passed
> from
> > > the browser to the renderer somehow. Any pointers?
> > 
> > Yeah it works, I have tested it :)
> 
> Awesome! Thanks for confirming!
>  
> > IsEnrolledToDomain calls the Win32 API IsOS(OS_DOMAINMEMBER), which works
fine
> > in our sandbox, from the tests I did (we have VMs setup with domain
> controllers
> > for this type of testing).
> 
> Sweet! This is great news!
> 
> > We could pass it from the browser to the renderers, but that API call is
very
> > quick, I don't think it's worth it.
> > 
> > WDYT?
> 
> No need to make it more complex. As long as the API works in the renderer, it
is
> fine. Is there any test that will fail if it starts failing to work inside the
> sandbox? It will be ideal to have one.
> 
> LGTM

I would love to have proper testing, but this will require running in the proper
environment (mocking that function is useless). It is something we are planning
on doing hopefully by Q2 (test bots simulating corporate environments), so stay
tuned!

Thanks for the review, sending to CQ.

[Georges Khalil, 2017-01-10 19:09:51]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2017-01-10 19:10:12]

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-01-10 20:36:57]

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

[commit-bot: I haz the power, 2017-01-10 20:36:58]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Georges Khalil, 2017-01-10 20:42:12]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2017-01-10 20:43:17]

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-01-10 21:18:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484080932876390,
"parent_rev": "c21db481b563e7a38dc6820bc84fca1db10873d4", "commit_rev":
"80353b52b8d21e3481f6e1b69eec86f3399bc5cf"}

[commit-bot: I haz the power, 2017-01-10 21:19:34]

Description was changed from

==========
Add enrolled to domain bit to crash keys in renderer process.

This is a follow up to 2514483002 which adds this flag to the browser process.

BUG=660868
==========

to

==========
Add enrolled to domain bit to crash keys in renderer process.

This is a follow up to 2514483002 which adds this flag to the browser process.

BUG=660868

Review-Url: https://codereview.chromium.org/2618623003
Cr-Commit-Position: refs/heads/master@{#442690}
Committed:
https://chromium.googlesource.com/chromium/src/+/80353b52b8d21e3481f6e1b69eec...
==========

[commit-bot: I haz the power, 2017-01-10 21:19:35]

Committed patchset #1 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/80353b52b8d21e3481f6e1b69eec...