Clear client id prefs when opting out of UMA.

Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

This CL clears both the client id stored in local state prefs as
well as the backup of it in the registry on Windows. On other
platforms, the backup of the client id is stored in the consent
file, which was already being correctly deleted.

The codepath to enable metrics is different on Android and
this CL adds a couple of TODOs to unify things more in the
future. That would be a larger refactoring and I think should
be done in a separate CL.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.

Committed: https://crrev.com/2b53ee08893700984b965c09713a65342d4019a2
Cr-Commit-Position: refs/heads/master@{#411979}

[Alexei Svitkine (slow), 2016-08-08 21:30:14]

Description was changed from

==========
Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.
==========

to

==========
Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

This CL clears both the client id stored in local state prefs as
well as the backup of it in the registry on Windows. On other
platforms, the backup of the client id is stored in the consent
file, which was already being correctly deleted.

The codepath to enable metrics is different on Android and
this CL adds a couple of TODOs to unify things more in the
future. That would be a larger refactoring and I think should
be done in a separate CL.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.
==========

[Alexei Svitkine (slow), 2016-08-08 21:30:23]

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

[commit-bot: I haz the power, 2016-08-08 21:30:49]

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, 2016-08-08 22:56:29]

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

[commit-bot: I haz the power, 2016-08-08 22:56:29]

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

[Alexei Svitkine (slow), 2016-08-09 01:01:31]

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

[commit-bot: I haz the power, 2016-08-09 01:02:30]

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, 2016-08-09 01:09:48]

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

[commit-bot: I haz the power, 2016-08-09 01:09:49]

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

[Alexei Svitkine (slow), 2016-08-12 17:06:02]

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

[commit-bot: I haz the power, 2016-08-12 17:06:16]

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

[Alexei Svitkine (slow), 2016-08-12 17:11:06]

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

[Alexei Svitkine (slow), 2016-08-12 17:11:10]

Patchset #6 (id:100001) has been deleted

[Alexei Svitkine (slow), 2016-08-12 17:11:17]

Patchset #5 (id:80001) has been deleted

[Alexei Svitkine (slow), 2016-08-12 17:11:21]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-08-12 17:11:23]

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

[Alexei Svitkine (slow), 2016-08-12 17:11:25]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2016-08-12 17:11:44]

Patchset #1 (id:20001) has been deleted

[Alexei Svitkine (slow), 2016-08-12 17:32:50]

Patchset #1 (id:60001) has been deleted

[Alexei Svitkine (slow), 2016-08-12 17:36:48]

asvitkine@chromium.org changed reviewers:
+ gayane@chromium.org, grt@chromium.org, jwd@chromium.org

[Alexei Svitkine (slow), 2016-08-12 17:36:49]

gayane & jwd: please review the metrics changes
grt: please review the google update settings changes

Thanks!

[commit-bot: I haz the power, 2016-08-12 17:47:39]

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

[commit-bot: I haz the power, 2016-08-12 17:47:40]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-08-12 18:29:26]

On 2016/08/12 17:36:49, Alexei Svitkine (very slow) wrote:
> gayane & jwd: please review the metrics changes
> grt: please review the google update settings changes
> 
> Thanks!

It looks like client_id_ in the state manager doesn't get cleared, which I think
means it will be reused if UMA is turned back on in this session. It doesn't
look like it will ever get written back out to prefs or registry though. Maybe
the metrics EnableRecording behaviour should be changed to generate a new ID...
but that would put you in a strange running state... 

Looking around, can id cleanup be done in
MetricsServiceClient.OnRecordingDisabled? It's where we clear it from the crash
keys. That should be a unified place for desktop and android, right?

[Alexei Svitkine (slow), 2016-08-12 19:21:37]

Interesting. You're right that it will be re-used for the session.

I think we could just remove the early return at the start of
MetricsStateManager::ForceClientIdCreation() and then it should just work.

I'll take a look at OnRecordingDisabled() though to see if that might be a
better place.

[Alexei Svitkine (slow), 2016-08-12 19:23:42]

On 2016/08/12 19:21:37, Alexei Svitkine (very slow) wrote:
> Interesting. You're right that it will be re-used for the session.
> 
> I think we could just remove the early return at the start of
> MetricsStateManager::ForceClientIdCreation() and then it should just work.
> 
> I'll take a look at OnRecordingDisabled() though to see if that might be a
> better place.

So OnRecordingDisabled() is called by MetricsService::Stop() which is called
during shutdown:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rc...

So it doesn't make sense to do it there.

[Alexei Svitkine (slow), 2016-08-12 19:25:18]

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

[Alexei Svitkine (slow), 2016-08-12 19:25:59]

Removed the early return in MetricsStateManager::ForceClientIdCreation() per
discussion.

PTAL.

[commit-bot: I haz the power, 2016-08-12 19:26:11]

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, 2016-08-12 19:43:21]

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

[commit-bot: I haz the power, 2016-08-12 19:43:21]

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

