Remove raw handles from base::win::RegKey

net/proxy/proxy_config_service_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 "net/proxy/proxy_config_service_win.h"
 
 #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"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/string_tokenizer.h"
 #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"
 
 #pragma comment(lib, "winhttp.lib")
 
 namespace net {
 
 namespace {
 
 const int kPollIntervalSec = 10;
 
 void FreeIEConfig(WINHTTP_CURRENT_USER_IE_PROXY_CONFIG* ie_config) {
   if (ie_config->lpszAutoConfigUrl)
     GlobalFree(ie_config->lpszAutoConfigUrl);
   if (ie_config->lpszProxy)
     GlobalFree(ie_config->lpszProxy);
   if (ie_config->lpszProxyBypass)
     GlobalFree(ie_config->lpszProxyBypass);
 }
 
 }  // 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),
           &ProxyConfigServiceWin::GetCurrentProxyConfig) {
 }
 
 ProxyConfigServiceWin::~ProxyConfigServiceWin() {
   // The registry functions below will end up going to disk.  Do this on another
   // thread to avoid slowing the IO thread.  http://crbug.com/61453
   base::ThreadRestrictions::ScopedAllowIO allow_io;
@@ skipping 37 matching lines @@
       L"Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings");
 
   AddKeyToWatchList(
       HKEY_LOCAL_MACHINE,
       L"SOFTWARE\\Policies\\Microsoft\\Windows\\CurrentVersion\\"
       L"Internet Settings");
 }
 
 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(
           "ProxyConfigServiceWin_OnObjectSignaled"));
 
   // 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();
 }
 
 // static
 void ProxyConfigServiceWin::GetCurrentProxyConfig(ProxyConfig* config) {
   WINHTTP_CURRENT_USER_IE_PROXY_CONFIG ie_config = {0};
   if (!WinHttpGetIEProxyConfigForCurrentUser(&ie_config)) {
     LOG(ERROR) << "WinHttpGetIEProxyConfigForCurrentUser failed: " <<
@@ skipping 26 matching lines @@
       std::string bypass_url_domain = proxy_server_bypass_list.token();
       config->proxy_rules().bypass_rules.AddRuleFromString(bypass_url_domain);
     }
   }
   if (ie_config.lpszAutoConfigUrl)
     config->set_pac_url(GURL(ie_config.lpszAutoConfigUrl));
   config->set_source(PROXY_CONFIG_SOURCE_SYSTEM);
 }
 
 }  // namespace net