Platform-specific prune state storage.

Platform-specific prune state storage.

BUG=496744
TBR=sky@chromium.org

Committed: https://crrev.com/c1d1b7554e800c0bb446eea791b84493d27aee9d
Cr-Commit-Position: refs/heads/master@{#347571}

[grt (UTC plus 2), 2015-07-30 04:13:38]

Patchset #2 (id:20001) has been deleted

[grt (UTC plus 2), 2015-07-31 04:22:18]

Patchset #2 (id:40001) has been deleted

[grt (UTC plus 2), 2015-07-31 04:22:29]

Patchset #1 (id:1) has been deleted

[grt (UTC plus 2), 2015-07-31 04:22:50]

grt@chromium.org changed reviewers:
+ caitkp@chromium.org, robertshield@chromium.org

[grt (UTC plus 2), 2015-07-31 04:25:09]

Cait and Robert:

Here's what I have so far for the registry-based state storage. It's lacking
some code comments and some unit tests. I'm happy to pick this up upon my return
and complete it.  Otherwise, one approach would be to do a review on the
implementation, take notes on what looks tricky enough to be tested, then beef
up the unittests to cover those cases.

Cheers.

[Cait (Slow), 2015-08-18 18:19:45]

Looks pretty good overall -- but I agree that a few more comments/unittests
surrounding the platform_state_store might make it easier to figure out what's
going on.

https://codereview.chromium.org/1243293003/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
(right):

https://codereview.chromium.org/1243293003/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc:27: //
Copies the (key, digest) pairs from |keys_and_digests| (a dict of string
It might be helpful to explain at the beginning of these functions how (type,
dict) pairs relate to (key, digest) pairs (or add some tests with sample data).

[grt (UTC plus 2), 2015-08-27 00:54:48]

Patchset #6 (id:160001) has been deleted

[grt (UTC plus 2), 2015-08-27 01:04:51]

Patchset #6 (id:180001) has been deleted

[grt (UTC plus 2), 2015-08-27 01:24:13]

Patchset #6 (id:200001) has been deleted

[grt (UTC plus 2), 2015-08-27 01:24:44]

grt@chromium.org changed reviewers:
+ pkasting@chromium.org

[grt (UTC plus 2), 2015-08-27 01:25:03]

grt@chromium.org changed reviewers:
+ jwd@chromium.org

[grt (UTC plus 2), 2015-08-27 01:25:21]

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

[commit-bot: I haz the power, 2015-08-27 01:25:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243293003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243293003/220001

[grt (UTC plus 2), 2015-08-27 01:26:02]

PTAL
pkasting: third_party/protobuf OWNERS review
jwd: tools/metrics/histograms OWNERS review
Thanks.

[commit-bot: I haz the power, 2015-08-27 02:55:11]

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

[commit-bot: I haz the power, 2015-08-27 02:55:12]

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

[Peter Kasting, 2015-08-27 07:48:55]

(1) The protobuf upgrade was reverted.  The best thing to do is going to be to
try to get your changes applied upstream so next time we try again to pull a new
version we'll get the right thing.
(2) I'd prefer to avoid local patches just to fix warnings anyway, even if (1)
weren't true.  Instead, fix the warnings upstream, and in the meantime simply
disable the warnings in the downstream gyp/gn files.  When the upstream patch
lands, we can pull a new version and remove the warning override.
(3) I don't think this code is correct; the functions which take a
|default_enum_value| ignore the value and assume it's false, which seems
questionable.

[grt (UTC plus 2), 2015-08-27 20:55:35]

grt@chromium.org changed reviewers:
- pkasting@chromium.org

[grt (UTC plus 2), 2015-08-27 20:57:01]

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

[commit-bot: I haz the power, 2015-08-27 20:57:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243293003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243293003/240001

[grt (UTC plus 2), 2015-08-27 20:58:40]

Since the roll of the protobuf library was backed out, I've had to switch from
protobuf maps back to repeated fields. I've done so in such a way as to make the
wire formats identical so that this code can someday be switched to using a map.
This will simplify it a tad.

Please take another look.

[commit-bot: I haz the power, 2015-08-27 22:37:39]

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

[commit-bot: I haz the power, 2015-08-27 22:37:39]

Dry run: This issue passed the CQ dry run.

[jwd, 2015-08-28 15:28:59]

https://codereview.chromium.org/1243293003/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1243293003/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38584: +    The size, in bytes, of data
stored in the platform state store.
This sounds like it's the total size of the data in the store, but it looks like
it's actually the size of the data being stored (I don't know if there's a
difference for this). Can you update the wording a bit to make it clearer.

Can you also mention when it's being logged, i.e. on when data gets stored to
it.

[grt (UTC plus 2), 2015-08-28 20:20:44]

Thanks!

https://codereview.chromium.org/1243293003/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1243293003/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38584: +    The size, in bytes, of data
stored in the platform state store.
On 2015/08/28 15:28:59, Jesse Doherty wrote:
> This sounds like it's the total size of the data in the store, but it looks
like
> it's actually the size of the data being stored (I don't know if there's a
> difference for this). Can you update the wording a bit to make it clearer.
> 
> Can you also mention when it's being logged, i.e. on when data gets stored to
> it.

Done.

[jwd, 2015-08-31 15:42:36]

lgtm

[Cait (Slow), 2015-08-31 17:55:29]

looks good overall, just a couple comments -- and the proto <--> pref code is
much easier to follow with the map-like structure.

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc
(right):

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc:50:
return PlatformStateStoreLoadResult::CLEARED_NO_DATA;
Is the assumption here that if we failed to open the key, it was because there
is no key, and thus no data to clear?

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc
(right):

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc:55:
user_pref_store->set_read_error(
Is setting the read_error really the best way to tell the ProfileManager that a
profile is new? If so, this approach seems fine, but might be worth adding a
comment to explain this.

https://codereview.chromium.org/1243293003/diff/280001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.h
(right):

https://codereview.chromium.org/1243293003/diff/280001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:70: //
contents. Exposed for testing.
Which tests are these exposed for?

[grt (UTC plus 2), 2015-09-01 13:55:23]

PTAL!

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc
(right):

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc:50:
return PlatformStateStoreLoadResult::CLEARED_NO_DATA;
On 2015/08/31 17:55:29, Cait wrote:
> Is the assumption here that if we failed to open the key, it was because there
> is no key, and thus no data to clear?

Yes. While Open() could fail for other reasons, non-existent key is the most
likely. MSDN doesn't specify exactly what error code is returned in this case,
so I'm assuming here that anything else will be lost in the noise. If this UMA
stat is being logged more than we think it should be, we could UMA the return
value from Open().

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc
(right):

https://codereview.chromium.org/1243293003/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc:55:
user_pref_store->set_read_error(
On 2015/08/31 17:55:29, Cait wrote:
> Is setting the read_error really the best way to tell the ProfileManager that
a
> profile is new? If so, this approach seems fine, but might be worth adding a
> comment to explain this.

As far as I can tell, it's the only way. Comment added.

https://codereview.chromium.org/1243293003/diff/280001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.h
(right):

https://codereview.chromium.org/1243293003/diff/280001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:70: //
contents. Exposed for testing.
On 2015/08/31 17:55:29, Cait wrote:
> Which tests are these exposed for?

The ones I just wrote. :-)

[grt (UTC plus 2), 2015-09-01 13:55:31]

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

[commit-bot: I haz the power, 2015-09-01 13:55:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243293003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243293003/300001

[robertshield, 2015-09-01 14:20:12]

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.h
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:42: #if
defined(USE_PLATFORM_STATE_STORE)
I thought it more conventional to provide empty stub implementations for methods
on platforms that don't implement them, rather than preprocess away the
declarations. Has the zeitgeist shifted?

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:46: //
sake of testing.
could these be placed in an internal namespace? If I missed this comment, I
would think that they're part of the public interface.

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc:27:
BrowserDistribution::GetDistribution()->GetInstallSubDir())
FYI, I've seen two ways of building such reg key paths in the code base, this
one:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
and the one you have here. This is a fair bit nicer (and probably more correct),
so go with this, but thought I'd mention the difference.

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc:44:
registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER);
given that this and the declaration of registry_override_manager_ are the only
things the base class does, you could shorten this by inlining it in
StateStoreTest and doing away with the conditionally-defined base class.

https://codereview.chromium.org/1243293003/diff/300001/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:855: // Chrome on a system-level install;
see RemoveBlacklistState for details.
This deletes all persistent state, more than just what is saved in this CL. 

This is almost certainly right, but I see a bunch of stuff stored under that
key, did you double check that none of it is meant to survive an
uninstall-reinstall cycle?

[commit-bot: I haz the power, 2015-09-01 14:51:53]

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

[commit-bot: I haz the power, 2015-09-01 14:51:54]

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

[Cait (Slow), 2015-09-01 19:03:05]

lgtm

[grt (UTC plus 2), 2015-09-03 18:12:06]

PTAL

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.h
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:42: #if
defined(USE_PLATFORM_STATE_STORE)
On 2015/09/01 14:20:12, robertshield wrote:
> I thought it more conventional to provide empty stub implementations for
methods
> on platforms that don't implement them, rather than preprocess away the
> declarations. Has the zeitgeist shifted?

Load/Store above are the public functions, and exist (and empty) for platforms
that don't use a platform-specific state store. The other declarations in this
file are exposed only for the sake of tests (as mentioned in the comment on line
44).

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:46: //
sake of testing.
On 2015/09/01 14:20:12, robertshield wrote:
> could these be placed in an internal namespace? If I missed this comment, I
> would think that they're part of the public interface.

I have been told explicitly not to do that:
https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g...

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc:27:
BrowserDistribution::GetDistribution()->GetInstallSubDir())
On 2015/09/01 14:20:12, robertshield wrote:
> FYI, I've seen two ways of building such reg key paths in the code base, this
> one:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
> and the one you have here. This is a fair bit nicer (and probably more
correct),
> so go with this, but thought I'd mention the difference. 

Thanks for pointing that out. The other way doesn't separate SxS from Google
Chrome. I wonder if all consumers of PRODUCT_STRING_PATH are aware of this?

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc:44:
registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER);
On 2015/09/01 14:20:12, robertshield wrote:
> given that this and the declaration of registry_override_manager_ are the only
> things the base class does, you could shorten this by inlining it in
> StateStoreTest and doing away with the conditionally-defined base class.