[grt (UTC plus 2), 2016-08-12 19:43:32]

chrome/installer lgtm w/ a nit

should we also clear the crash client id?

https://codereview.chromium.org/2222903004/diff/120001/chrome/installer/util/...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/2222903004/diff/120001/chrome/installer/util/...
chrome/installer/util/google_update_settings.cc:335:
GoogleUpdateSettings::StoreMetricsClientInfo(metrics::ClientInfo());
nit: omit "GoogleUpdateSettings::"

[jwd, 2016-08-12 20:00:55]

On 2016/08/12 19:43:32, grt (UTC plus 2) wrote:
> chrome/installer lgtm w/ a nit
> 
> should we also clear the crash client id?
I believe the crash client id is already being cleared in OnRecordingDisabled,
with the call to crash_keys::ClearMetricsClientId, but I'm not sure if there any
other place where it would be stored.
> 
>
https://codereview.chromium.org/2222903004/diff/120001/chrome/installer/util/...
> File chrome/installer/util/google_update_settings.cc (right):
> 
>
https://codereview.chromium.org/2222903004/diff/120001/chrome/installer/util/...
> chrome/installer/util/google_update_settings.cc:335:
> GoogleUpdateSettings::StoreMetricsClientInfo(metrics::ClientInfo());
> nit: omit "GoogleUpdateSettings::"

[jwd, 2016-08-12 20:02:06]

On 2016/08/12 19:23:42, Alexei Svitkine (very slow) wrote:
> On 2016/08/12 19:21:37, Alexei Svitkine (very slow) wrote:
> > Interesting. You're right that it will be re-used for the session.
> > 
> > I think we could just remove the early return at the start of
> > MetricsStateManager::ForceClientIdCreation() and then it should just work.
> > 
> > I'll take a look at OnRecordingDisabled() though to see if that might be a
> > better place.
> 
> So OnRecordingDisabled() is called by MetricsService::Stop() which is called
> during shutdown:
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rc...
> 
> So it doesn't make sense to do it there.

The crash client id is being cleared there, wrapped in a !IsShuttingDown check.
Would it then make sense to consolidate that with the clearing being done in
this CL?

[Alexei Svitkine (slow), 2016-08-12 21:44:14]

On 2016/08/12 20:02:06, jwd wrote:
> On 2016/08/12 19:23:42, Alexei Svitkine (very slow) wrote:
> > On 2016/08/12 19:21:37, Alexei Svitkine (very slow) wrote:
> > > Interesting. You're right that it will be re-used for the session.
> > > 
> > > I think we could just remove the early return at the start of
> > > MetricsStateManager::ForceClientIdCreation() and then it should just work.
> > > 
> > > I'll take a look at OnRecordingDisabled() though to see if that might be a
> > > better place.
> > 
> > So OnRecordingDisabled() is called by MetricsService::Stop() which is called
> > during shutdown:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rc...
> > 
> > So it doesn't make sense to do it there.
> 
> The crash client id is being cleared there, wrapped in a !IsShuttingDown
check.
> Would it then make sense to consolidate that with the clearing being done in
> this CL?

Right. I feel nervous about doing it here - since it's called whenever
MetricsService::Stop() is called. And I'd rather not conflate stopping the
service with opting out of UMA reporting.

(It's an argument for actually moving the crash client id clearing out as well.)

I think for crash, the id gets synced on start up, so clearing it on shutdown
(even before the IsShuttingDown() check was added - which was recent) was not
very problematic. (It was just making crashes during shutdown not have client
ids, which was a problem, but no lasting effects).

So in summary, I'd actually prefer us killing the OnRecordingDisabled() client
callback entirely and moving the crash id clearing to somewhere else. Maybe just
a helper function in metrics_reporting_state.cc that would clear all of these.
WDYT?

[jwd, 2016-08-12 21:52:02]

On 2016/08/12 21:44:14, Alexei Svitkine (very slow) wrote:
> On 2016/08/12 20:02:06, jwd wrote:
> > On 2016/08/12 19:23:42, Alexei Svitkine (very slow) wrote:
> > > On 2016/08/12 19:21:37, Alexei Svitkine (very slow) wrote:
> > > > Interesting. You're right that it will be re-used for the session.
> > > > 
> > > > I think we could just remove the early return at the start of
> > > > MetricsStateManager::ForceClientIdCreation() and then it should just
work.
> > > > 
> > > > I'll take a look at OnRecordingDisabled() though to see if that might be
a
> > > > better place.
> > > 
> > > So OnRecordingDisabled() is called by MetricsService::Stop() which is
called
> > > during shutdown:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rc...
> > > 
> > > So it doesn't make sense to do it there.
> > 
> > The crash client id is being cleared there, wrapped in a !IsShuttingDown
> check.
> > Would it then make sense to consolidate that with the clearing being done in
> > this CL?
> 
> Right. I feel nervous about doing it here - since it's called whenever
> MetricsService::Stop() is called. And I'd rather not conflate stopping the
> service with opting out of UMA reporting.
> 
> (It's an argument for actually moving the crash client id clearing out as
well.)
> 
> I think for crash, the id gets synced on start up, so clearing it on shutdown
> (even before the IsShuttingDown() check was added - which was recent) was not
> very problematic. (It was just making crashes during shutdown not have client
> ids, which was a problem, but no lasting effects).
> 
> So in summary, I'd actually prefer us killing the OnRecordingDisabled() client
> callback entirely and moving the crash id clearing to somewhere else. Maybe
just
> a helper function in metrics_reporting_state.cc that would clear all of these.
> WDYT?

SGTM

[Alexei Svitkine (slow), 2016-08-12 22:07:39]

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

[commit-bot: I haz the power, 2016-08-12 22:08:20]

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, 2016-08-12 22:23:02]

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

