Cleanup PreferenceMACs on browser_tests startup

Cleanup PreferenceMACs on browser_tests startup
and prevent proliferation of scoped_dir subkeys under it in the future.

BUG=721245
R=jam@chromium.org

Review-Url: https://codereview.chromium.org/2893703002 .
Cr-Commit-Position: refs/heads/master@{#472565}
Committed: https://chromium.googlesource.com/chromium/src/+/dfcdd8a05d8e0867a68cfc366fb7e7d79b3a318f

[gab, 2017-05-17 18:27:58]

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

[gab, 2017-05-17 18:28:13]

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

[gab, 2017-05-17 18:28:13]

John PTaL, thanks!

[commit-bot: I haz the power, 2017-05-17 18:29:50]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-17 18:29:55]

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

[commit-bot: I haz the power, 2017-05-17 18:30:55]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-17 18:34:54]

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

[commit-bot: I haz the power, 2017-05-17 18:35:53]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-17 18:55:12]

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

[commit-bot: I haz the power, 2017-05-17 18:55:54]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-17 18:56:23]

Patchset #3 (id:40001) has been deleted

[jam, 2017-05-17 19:06:16]

Thanks!

https://codereview.chromium.org/2893703002/diff/60001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (left):

https://codereview.chromium.org/2893703002/diff/60001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:522: // Test is flaky.
crbug.com/723639
https://codereview.chromium.org/2882423002 was also disabled

https://codereview.chromium.org/2893703002/diff/60001/chrome/test/base/chrome...
File chrome/test/base/chrome_test_launcher.cc (right):

https://codereview.chromium.org/2893703002/diff/60001/chrome/test/base/chrome...
chrome/test/base/chrome_test_launcher.cc:111: key.DeleteKey(L"PreferenceMACs");
does this mean that if someone runs official browser_tests, it will delete this
key for the chrome that the use? that seems bad?

can we only delete the subkeys that have "scoped" in them?

https://codereview.chromium.org/2893703002/diff/60001/content/public/test/tes...
File content/public/test/test_launcher.h (right):

https://codereview.chromium.org/2893703002/diff/60001/content/public/test/tes...
content/public/test/test_launcher.h:64: virtual void PreShardingCleanup() {}
nit: just call it PreSharding and don't mention implementation details that
chrome uses like registry, cleanup etc

https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
File services/preferences/tracked/registry_hash_store_contents_win.cc (right):

https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
services/preferences/tracked/registry_hash_store_contents_win.cc:76:
reset_on_delete_(base::StartsWith(store_key,
I'm curious, it seems this cleanup code is quite far from the place that's
creating scoped_dir profiles. so there's a risk that as that code changes, say
by updating the name, we will forget this part (and also the test cleanup).

is it possible to mark this object at the callsite that knows it's creating a
scoped temp dir?

[gab, 2017-05-17 19:56:09]

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

[commit-bot: I haz the power, 2017-05-17 19:56:17]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-17 19:58:42]

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

[gab, 2017-05-17 19:59:02]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-17 19:59:10]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-17 20:01:05]

https://codereview.chromium.org/2893703002/diff/60001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (left):

https://codereview.chromium.org/2893703002/diff/60001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:522: // Test is flaky.
crbug.com/723639
On 2017/05/17 19:06:16, jam wrote:
> https://codereview.chromium.org/2882423002 was also disabled

Done.

https://codereview.chromium.org/2893703002/diff/60001/chrome/test/base/chrome...
File chrome/test/base/chrome_test_launcher.cc (right):

https://codereview.chromium.org/2893703002/diff/60001/chrome/test/base/chrome...
chrome/test/base/chrome_test_launcher.cc:111: key.DeleteKey(L"PreferenceMACs");
On 2017/05/17 19:06:16, jam wrote:
> does this mean that if someone runs official browser_tests, it will delete
this
> key for the chrome that the use? that seems bad?

Yes, but it's not that bad (discussed details offline)

> 
> can we only delete the subkeys that have "scoped" in them?

We could but that'd increase complexity where I don't think it's worth it.

https://codereview.chromium.org/2893703002/diff/60001/content/public/test/tes...
File content/public/test/test_launcher.h (right):

