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

chrome/browser/extensions/external_registry_loader_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/external_registry_loader_win.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
@@ skipping 36 matching lines @@
 
 std::string MakePrefName(const std::string& extension_id,
                          const std::string& pref_name) {
   return base::StringPrintf("%s.%s", extension_id.c_str(), pref_name.c_str());
 }
 
 }  // namespace
 
 namespace extensions {
 
+ExternalRegistryLoader::ExternalRegistryLoader()
+    : is_watching_hklm_(false), is_watching_hkcu_(false) {}
+
 void ExternalRegistryLoader::StartLoading() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&ExternalRegistryLoader::LoadOnFileThread, this));
 }
 
 std::unique_ptr<base::DictionaryValue>
 ExternalRegistryLoader::LoadPrefsOnFileThread() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ skipping 126 matching lines @@
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&ExternalRegistryLoader::CompleteLoadAndStartWatchingRegistry,
                  this));
 }
 
 void ExternalRegistryLoader::CompleteLoadAndStartWatchingRegistry() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   LoadFinished();
 
-  // Start watching registry.
+  // Start watching registry if we are not already.
   LONG result = ERROR_SUCCESS;
-  if ((result = hklm_key_.Create(HKEY_LOCAL_MACHINE, kRegistryExtensions,
-                                 KEY_NOTIFY | KEY_WOW64_32KEY)) ==
-      ERROR_SUCCESS) {
-    base::win::RegKey::ChangeCallback callback =
-        base::Bind(&ExternalRegistryLoader::OnRegistryKeyChanged,
-                   base::Unretained(this), base::Unretained(&hklm_key_));
-    hklm_key_.StartWatching(callback);
-  } else {
-    LOG(WARNING) << "Error observing HKLM: " << result;
+  if (!is_watching_hklm_) {

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

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]

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:35]

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?

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

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

+    if ((result = hklm_key_.Create(HKEY_LOCAL_MACHINE, kRegistryExtensions,
+                                   KEY_NOTIFY | KEY_WOW64_32KEY)) ==
+        ERROR_SUCCESS) {
+      base::win::RegKey::ChangeCallback callback =
+          base::Bind(&ExternalRegistryLoader::OnRegistryKeyChanged,
+                     base::Unretained(this), base::Unretained(&hklm_key_));
+      hklm_key_.StartWatching(callback);
+      is_watching_hklm_ = true;
+    } else {
+      LOG(WARNING) << "Error observing HKLM: " << result;
+    }
   }
 
-  if ((result = hkcu_key_.Create(HKEY_CURRENT_USER, kRegistryExtensions,
-                                 KEY_NOTIFY)) == ERROR_SUCCESS) {
-    base::win::RegKey::ChangeCallback callback =
-        base::Bind(&ExternalRegistryLoader::OnRegistryKeyChanged,
-                   base::Unretained(this), base::Unretained(&hkcu_key_));
-    hkcu_key_.StartWatching(callback);
-  } else {
-    LOG(WARNING) << "Error observing HKCU: " << result;
+  if (!is_watching_hkcu_) {
+    if ((result = hkcu_key_.Create(HKEY_CURRENT_USER, kRegistryExtensions,
+                                   KEY_NOTIFY)) == ERROR_SUCCESS) {
+      base::win::RegKey::ChangeCallback callback =
+          base::Bind(&ExternalRegistryLoader::OnRegistryKeyChanged,
+                     base::Unretained(this), base::Unretained(&hkcu_key_));
+      hkcu_key_.StartWatching(callback);
+      is_watching_hkcu_ = true;
+    } else {
+      LOG(WARNING) << "Error observing HKCU: " << result;
+    }
   }
 }
 
 void ExternalRegistryLoader::OnRegistryKeyChanged(base::win::RegKey* key) {
   // |OnRegistryKeyChanged| is removed as an observer when the ChangeCallback is
   // called, so we need to re-register.
   key->StartWatching(base::Bind(&ExternalRegistryLoader::OnRegistryKeyChanged,
                                 base::Unretained(this), base::Unretained(key)));
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&ExternalRegistryLoader::UpdatePrefsOnFileThread, this));
 }
 
 void ExternalRegistryLoader::UpdatePrefsOnFileThread() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   base::TimeTicks start_time = base::TimeTicks::Now();
   std::unique_ptr<base::DictionaryValue> prefs = LoadPrefsOnFileThread();
   LOCAL_HISTOGRAM_TIMES("Extensions.ExternalRegistryLoaderWinUpdate",
                         base::TimeTicks::Now() - start_time);
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&ExternalRegistryLoader::OnUpdated, this,
                                      base::Passed(&prefs)));
 }
 
 }  // namespace extensions