[commit-bot: I haz the power, 2016-08-12 22:23:02]

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

[Alexei Svitkine (slow), 2016-08-12 22:29:59]

Thanks. I'll do the clean up of moving the crash client id clearing in the same
place in a follow-up CL (as well as remove the extra metrics client API).

In the latest patch set, I had to add a bit of extra logic to 
components/metrics/metrics_state_manager.cc to make test expectations around
when client id backups where triggered pass, now that the early return on
client_id_ at the top was removed.

PTAL.

https://codereview.chromium.org/2222903004/diff/120001/chrome/installer/util/...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/2222903004/diff/120001/chrome/installer/util/...
chrome/installer/util/google_update_settings.cc:335:
GoogleUpdateSettings::StoreMetricsClientInfo(metrics::ClientInfo());
On 2016/08/12 19:43:32, grt (UTC plus 2) wrote:
> nit: omit "GoogleUpdateSettings::"

Done.

[Alexei Svitkine (slow), 2016-08-12 22:59:48]

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

[commit-bot: I haz the power, 2016-08-12 23:00:20]

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, 2016-08-13 00:25:33]

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

[commit-bot: I haz the power, 2016-08-13 00:25:34]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-08-15 15:13:55]

lgtm

[gayane -on leave until 09-2017, 2016-08-15 15:43:09]

lgtm

[Alexei Svitkine (slow), 2016-08-15 16:00:01]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-08-15 16:00:01]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2222903004/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-15 16:00: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, 2016-08-15 17:36:43]

Description was changed from

==========
Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

This CL clears both the client id stored in local state prefs as
well as the backup of it in the registry on Windows. On other
platforms, the backup of the client id is stored in the consent
file, which was already being correctly deleted.

The codepath to enable metrics is different on Android and
this CL adds a couple of TODOs to unify things more in the
future. That would be a larger refactoring and I think should
be done in a separate CL.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.
==========

to

==========
Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

This CL clears both the client id stored in local state prefs as
well as the backup of it in the registry on Windows. On other
platforms, the backup of the client id is stored in the consent
file, which was already being correctly deleted.

The codepath to enable metrics is different on Android and
this CL adds a couple of TODOs to unify things more in the
future. That would be a larger refactoring and I think should
be done in a separate CL.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.
==========

[commit-bot: I haz the power, 2016-08-15 17:36:46]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-08-15 17:41:01]

Description was changed from

==========
Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

This CL clears both the client id stored in local state prefs as
well as the backup of it in the registry on Windows. On other
platforms, the backup of the client id is stored in the consent
file, which was already being correctly deleted.

The codepath to enable metrics is different on Android and
this CL adds a couple of TODOs to unify things more in the
future. That would be a larger refactoring and I think should
be done in a separate CL.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.
==========

to

==========
Clear client id prefs when opting out of UMA.

When a user opts out of UMA, we should clear their previous
client id. Opting back in should result in a new client id
to be re-generated.

This CL clears both the client id stored in local state prefs as
well as the backup of it in the registry on Windows. On other
platforms, the backup of the client id is stored in the consent
file, which was already being correctly deleted.

The codepath to enable metrics is different on Android and
this CL adds a couple of TODOs to unify things more in the
future. That would be a larger refactoring and I think should
be done in a separate CL.

BUG=635669
TEST=On a machine that does not have Chrome entreprise policy, open
chrome://settings and enable "Automatically send usage statistics
and crash reports to Google". Then, check that there is a client_id2
entry in chrome://local-state. Now, uncheck the checkbox on the
setting page and verify that reloading chrome://local-state does
not have a client_id2 field.

Committed: https://crrev.com/2b53ee08893700984b965c09713a65342d4019a2
Cr-Commit-Position: refs/heads/master@{#411979}
==========

[commit-bot: I haz the power, 2016-08-15 17:41:03]

Patchset 4 (id:??) landed as
https://crrev.com/2b53ee08893700984b965c09713a65342d4019a2
Cr-Commit-Position: refs/heads/master@{#411979}