https://codereview.chromium.org/2893703002/diff/60001/content/public/test/tes...
content/public/test/test_launcher.h:64: virtual void PreShardingCleanup() {}
On 2017/05/17 19:06:16, jam wrote:
> nit: just call it PreSharding and don't mention implementation details that
> chrome uses like registry, cleanup etc

Done.

https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
File services/preferences/tracked/registry_hash_store_contents_win.cc (right):

https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
services/preferences/tracked/registry_hash_store_contents_win.cc:76:
reset_on_delete_(base::StartsWith(store_key,
On 2017/05/17 19:06:16, jam wrote:
> I'm curious, it seems this cleanup code is quite far from the place that's
> creating scoped_dir profiles. so there's a risk that as that code changes, say
> by updating the name, we will forget this part (and also the test cleanup).
> 
> is it possible to mark this object at the callsite that knows it's creating a
> scoped temp dir?

I debated this too... basically the problem is a test does
CreateProfile(scoped_tmp_dir.path(), ...) and that dir eventually trickles here.
The "faulty" test has no idea it's even triggering this code so asking it to
mark anything at the callsite will likely result in someone forgetting to
eventually when writing such a test (their test will still pass but will cause
this issue).

Added a static call on ScopedTempDir to retrieve the prefix it's using in order
to at least link the name it's using to this code.

https://codereview.chromium.org/2893703002/diff/100001/base/files/scoped_temp...
File base/files/scoped_temp_dir.cc (left):

https://codereview.chromium.org/2893703002/diff/100001/base/files/scoped_temp...
base/files/scoped_temp_dir.cc:42: FILE_PATH_LITERAL("scoped_dir_"),
This was "scoped_dir_" not "scoped_dir" but I really don't this was intentional
and/or matters.

[jam, 2017-05-17 20:04:31]

lgtm

[gab, 2017-05-17 20:05:48]

The CQ bit was unchecked by gab@chromium.org

[gab, 2017-05-17 20:05:53]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-05-17 20:06:00]

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-05-17 20:35:14]

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

[commit-bot: I haz the power, 2017-05-17 20:35:15]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gab, 2017-05-17 20:43:00]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-17 20:43:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2893703002/#ps120001
(title: "fix compile")

[commit-bot: I haz the power, 2017-05-17 20:44:06]

CQ is trying da patch.

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

[grt (UTC plus 2), 2017-05-17 20:52:47]

https://codereview.chromium.org/2893703002/diff/120001/chrome/test/base/chrom...
File chrome/test/base/chrome_test_launcher.cc (right):

https://codereview.chromium.org/2893703002/diff/120001/chrome/test/base/chrom...
chrome/test/base/chrome_test_launcher.cc:111: key.DeleteKey(L"PreferenceMACs");
for devs like me who use Google Chrome and build Google Chrome, this will delete
the macs for my normal chrome install. i suppose this doesn't matter in the
grand scheme since tests will overwrite my macs anyway, right?

[gab, 2017-05-17 20:57:37]

https://codereview.chromium.org/2893703002/diff/120001/chrome/test/base/chrom...
File chrome/test/base/chrome_test_launcher.cc (right):

https://codereview.chromium.org/2893703002/diff/120001/chrome/test/base/chrom...
chrome/test/base/chrome_test_launcher.cc:111: key.DeleteKey(L"PreferenceMACs");
On 2017/05/17 20:52:47, grt (UTC plus 2) wrote:
> for devs like me who use Google Chrome and build Google Chrome, this will
delete
> the macs for my normal chrome install. i suppose this doesn't matter in the
> grand scheme since tests will overwrite my macs anyway, right?

Right.

[gab, 2017-05-17 21:01:19]

Description was changed from

==========
Cleanup PreferenceMACs on browser_tests startup
and prevent proliferation of scoped_dir subkeys under it in the future.

BUG=721245
==========

to

==========
Cleanup PreferenceMACs on browser_tests startup
and prevent proliferation of scoped_dir subkeys under it in the future.

BUG=721245
R=jam@chromium.org

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

[gab, 2017-05-17 21:01:20]

Committed patchset #5 (id:120001) manually as
dfcdd8a05d8e0867a68cfc366fb7e7d79b3a318f (presubmit successful).

[gab, 2017-05-17 21:03:39]

On 2017/05/17 21:01:20, gab wrote:
> Committed patchset #5 (id:120001) manually as
> dfcdd8a05d8e0867a68cfc366fb7e7d79b3a318f (presubmit successful).

