Remove raw handles from base::win::RegKey

BUG=419210, 423634
R=cpu@chromium.org, eroman@chromium.org, sky@chromium.org

Review URL: https://codereview.chromium.org/632833002

Cr-Commit-Position: refs/heads/master@{#299737}

3 files changed, 144 insertions, 60 deletions

diff --git a/base/win/registry.cc b/base/win/registry.cc
index e8fb892..8e7083f 100644
--- a/base/win/registry.cc
+++ b/base/win/registry.cc
@@ -34,23 +34,74 @@ const REGSAM kWow64AccessMask = KEY_WOW64_32KEY | KEY_WOW64_64KEY;
 
 }  // namespace
 
+// Watches for modifications to a key.
+class RegKey::Watcher : public ObjectWatcher::Delegate {
+ public:
+  explicit Watcher(RegKey* owner) : owner_(owner) {}
+  ~Watcher() {}
+
+  bool StartWatching(HKEY key, const ChangeCallback& callback);
+
+  // Implementation of ObjectWatcher::Delegate.
+  void OnObjectSignaled(HANDLE object) override {
+    DCHECK(watch_event_.IsValid() && watch_event_.Get() == object);
+    ChangeCallback callback = callback_;
+    callback_.Reset();
+    callback.Run();
+  }
+
+ private:
+  RegKey* owner_;
+  ScopedHandle watch_event_;
+  ObjectWatcher object_watcher_;
+  ChangeCallback callback_;
+  DISALLOW_COPY_AND_ASSIGN(Watcher);
+};
+
+bool RegKey::Watcher::StartWatching(HKEY key, const ChangeCallback& callback) {
+  DCHECK(key);
+  DCHECK(callback_.is_null());
+  if (GetVersion() < VERSION_VISTA) {
+    // It is an error to register multiple times before Vista.
+    if (watch_event_.IsValid()) {
+      callback_ = callback;
+      return true;
+    }
+  }
+
+  if (!watch_event_.IsValid())
+    watch_event_.Set(CreateEvent(NULL, TRUE, FALSE, NULL));
+
+  if (!watch_event_.IsValid())
+    return false;
+
+  DWORD filter = REG_NOTIFY_CHANGE_NAME |
+                 REG_NOTIFY_CHANGE_ATTRIBUTES |
+                 REG_NOTIFY_CHANGE_LAST_SET |
+                 REG_NOTIFY_CHANGE_SECURITY;
+
+  // Watch the registry key for a change of value.
+  LONG result = RegNotifyChangeKeyValue(key, TRUE, filter, watch_event_.Get(),
+                                        TRUE);
+  if (result != ERROR_SUCCESS) {
+    watch_event_.Close();
+    return false;
+  }
+
+  callback_ = callback;
+  return object_watcher_.StartWatching(watch_event_.Get(), this);
+}
+
 // RegKey ----------------------------------------------------------------------
 