I decided to go this route since other platforms will surely need their own
magic. I like how this isolates the platform-specific changes.

https://codereview.chromium.org/1243293003/diff/300001/chrome/installer/setup...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/installer/setup...
chrome/installer/setup/uninstall.cc:855: // Chrome on a system-level install;
see RemoveBlacklistState for details.
On 2015/09/01 14:20:12, robertshield wrote:
> This deletes all persistent state, more than just what is saved in this CL. 
> 
> This is almost certainly right, but I see a bunch of stuff stored under that
> key, did you double check that none of it is meant to survive an
> uninstall-reinstall cycle?

No, I didn't. As a policy, I believe that selecting "Also delete your browsing
data?" on uninstall should remove all traces of Chrome. Unfortunately, this key
is used as a public configuration knob for certain features so blindly removing
it like this won't work. I'll address this in a separate CL.

[robertshield, 2015-09-05 03:23:25]

lgtm

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.h
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:46: //
sake of testing.
On 2015/09/03 18:12:05, grt wrote:
> On 2015/09/01 14:20:12, robertshield wrote:
> > could these be placed in an internal namespace? If I missed this comment, I
> > would think that they're part of the public interface.
> 
> I have been told explicitly not to do that:
>
https://codereview.chromium.org/729273002/diff/180001/chrome/browser/google/g...