Opted to land manually because this very issue is (I think) what's causing the
CQ to be slow (too much flakiness).

It also causes many tests to be marked as flaky and being disabled left and
right.

CQ run should still complete async so if this makes anything red on waterfall,
we'll be able to correlate.

[grt (UTC plus 2), 2017-05-17 21:14:23]

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

[grt (UTC plus 2), 2017-05-17 21:14:24]

It's my understanding that scoped_dirs left in %TEMP% by crashing tests are
cleaned up by a script on the bot that runs before tests rather than by in-test
code. Why is this case different?

I'm also a bit uneasy about test-specific code in the product for the Reset
case. Is there not a way that will keep all of the code in the test binaries?

[gab, 2017-05-17 21:24:09]

On 2017/05/17 21:14:24, grt (UTC plus 2) wrote:
> It's my understanding that scoped_dirs left in %TEMP% by crashing tests are
> cleaned up by a script on the bot that runs before tests rather than by
in-test
> code. Why is this case different?

It's done on bot startup IIUC. Coupling that further with specific Chrome
registry keys felt weird. Chrome should do its own cleanup.

> 
> I'm also a bit uneasy about test-specific code in the product for the Reset
> case. Is there not a way that will keep all of the code in the test binaries?

Yeah... I debated this too, some thoughts above on why I did it this way @
https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...

[grt (UTC plus 2), 2017-05-18 08:21:58]

https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
File services/preferences/tracked/registry_hash_store_contents_win.cc (right):

https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
services/preferences/tracked/registry_hash_store_contents_win.cc:76:
reset_on_delete_(base::StartsWith(store_key,
On 2017/05/17 20:01:05, gab wrote:
> On 2017/05/17 19:06:16, jam wrote:
> > I'm curious, it seems this cleanup code is quite far from the place that's
> > creating scoped_dir profiles. so there's a risk that as that code changes,
say
> > by updating the name, we will forget this part (and also the test cleanup).
> > 
> > is it possible to mark this object at the callsite that knows it's creating
a
> > scoped temp dir?
> 
> I debated this too... basically the problem is a test does
> CreateProfile(scoped_tmp_dir.path(), ...) and that dir eventually trickles
here.
> The "faulty" test has no idea it's even triggering this code so asking it to
> mark anything at the callsite will likely result in someone forgetting to
> eventually when writing such a test (their test will still pass but will cause
> this issue).

My suggestion on the bug was to create a ScopedProfile object for tests that
wraps a ScopedTempDir and does the appropriate registry cleanup. This will
require changing the existing tests, but it keeps the cleanup closer to where it
belongs. WDYT?

[gab, 2017-05-18 13:50:31]

On 2017/05/18 08:21:58, grt (UTC plus 2) wrote:
>
https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
> File services/preferences/tracked/registry_hash_store_contents_win.cc (right):
> 
>
https://codereview.chromium.org/2893703002/diff/60001/services/preferences/tr...
> services/preferences/tracked/registry_hash_store_contents_win.cc:76:
> reset_on_delete_(base::StartsWith(store_key,
> On 2017/05/17 20:01:05, gab wrote:
> > On 2017/05/17 19:06:16, jam wrote:
> > > I'm curious, it seems this cleanup code is quite far from the place that's
> > > creating scoped_dir profiles. so there's a risk that as that code changes,
> say
> > > by updating the name, we will forget this part (and also the test
cleanup).
> > > 
> > > is it possible to mark this object at the callsite that knows it's
creating
> a
> > > scoped temp dir?
> > 
> > I debated this too... basically the problem is a test does
> > CreateProfile(scoped_tmp_dir.path(), ...) and that dir eventually trickles
> here.
> > The "faulty" test has no idea it's even triggering this code so asking it to
> > mark anything at the callsite will likely result in someone forgetting to
> > eventually when writing such a test (their test will still pass but will
cause
> > this issue).
> 
> My suggestion on the bug was to create a ScopedProfile object for tests that
> wraps a ScopedTempDir and does the appropriate registry cleanup. This will
> require changing the existing tests, but it keeps the cleanup closer to where
it
> belongs. WDYT?

The problem is that such a ScopedProfile will not be *required* to make a test
that creates a profile in a scoped_dir pass. As such I'm afraid someone will
write such a test one day and we will end up here again a few months later...