-RegKey::RegKey()
-    : key_(NULL),
-      watch_event_(0),
-      wow64access_(0) {
+RegKey::RegKey() : key_(NULL), wow64access_(0) {
 }
 
-RegKey::RegKey(HKEY key)
-    : key_(key),
-      watch_event_(0),
-      wow64access_(0) {
+RegKey::RegKey(HKEY key) : key_(key), wow64access_(0) {
 }
 
 RegKey::RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access)
     : key_(NULL),
-      watch_event_(0),
       wow64access_(0) {
   if (rootkey) {
     if (access & (KEY_SET_VALUE | KEY_CREATE_SUB_KEY | KEY_CREATE_LINK))
@@ -150,7 +201,6 @@ LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) {
 }
 
 void RegKey::Close() {
-  StopWatching();
   if (key_) {
     ::RegCloseKey(key_);
     key_ = NULL;
@@ -168,7 +218,6 @@ void RegKey::Set(HKEY key) {
 
 HKEY RegKey::Take() {
   DCHECK(wow64access_ == 0);
-  StopWatching();
   HKEY key = key_;
   key_ = NULL;
   return key;
@@ -367,44 +416,14 @@ LONG RegKey::WriteValue(const wchar_t* name,
   return result;
 }
 
-LONG RegKey::StartWatching() {
-  DCHECK(key_);
-  if (!watch_event_)
-    watch_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
+bool RegKey::StartWatching(const ChangeCallback& callback) {
+  if (!key_watcher_)
+    key_watcher_.reset(new Watcher(this));
 
-  DWORD filter = REG_NOTIFY_CHANGE_NAME |
-                 REG_NOTIFY_CHANGE_ATTRIBUTES |
-                 REG_NOTIFY_CHANGE_LAST_SET |
-                 REG_NOTIFY_CHANGE_SECURITY;
+  if (!key_watcher_.get()->StartWatching(key_, callback))
+    return false;
 
-  // Watch the registry key for a change of value.
-  LONG result = RegNotifyChangeKeyValue(key_, TRUE, filter, watch_event_, TRUE);
-  if (result != ERROR_SUCCESS) {
-    CloseHandle(watch_event_);
-    watch_event_ = 0;
-  }
-
-  return result;
-}
-
-bool RegKey::HasChanged() {
-  if (watch_event_) {
-    if (WaitForSingleObject(watch_event_, 0) == WAIT_OBJECT_0) {
-      StartWatching();
-      return true;
-    }
-  }
-  return false;
-}
-
-LONG RegKey::StopWatching() {
-  LONG result = ERROR_INVALID_HANDLE;
-  if (watch_event_) {
-    CloseHandle(watch_event_);
-    watch_event_ = 0;
-    result = ERROR_SUCCESS;
-  }
-  return result;
+  return true;
 }
 
 // static
diff --git a/base/win/registry.h b/base/win/registry.h
index e5524b8..c3e015b 100644
--- a/base/win/registry.h
+++ b/base/win/registry.h
@@ -12,6 +12,8 @@
 #include "base/base_export.h"
 #include "base/basictypes.h"
 #include "base/stl_util.h"
+#include "base/win/object_watcher.h"
+#include "base/win/scoped_handle.h"
 
 namespace base {
 namespace win {
@@ -25,6 +27,9 @@ namespace win {
 // are not touched in case of failure.
 class BASE_EXPORT RegKey {
  public:
+  // Called from the MessageLoop when the key changes.
+  typedef base::Callback<void()> ChangeCallback;
+
   RegKey();
   explicit RegKey(HKEY key);
   RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access);
@@ -120,22 +125,16 @@ class BASE_EXPORT RegKey {
 
   // Starts watching the key to see if any of its values have changed.
   // The key must have been opened with the KEY_NOTIFY access privilege.
-  LONG StartWatching();
-
-  // If StartWatching hasn't been called, always returns false.
-  // Otherwise, returns true if anything under the key has changed.
-  // This can't be const because the |watch_event_| may be refreshed.
-  bool HasChanged();
-
-  // Will automatically be called by destructor if not manually called
-  // beforehand.  Returns true if it was watching, false otherwise.
-  LONG StopWatching();
+  // Returns true on success.
+  // To stop watching, delete this RegKey object. To continue watching the
+  // object after the callback is invoked, call StartWatching again.
+  bool StartWatching(const ChangeCallback& callback);
 
-  inline bool IsWatching() const { return watch_event_ != 0; }
-  HANDLE watch_event() const { return watch_event_; }
   HKEY Handle() const { return key_; }
 
  private:
+  class Watcher;
+
   // Calls RegDeleteKeyEx on supported platforms, alternatively falls back to
   // RegDeleteKey.
   static LONG RegDeleteKeyExWrapper(HKEY hKey,
@@ -147,9 +146,10 @@ class BASE_EXPORT RegKey {
   static LONG RegDelRecurse(HKEY root_key,
                             const std::wstring& name,
                             REGSAM access);
+
   HKEY key_;  // The registry key being iterated.
-  HANDLE watch_event_;
   REGSAM wow64access_;
+  scoped_ptr<Watcher> key_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(RegKey);
 };
diff --git a/base/win/registry_unittest.cc b/base/win/registry_unittest.cc
index d2610ef..6548474 100644
--- a/base/win/registry_unittest.cc
+++ b/base/win/registry_unittest.cc
@@ -7,7 +7,10 @@
 #include <cstring>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/win/windows_version.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -349,6 +352,68 @@ TEST_F(RegistryTest, OpenSubKey) {
   ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(L"foo"));
 }
 
+class TestChangeDelegate {
+ public:
+   TestChangeDelegate() : called_(false) {}
+   ~TestChangeDelegate() {}
+
+   void OnKeyChanged() {
+     MessageLoop::current()->QuitWhenIdle();
+     called_ = true;
+   }
+
+   bool WasCalled() {
+     bool was_called = called_;
+     called_ = false;
+     return was_called;
+   }
+
+ private:
+  bool called_;
+};
+
+TEST_F(RegistryTest, ChangeCallback) {
+  RegKey key;
+  TestChangeDelegate delegate;
+  MessageLoop message_loop;
+
+  std::wstring foo_key(kRootKey);
+  foo_key += L"\\Foo";
+  ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, foo_key.c_str(),
+                                      KEY_READ));
+
+  ASSERT_TRUE(key.StartWatching(Bind(&TestChangeDelegate::OnKeyChanged,
+                                     Unretained(&delegate))));
+  EXPECT_FALSE(delegate.WasCalled());
+
+  // Make some change.
+  RegKey key2;
+  ASSERT_EQ(ERROR_SUCCESS, key2.Open(HKEY_CURRENT_USER, foo_key.c_str(),
+                                      KEY_READ | KEY_SET_VALUE));
+  ASSERT_TRUE(key2.Valid());
+  EXPECT_EQ(ERROR_SUCCESS, key2.WriteValue(L"name", L"data"));
+
+  // Allow delivery of the notification.
+  EXPECT_FALSE(delegate.WasCalled());
+  base::RunLoop().Run();
+
+  ASSERT_TRUE(delegate.WasCalled());
+  EXPECT_FALSE(delegate.WasCalled());
+
+  ASSERT_TRUE(key.StartWatching(Bind(&TestChangeDelegate::OnKeyChanged,
+                                     Unretained(&delegate))));
+
+  // Change something else.
+  EXPECT_EQ(ERROR_SUCCESS, key2.WriteValue(L"name2", L"data2"));
+  base::RunLoop().Run();
+  ASSERT_TRUE(delegate.WasCalled());
+
+  ASSERT_TRUE(key.StartWatching(Bind(&TestChangeDelegate::OnKeyChanged,
+                                     Unretained(&delegate))));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(delegate.WasCalled());
+}
+
 }  // namespace
 
 }  // namespace win