[Ext Registry] Do not try to re-watch registry if we are already watching it.

[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code. After we started calling StartLoading > 1nce, we unintentionally
started retrying to observe windows registry if previous observing
failed for some reason. This CL also removes this hard-to-explain retry. The
reasoning is that if we want to retry observing registry, we should be doing
that explicitly (and periodically?) and not rely on whether additional StartLoading
was called or not.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.

Review-Url: https://codereview.chromium.org/2796783002
Cr-Commit-Position: refs/heads/master@{#461903}
Committed: https://chromium.googlesource.com/chromium/src/+/36920277941e915314f558ada0a27236c633f751

[lazyboy, 2017-04-04 01:27:29]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2017-04-04 01:44:07]

Any chance of a test? :)

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_loader_win.cc:208: if
(!is_watching_hklm_) {
Do we want these (this and is_watching_hkcu_) as separate bools?  If so, why? 
It doesn't seem like we have logic to try again in the case of an error result
(unless we happen to call StartLoading() again).

[lazyboy, 2017-04-04 18:45:10]

Added a unit test.
Ideally, ContentVerifierPolicyTest.PolicyCorruption* test would have covered
this restart after corruption case, but unfortunately we do not use a registry
provider in tests (for obvious trickiness involved in it).

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_loader_win.cc:208: if
(!is_watching_hklm_) {
On 2017/04/04 01:44:07, Devlin wrote:
> Do we want these (this and is_watching_hkcu_) as separate bools?  If so, why? 
> It doesn't seem like we have logic to try again in the case of an error result
> (unless we happen to call StartLoading() again).

These can independently fail. Or at least HKLM can easily fail, that's why I
resorted to two bools.

Right, we do not have logic to retry and only we have an opportunity to retry if
StartLoading is called.

[Devlin, 2017-04-04 20:48:36]

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_loader_win.cc:208: if
(!is_watching_hklm_) {
On 2017/04/04 18:45:10, lazyboy wrote:
> On 2017/04/04 01:44:07, Devlin wrote:
> > Do we want these (this and is_watching_hkcu_) as separate bools?  If so,
why? 
> > It doesn't seem like we have logic to try again in the case of an error
result
> > (unless we happen to call StartLoading() again).
> 
> These can independently fail. Or at least HKLM can easily fail, that's why I
> resorted to two bools.
> 
> Right, we do not have logic to retry and only we have an opportunity to retry
if
> StartLoading is called.

So, my question here is more around whether or not calling StartLoading() twice
*should* have potential side effects.  Originally, I'd think not, but as it is
now, it could - a watcher that previously failed could be restarted.  I'm not
sure that's a bad thing; it just seems interesting that this would be how we
address it.  Is that change intentional?  If so, maybe worth a comment in the
code or the CL?

https://codereview.chromium.org/2796783002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/2796783002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/external_registry_loader_win.cc:234: LOG(WARNING) <<
"Function finished";
remove

[lazyboy, 2017-04-04 21:45:36]

Patchset #3 (id:40001) has been deleted

[lazyboy, 2017-04-04 21:45:45]

Patchset #3 (id:60001) has been deleted

[lazyboy, 2017-04-04 21:47:23]

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/2796783002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/external_registry_loader_win.cc:208: if
(!is_watching_hklm_) {
On 2017/04/04 20:48:35, Devlin wrote:
> On 2017/04/04 18:45:10, lazyboy wrote:
> > On 2017/04/04 01:44:07, Devlin wrote:
> > > Do we want these (this and is_watching_hkcu_) as separate bools?  If so,
> why? 
> > > It doesn't seem like we have logic to try again in the case of an error
> result
> > > (unless we happen to call StartLoading() again).
> > 
> > These can independently fail. Or at least HKLM can easily fail, that's why I
> > resorted to two bools.
> > 
> > Right, we do not have logic to retry and only we have an opportunity to
retry
> if
> > StartLoading is called.
> 
> So, my question here is more around whether or not calling StartLoading()
twice
> *should* have potential side effects.  Originally, I'd think not, but as it is
> now, it could - a watcher that previously failed could be restarted.  I'm not
> sure that's a bad thing;

Note that a watcher that previously failed would actually have been restarted
without this CL. But that was not intentional, more like a side effect we
inherited when we allowed StartLoading  to be called more than once.

Since this wasn't intentional, I think it's better to remove this one-off case
that doesn't seem too deterministic and is hard to reason about.

 it just seems interesting that this would be how we
> address it.  Is that change intentional?  If so, maybe worth a comment in the
> code or the CL?

https://codereview.chromium.org/2796783002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/external_registry_loader_win.cc (right):

https://codereview.chromium.org/2796783002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/external_registry_loader_win.cc:234: LOG(WARNING) <<
"Function finished";
On 2017/04/04 20:48:36, Devlin wrote:
> remove

Done.

[lazyboy, 2017-04-04 21:47:32]

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

[commit-bot: I haz the power, 2017-04-04 21:48:17]

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

[lazyboy, 2017-04-04 21:49:24]

Description was changed from

==========
[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.
==========

to

==========
[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code. After we started calling StartLoading > 1nce, we unintentionally
started retrying to observe windows registry if previous observing
failed for some reason. This CL also removes this hard-to-explain retry.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.
==========

[lazyboy, 2017-04-04 21:50:40]

Description was changed from

==========
[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code. After we started calling StartLoading > 1nce, we unintentionally
started retrying to observe windows registry if previous observing
failed for some reason. This CL also removes this hard-to-explain retry.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.
==========

to

==========
[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code. After we started calling StartLoading > 1nce, we unintentionally
started retrying to observe windows registry if previous observing
failed for some reason. This CL also removes this hard-to-explain retry. The
reasoning is that if we want to retry observing registry, we should be doing
that explicitly (and periodically?) and not rely on whether additional
StartLoading
was called or not.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.
==========

[Devlin, 2017-04-04 22:34:38]

lgtm

[commit-bot: I haz the power, 2017-04-04 23:27:54]

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

[commit-bot: I haz the power, 2017-04-04 23:27:55]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-04-04 23:41:36]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2017-04-04 23:42:34]

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-04-04 23:51:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491349296295320,
"parent_rev": "9fc34de2a20034026e1dd89525740b4239b1c541", "commit_rev":
"36920277941e915314f558ada0a27236c633f751"}

[commit-bot: I haz the power, 2017-04-04 23:52:46]

Description was changed from

==========
[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code. After we started calling StartLoading > 1nce, we unintentionally
started retrying to observe windows registry if previous observing
failed for some reason. This CL also removes this hard-to-explain retry. The
reasoning is that if we want to retry observing registry, we should be doing
that explicitly (and periodically?) and not rely on whether additional
StartLoading
was called or not.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.
==========

to

==========
[Ext Registry] Do not try to re-watch registry if we are already watching it.

Firstly, registry observer/watcher is already set up, then we do not need
to observe that again. Because we didn't use to call
ExternalLoader::StartLoading more than once before
https://crrev.com/2336403002, this wan't an issue when we added registry
watching code. After we started calling StartLoading > 1nce, we unintentionally
started retrying to observe windows registry if previous observing
failed for some reason. This CL also removes this hard-to-explain retry. The
reasoning is that if we want to retry observing registry, we should be doing
that explicitly (and periodically?) and not rely on whether additional
StartLoading
was called or not.

Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using
same RegKey. This is DCHECK(callback_.is_null()) in
RegKey::Watcher::StartWatching.

BUG=653045
Test=On windows, make chrome have a policy forced extension installed.
Shut down chrome. Manually corrupt the policy installed extension.
Re-laucnch chrome in debug mode: observe no more DCHECK failures.

Review-Url: https://codereview.chromium.org/2796783002
Cr-Commit-Position: refs/heads/master@{#461903}
Committed:
https://chromium.googlesource.com/chromium/src/+/36920277941e915314f558ada0a2...
==========

[commit-bot: I haz the power, 2017-04-04 23:52:47]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/36920277941e915314f558ada0a2...