Hrm, ok. Not sure I agree with the premise of that thread that clearly
separating out things that aren't intended for other code to use is
over-designing, but the difference here is too small to be worth arguing about.

They could be moved to a platform_state_store_internal.h which avoids the
"namespace" objection, but would probably still get labeled as over-designing.
Let's leave it as it is I suppose, the chances of this ever being a real problem
are low.

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc:27:
BrowserDistribution::GetDistribution()->GetInstallSubDir())
On 2015/09/03 18:12:05, grt wrote:
> On 2015/09/01 14:20:12, robertshield wrote:
> > FYI, I've seen two ways of building such reg key paths in the code base,
this
> > one:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
> > and the one you have here. This is a fair bit nicer (and probably more
> correct),
> > so go with this, but thought I'd mention the difference. 
> 
> Thanks for pointing that out. The other way doesn't separate SxS from Google
> Chrome. I wonder if all consumers of PRODUCT_STRING_PATH are aware of this?

I reckon not, though for cases that don't store profile- or install- specific
state it might not matter and in some cases sharing stuff between chrome and
canary might be desirable.

[Peter Kasting, 2015-09-05 03:31:54]

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

[Peter Kasting, 2015-09-05 03:31:55]

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.h
(right):

https://codereview.chromium.org/1243293003/diff/300001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.h:46: //
sake of testing.
On 2015/09/05 03:23:24, robertshield wrote:
> They could be moved to a platform_state_store_internal.h which avoids the
> "namespace" objection, but would probably still get labeled as over-designing.

I don't have any problem with that solution.

[grt (UTC plus 2), 2015-09-05 12:18:50]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2015-09-05 12:18:51]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org,
caitkp@chromium.org
Link to the patchset: https://codereview.chromium.org/1243293003/#ps320001
(title: "moved uninstall cleanup to separate cl")

[commit-bot: I haz the power, 2015-09-05 12:18:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243293003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243293003/320001

[commit-bot: I haz the power, 2015-09-05 12:28:58]

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

[commit-bot: I haz the power, 2015-09-05 12:28:58]

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

[grt (UTC plus 2), 2015-09-05 12:46:27]

grt@chromium.org changed reviewers:
+ sky@chromium.org

[grt (UTC plus 2), 2015-09-05 12:47:04]

TBR sky@chromium.org as OWNER for chrome/browser/BUILD.gn change.

[grt (UTC plus 2), 2015-09-05 12:47:11]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-09-05 12:47:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243293003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243293003/320001

[commit-bot: I haz the power, 2015-09-05 13:26:49]

Committed patchset #11 (id:320001)

[commit-bot: I haz the power, 2015-09-05 13:27:40]

Patchset 11 (id:??) landed as
https://crrev.com/c1d1b7554e800c0bb446eea791b84493d27aee9d
Cr-Commit-Position: refs/heads/master@{#347571}

[sky, 2015-09-08 16:21:10]

chrome/browser/BUILD.gn LGTM