[win] Don't initialize the crash reporter on windows if disabled by configuration management.

Don't initialize the crash reporter on windows if disabled by configuration management.

BUG=49662
TEST=No crash reports get submitted if policy doesn't allow it.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54525

[Mattias Nissler (ping if slow), 2010-07-27 18:54:45]

Hi, could you review? danno please check the policy parts and I brought in
nsylvain to check the changes to the crash reporter code since I don't know it
very well.

[danno, 2010-07-28 15:23:26]

policy parts LGTM

[nsylvain, 2010-07-29 17:03:55]

The code LGTM but I'm not sure I understand the difference between that and
GoogleUpdateSettings::GetCollectStatsConsent()

Is it not doing a similar thing?

[Mattias Nissler (ping if slow), 2010-07-29 21:06:37]

On 2010/07/29 17:03:55, nsylvain wrote:
> The code LGTM but I'm not sure I understand the difference between that and
> GoogleUpdateSettings::GetCollectStatsConsent()
> 
> Is it not doing a similar thing?

These changes allow to turn off crash reporting through windows group policy. I
plan to upload a new version tomorrow that also supports forcing crash reporting
on through group policy.

[nsylvain, 2010-07-29 21:28:12]

http://codereview.chromium.org/3033030/diff/2001/3001
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/3033030/diff/2001/3001#newcode404
chrome/app/breakpad_win.cc:404: return false;
should not this be true? Since by default no one will have this setting, and we
don't want to disable everyone because of that.

[Mattias Nissler (ping if slow), 2010-07-30 10:08:11]

Ok, here's an updated version with logic that allows policy to force crash
reporting on.

http://codereview.chromium.org/3033030/diff/2001/3001
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/3033030/diff/2001/3001#newcode404
chrome/app/breakpad_win.cc:404: return false;
On 2010/07/29 21:28:12, nsylvain wrote:
> should not this be true? Since by default no one will have this setting, and
we
> don't want to disable everyone because of that.

You are absolutely right. Thanks!

[nsylvain, 2010-07-30 17:12:49]

Is the plumbing to make sure the option reflects correctly in the options menu
done?  It seems right now that the option is just going to be ignored if this
registry key exists.

http://codereview.chromium.org/3033030/diff/3008/15001
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/3033030/diff/3008/15001#newcode434
chrome/app/breakpad_win.cc:434: bool use_crash_service = !controlled_by_policy
&&
I'm not sure when/how the registry value is going to be set, but if it is
remotely possible that it gets set on the buildbot, then this code it wrong. 
Even if it's control by policy, if the HEADLESS env variable is set, we should
use the crash_service.

People need that to debug too.

http://codereview.chromium.org/3033030/diff/3008/15007
File chrome/common_constants.gypi (right):

http://codereview.chromium.org/3033030/diff/3008/15007#newcode27
chrome/common_constants.gypi:27: 'common/net/gaia/gaia_constants.cc',
are you supposed to svn add those files too?

[Mattias Nissler (ping if slow), 2010-07-30 18:14:05]

The option dialog already adheres to the registry value. That's handled through
the configuration management support in PrefService (see
configuration_policy_provider_win.cc for the code that reads the registry). I'd
love to go through that infrastructure also, but since the crash reporter starts
very early, the infrastructure is not yet available and I need to resort to
reading bare registry keys.

http://codereview.chromium.org/3033030/diff/3008/15001
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/3033030/diff/3008/15001#newcode434
chrome/app/breakpad_win.cc:434: bool use_crash_service = !controlled_by_policy
&&
On 2010/07/30 17:12:49, nsylvain wrote:
> I'm not sure when/how the registry value is going to be set, but if it is
> remotely possible that it gets set on the buildbot, then this code it wrong. 
> Even if it's control by policy, if the HEADLESS env variable is set, we should
> use the crash_service.
> 
> People need that to debug too.
> 
> 

The registry value is the hook administrators use to lock down Chrome on their
client machines. It wouldn't make sense to allow users to work around the
configuration policy by setting an environment variable. The registry value will
never be set on the buildbots, unless admins try to lock down chrome on those
machines. But that parameter we (or you, technically) control.

http://codereview.chromium.org/3033030/diff/3008/15007
File chrome/common_constants.gypi (right):

http://codereview.chromium.org/3033030/diff/3008/15007#newcode27
chrome/common_constants.gypi:27: 'common/net/gaia/gaia_constants.cc',
On 2010/07/30 17:12:49, nsylvain wrote:
> are you supposed to svn add those files too?

I suppose you looked at the diff to the previous patch. They only show up since
someone else added them meanwhile and I rebased to ToT, but are not part of this
CL.

[nsylvain, 2010-07-30 20:28:58]

On Fri, Jul 30, 2010 at 11:14 AM, <mnissler@chromium.org> wrote:

> The option dialog already adheres to the registry value. That's handled
> through
> the configuration management support in PrefService (see
> configuration_policy_provider_win.cc for the code that reads the registry).
> I'd
> love to go through that infrastructure also, but since the crash reporter
> starts
> very early, the infrastructure is not yet available and I need to resort to
> reading bare registry keys.
>
>
>
> http://codereview.chromium.org/3033030/diff/3008/15001
> File chrome/app/breakpad_win.cc (right):
>
> http://codereview.chromium.org/3033030/diff/3008/15001#newcode434
> chrome/app/breakpad_win.cc:434: bool use_crash_service =
> !controlled_by_policy &&
> On 2010/07/30 17:12:49, nsylvain wrote:
>
>> I'm not sure when/how the registry value is going to be set, but if it
>>
> is
>
>> remotely possible that it gets set on the buildbot, then this code it
>>
> wrong.
>
>> Even if it's control by policy, if the HEADLESS env variable is set,
>>
> we should
>
>> use the crash_service.
>>
>
>  People need that to debug too.
>>
>
>
>
> The registry value is the hook administrators use to lock down Chrome on
> their client machines. It wouldn't make sense to allow users to work
> around the configuration policy by setting an environment variable. The
> registry value will never be set on the buildbots, unless admins try to
> lock down chrome on those machines. But that parameter we (or you,
> technically) control.


I assume the main reason for administrators to lock down chrome is because
they don't want to send crash dump to Google, right?

using the crash_service will not send crash dumps to Google. It just saves
them locally.

But if you think this should not be an issue, then i'm ok... as long as
google does not start doing that!


>
> http://codereview.chromium.org/3033030/diff/3008/15007
> File chrome/common_constants.gypi (right):
>
> http://codereview.chromium.org/3033030/diff/3008/15007#newcode27
> chrome/common_constants.gypi:27: 'common/net/gaia/gaia_constants.cc',
> On 2010/07/30 17:12:49, nsylvain wrote:
>
>> are you supposed to svn add those files too?
>>
>
> I suppose you looked at the diff to the previous patch. They only show
> up since someone else added them meanwhile and I rebased to ToT, but are
> not part of this CL.

Oh, ok :)

LGTM

>
>
> http://codereview.chromium.org/3033030/show
>

[Mattias Nissler (ping if slow), 2010-08-02 09:41:51]

On 2010/07/30 20:28:58, nsylvain wrote:
> On Fri, Jul 30, 2010 at 11:14 AM, <mailto:mnissler@chromium.org> wrote:
> 
> > The option dialog already adheres to the registry value. That's handled
> > through
> > the configuration management support in PrefService (see
> > configuration_policy_provider_win.cc for the code that reads the registry).
> > I'd
> > love to go through that infrastructure also, but since the crash reporter
> > starts
> > very early, the infrastructure is not yet available and I need to resort to
> > reading bare registry keys.
> >
> >
> >
> > http://codereview.chromium.org/3033030/diff/3008/15001
> > File chrome/app/breakpad_win.cc (right):
> >
> > http://codereview.chromium.org/3033030/diff/3008/15001#newcode434
> > chrome/app/breakpad_win.cc:434: bool use_crash_service =
> > !controlled_by_policy &&
> > On 2010/07/30 17:12:49, nsylvain wrote:
> >
> >> I'm not sure when/how the registry value is going to be set, but if it
> >>
> > is
> >
> >> remotely possible that it gets set on the buildbot, then this code it
> >>
> > wrong.
> >
> >> Even if it's control by policy, if the HEADLESS env variable is set,
> >>
> > we should
> >
> >> use the crash_service.
> >>
> >
> >  People need that to debug too.
> >>
> >
> >
> >
> > The registry value is the hook administrators use to lock down Chrome on
> > their client machines. It wouldn't make sense to allow users to work
> > around the configuration policy by setting an environment variable. The
> > registry value will never be set on the buildbots, unless admins try to
> > lock down chrome on those machines. But that parameter we (or you,
> > technically) control.
> 
> 
> I assume the main reason for administrators to lock down chrome is because
> they don't want to send crash dump to Google, right?

Yes, that's one aspect.

> 
> using the crash_service will not send crash dumps to Google. It just saves
> them locally.

I'd like to first make sure Chrome doesn't do any crash dumping. If we get
requests for local crash dumping in corporate environments (but not sending
information to Google) we can add appropriate functionality to the policy code.

> 
> But if you think this should not be an issue, then i'm ok... as long as
> google does not start doing that!

As long as we have root on our development and build machines, there's no issue
:)

> 
> 
> >
> > http://codereview.chromium.org/3033030/diff/3008/15007
> > File chrome/common_constants.gypi (right):
> >
> > http://codereview.chromium.org/3033030/diff/3008/15007#newcode27
> > chrome/common_constants.gypi:27: 'common/net/gaia/gaia_constants.cc',
> > On 2010/07/30 17:12:49, nsylvain wrote:
> >
> >> are you supposed to svn add those files too?
> >>
> >
> > I suppose you looked at the diff to the previous patch. They only show
> > up since someone else added them meanwhile and I rebased to ToT, but are
> > not part of this CL.
> 
> Oh, ok :)
> 
> LGTM
> 
> >
> >
> > http://codereview.chromium.org/3033030/show
> >
>