Delete Chrome state in registry on uninstall when the user profile is being deleted.

Delete Chrome state in registry on uninstall when the user profile is being deleted.

BUG=none
R=robertshield@chromium.org

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

[grt (UTC plus 2), 2015-09-03 18:05:45]

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

[commit-bot: I haz the power, 2015-09-03 18:07:02]

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

[commit-bot: I haz the power, 2015-09-03 18:19:53]

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

[commit-bot: I haz the power, 2015-09-03 18:19:54]

Dry run: This issue passed the CQ dry run.

[robertshield, 2015-09-03 20:23:34]

Couple of initial thoughts, whaddya think?

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:523: nullptr, nullptr, nullptr, nullptr);
This may work, but the thing that gives me pause is this snippet from MSDN:

"While an application is using the RegEnumKeyEx function, it should not make
calls to any registration functions that might change the key being enumerated."
(source: https://msdn.microsoft.com/en-us/library/windows/desktop/ms724862)

Would it work to RegEnumKeyEx to build a snapshot of the registry sub tree under
|key|, then build up a list of things to delete, then apply that?

This would race against concurrent modifications, but given that this is (I
infer) best effort clean up code I think that should be ok.

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:566: to_skip.insert(name);
Hrmm, needing to preserve a list of items to skip to account for errors makes me
think that the snapshot-then-remove approach will be easier to reason about.

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:574: if (to_skip.empty()) {
doesn't DeleteEmptyKey already check whether the key is empty?

[grt (UTC plus 2), 2015-09-04 02:22:48]

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:523: nullptr, nullptr, nullptr, nullptr);
On 2015/09/03 20:23:33, robertshield wrote:
> This may work, but the thing that gives me pause is this snippet from MSDN:
> 
> "While an application is using the RegEnumKeyEx function, it should not make
> calls to any registration functions that might change the key being
enumerated."
> (source: https://msdn.microsoft.com/en-us/library/windows/desktop/ms724862)

That sentence is saying "hey, the enumeration order isn't stable, so if you
add/delete a subkey, the indexing will be all wacko!" There's no state
associated with EnumKeyEx aside from the index, so there is no possibility for
any strange behavior if you don't have any expectations about the ordering of
subkeys after adding/removing them.

As you point out, MSDN is misleading: even if you don't add/remove while
enumerating, another proc could, so you really have no hope of making a single
pass (let along two) work 100% of the time.

The approach I took here does multiple iterations until the desired end state is
reached: all subkeys are removed except for the ones we really want to preserve
and the ones we fail to delete.

> Would it work to RegEnumKeyEx to build a snapshot of the registry sub tree
under
> |key|, then build up a list of things to delete, then apply that?
> 
> This would race against concurrent modifications, but given that this is (I
> infer) best effort clean up code I think that should be ok.

If you push me to switch to that, I will. Otherwise, what I have here is more
efficient and more correct. Simplifying it would sacrifice both of those.

https://codereview.chromium.org/1327793004/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:574: if (to_skip.empty()) {
On 2015/09/03 20:23:34, robertshield wrote:
> doesn't DeleteEmptyKey already check whether the key is empty?

Yes, though there's no need to call it if you know that the key isn't empty.

[grt (UTC plus 2), 2015-09-04 13:53:49]

I'll take a swing at the approach you suggested so we can look at the two side
by side.

[grt (UTC plus 2), 2015-09-06 12:04:50]

Robert: just curious: are you waiting to take another look at this with the
proposed changes? Thanks.

[robertshield, 2015-09-07 00:16:20]

Ah, sorry, wasn't sure if you were trying the other thing. The code here does
LGTM.

[grt (UTC plus 2), 2015-09-07 13:10:24]

On 2015/09/07 00:16:20, robertshield wrote:
> Ah, sorry, wasn't sure if you were trying the other thing. The code here does
> LGTM.

Thanks. I'll land this now. We can land the other variant later if I do it and
it's simpler.

[grt (UTC plus 2), 2015-09-07 13:10:31]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-09-07 13:10:34]

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

[commit-bot: I haz the power, 2015-09-07 13:36:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-09-07 13:37:00]

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