Remove raw handles from base::win::RegKey

net/proxy/proxy_config_service_win.cc

Index: net/proxy/proxy_config_service_win.cc
diff --git a/net/proxy/proxy_config_service_win.cc b/net/proxy/proxy_config_service_win.cc
index 32553db6acba0b26273ccbb05681fecaf910d9d9..7770e356f68d4c148af59395bca7304fb5e2709b 100644
--- a/net/proxy/proxy_config_service_win.cc
+++ b/net/proxy/proxy_config_service_win.cc
@@ -7,6 +7,8 @@
 #include <windows.h>
 #include <winhttp.h>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/profiler/scoped_profile.h"
@@ -16,6 +18,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/win/registry.h"
+#include "base/win/scoped_handle.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/proxy_config.h"
 
@@ -38,36 +41,6 @@ void FreeIEConfig(WINHTTP_CURRENT_USER_IE_PROXY_CONFIG* ie_config) {
 
 }  // namespace
 
-// RegKey and ObjectWatcher pair.
-class ProxyConfigServiceWin::KeyEntry {
- public:
-  bool StartWatching(base::win::ObjectWatcher::Delegate* delegate) {
-    // Try to create a watch event for the registry key (which watches the
-    // sibling tree as well).
-    if (key_.StartWatching() != ERROR_SUCCESS)
-      return false;
-
-    // Now setup an ObjectWatcher for this event, so we get OnObjectSignaled()
-    // invoked on this message loop once it is signalled.
-    if (!watcher_.StartWatching(key_.watch_event(), delegate))
-      return false;
-
-    return true;
-  }
-
-  bool CreateRegKey(HKEY rootkey, const wchar_t* subkey) {
-    return key_.Create(rootkey, subkey, KEY_NOTIFY) == ERROR_SUCCESS;
-  }
-
-  HANDLE watch_event() const {
-    return key_.watch_event();
-  }
-
- private:
-  base::win::RegKey key_;
-  base::win::ObjectWatcher watcher_;
-};
-
 ProxyConfigServiceWin::ProxyConfigServiceWin()
     : PollingProxyConfigService(
           base::TimeDelta::FromSeconds(kPollIntervalSec),
@@ -125,18 +98,21 @@ void ProxyConfigServiceWin::StartWatchingRegistryForChanges() {
 
 bool ProxyConfigServiceWin::AddKeyToWatchList(HKEY rootkey,
                                               const wchar_t* subkey) {
-  scoped_ptr<KeyEntry> entry(new KeyEntry);
-  if (!entry->CreateRegKey(rootkey, subkey))
+  scoped_ptr<base::win::RegKey> entry(new base::win::RegKey);

[eroman, 2014/10/10 21:34:35]

rename entry --> key

+  if (entry->Create(rootkey, subkey, KEY_NOTIFY) != ERROR_SUCCESS)
     return false;
 
-  if (!entry->StartWatching(this))
+  if (!entry->StartWatching(base::Bind(&ProxyConfigServiceWin::OnObjectSignaled,
+                                       base::Unretained(this),
+                                       base::Unretained(entry.get())))) {
     return false;
+  }
 
   keys_to_watch_.push_back(entry.release());
   return true;
 }
 
-void ProxyConfigServiceWin::OnObjectSignaled(HANDLE object) {
+void ProxyConfigServiceWin::OnObjectSignaled(base::win::RegKey* key) {
   // TODO(vadimt): Remove ScopedProfile below once crbug.com/418183 is fixed.
   tracked_objects::ScopedProfile tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
@@ -145,15 +121,18 @@ void ProxyConfigServiceWin::OnObjectSignaled(HANDLE object) {
   // Figure out which registry key signalled this change.
   KeyEntryList::iterator it;

[eroman, 2014/10/10 21:41:58]

Looks like the pre-existing code is bad and doesn't initialize "it".

I suggest changing this to
 it = std::find(keys_to_watch_.begin(), keys_to_watch_.end(), key);

[rvargas (doing something else), 2014/10/10 22:22:05]

dos it on the loop
done

[eroman, 2014/10/10 22:35:12]

You are right about the loop part; brain fail on my part :(

Thanks for changing to std::find() at any rate!

   for (it = keys_to_watch_.begin(); it != keys_to_watch_.end(); ++it) {
-    if ((*it)->watch_event() == object)
+    if ((*it) == key)
       break;
   }
 
   DCHECK(it != keys_to_watch_.end());
 
   // Keep watching the registry key.
-  if (!(*it)->StartWatching(this))
+  if (!key->StartWatching(base::Bind(&ProxyConfigServiceWin::OnObjectSignaled,
+                                     base::Unretained(this),
+                                     base::Unretained(key)))) {
     keys_to_watch_.erase(it);

[eroman, 2014/10/10 21:34:35]

Looks like the pre-existing code is bad, and is leaking the entry.
It is a pre-existing bug, but do you want to address it as part of this change?

[rvargas (doing something else), 2014/10/10 22:22:05]

Done.

+  }
 
   // Have the PollingProxyConfigService test for changes.
   CheckForChangesNow();