Pause UserInputMonitor when KeyboardLock is engaged

This CL addresses an issue where KeyboardLock stops working (on
Windows) when the microphone permission is granted.  I tracked this
down to the RawInput key handler created by the UserInputMonitor
instance used in the audio pipeline (for keypress noise
cancellation).  After discussing this with the audio team last year,
we decided to pause the UserInputMonitor when keyboard lock is
engaged (and resumed when keyboard lock is disengaged).

In order for the UserInputMonitor to know when the keyboard hook has
been engaged, I've added a KeyboardHookMonitor class that the keyboard
hook impl can use to notify observers of keyboard hook status changes.

Change-Id: Ib8a51a0a49d33c6989c9325739d071c26860f98d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2615666Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843142}

media/base/BUILD.gn

@@ -634,7 +634,10 @@ source_set("unit_tests") {
 
   if (is_win) {
     sources += [ "win/dxgi_device_scope_handle_unittest.cc" ]
-    deps += [ "//media/base/win:media_foundation_util" ]
+    deps += [
+      "//media/base/win:media_foundation_util",
+      "//ui/events:test_support",
+    ]
     libs = [
       "d3d11.lib",
       "mfplat.lib",

media/base/user_input_monitor_unittest.cc

@@ -22,6 +22,10 @@
 #include "ui/ozone/public/ozone_platform.h"  // nogncheck
 #endif
 
+#if defined(OS_WIN)
+#include "ui/events/test/keyboard_hook_monitor_utils.h"
+#endif
+
 namespace media {
 
 namespace {
@@ -110,4 +114,96 @@ TEST_F(UserInputMonitorTest, ReadWriteKeyPressMonitorCount) {
   EXPECT_EQ(count, ReadKeyPressMonitorCount(readonly_mapping));
 }
 
+#if defined(OS_WIN)
+
+//
+// Windows specific scenarios which require simulating keyboard hook events.
+//
+
+TEST_F(UserInputMonitorTest, BlockMonitoringAfterMonitoringEnabled) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI);
+
+  std::unique_ptr<UserInputMonitor> monitor = UserInputMonitor::Create(
+      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get());
+
+  if (!monitor)
+    return;
+
+  monitor->EnableKeyPressMonitoring();
+  ui::SimulateKeyboardHookRegistered();
+  ui::SimulateKeyboardHookUnregistered();
+  monitor->DisableKeyPressMonitoring();
+
+  monitor.reset();
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(UserInputMonitorTest, BlockMonitoringBeforeMonitoringEnabled) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI);
+
+  std::unique_ptr<UserInputMonitor> monitor = UserInputMonitor::Create(
+      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get());
+
+  if (!monitor)
+    return;
+
+  ui::SimulateKeyboardHookRegistered();
+  monitor->EnableKeyPressMonitoring();
+  ui::SimulateKeyboardHookUnregistered();
+  monitor->DisableKeyPressMonitoring();
+
+  monitor.reset();
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(UserInputMonitorTest, UnblockMonitoringAfterMonitoringDisabled) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI);
+
+  std::unique_ptr<UserInputMonitor> monitor = UserInputMonitor::Create(
+      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get());
+
+  if (!monitor)
+    return;
+
+  monitor->EnableKeyPressMonitoring();
+  ui::SimulateKeyboardHookRegistered();
+  monitor->DisableKeyPressMonitoring();
+  ui::SimulateKeyboardHookUnregistered();
+
+  monitor.reset();
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(UserInputMonitorTest, BlockKeypressMonitoringWithSharedMemoryBuffer) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI);
+
+  std::unique_ptr<UserInputMonitor> monitor = UserInputMonitor::Create(
+      base::ThreadTaskRunnerHandle::Get(), base::ThreadTaskRunnerHandle::Get());
+
+  if (!monitor)
+    return;
+
+  base::ReadOnlySharedMemoryMapping readonly_mapping =
+      static_cast<UserInputMonitorBase*>(monitor.get())
+          ->EnableKeyPressMonitoringWithMapping()
+          .Map();
+  EXPECT_EQ(0u, ReadKeyPressMonitorCount(readonly_mapping));
+  ui::SimulateKeyboardHookRegistered();
+  EXPECT_EQ(0u, ReadKeyPressMonitorCount(readonly_mapping));
+  ui::SimulateKeyboardHookUnregistered();
+  EXPECT_EQ(0u, ReadKeyPressMonitorCount(readonly_mapping));
+  monitor->DisableKeyPressMonitoring();
+
+  monitor.reset();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that read only region remains valid after disable.
+  EXPECT_EQ(0u, ReadKeyPressMonitorCount(readonly_mapping));
+}
+#endif  // defined(OS_WIN)
+
 }  // namespace media

media/base/user_input_monitor_win.cc

@@ -20,6 +20,8 @@
 #include "third_party/skia/include/core/SkPoint.h"
 #include "ui/events/keyboard_event_counter.h"
 #include "ui/events/keycodes/keyboard_code_conversion_win.h"
+#include "ui/events/win/keyboard_hook_monitor.h"
+#include "ui/events/win/keyboard_hook_observer.h"
 
 namespace media {
 namespace {
@@ -41,7 +43,8 @@ std::unique_ptr<RAWINPUTDEVICE> GetRawInputDevices(HWND hwnd, DWORD flags) {
 // UserInputMonitorWin since it needs to be deleted on the UI thread.
 class UserInputMonitorWinCore
     : public base::SupportsWeakPtr<UserInputMonitorWinCore>,
-      public base::CurrentThread::DestructionObserver {
+      public base::CurrentThread::DestructionObserver,
+      public ui::KeyboardHookObserver {
  public:
   enum EventBitMask {
     MOUSE_EVENT_MASK = 1,
@@ -55,6 +58,10 @@ class UserInputMonitorWinCore
   // DestructionObserver overrides.
   void WillDestroyCurrentMessageLoop() override;
 
+  // KeyboardHookObserver implementation.
+  void OnHookRegistered() override;
+  void OnHookUnregistered() override;
+
   uint32_t GetKeyPressCount() const;
   void StartMonitor();
   void StartMonitorWithMapping(base::WritableSharedMemoryMapping mapping);
@@ -69,6 +76,9 @@ class UserInputMonitorWinCore
                      LPARAM lparam,
                      LRESULT* result);
 
+  void CreateRawInputWindow();
+  void DestroyRawInputWindow();
+
   // Task runner on which |window_| is created.
   scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
 
@@ -79,6 +89,9 @@ class UserInputMonitorWinCore
   std::unique_ptr<base::win::MessageWindow> window_;
   ui::KeyboardEventCounter counter_;
 
+  bool pause_monitoring_ = false;
+  bool start_monitoring_after_hook_removed_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(UserInputMonitorWinCore);
 };
 
@@ -106,10 +119,17 @@ class UserInputMonitorWin : public UserInputMonitorBase {
 
 UserInputMonitorWinCore::UserInputMonitorWinCore(
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
-    : ui_task_runner_(ui_task_runner) {}
+    : ui_task_runner_(ui_task_runner) {
+  // Register this instance with the KeyboardHookMonitor to listen for changes
+  // in the KeyboardHook registration state.  Since this instance may have been
+  // constructed after a hook was registered, check the current state as well.
+  ui::KeyboardHookMonitor::GetInstance()->AddObserver(this);
+  pause_monitoring_ = ui::KeyboardHookMonitor::GetInstance()->IsActive();
+}
 
 UserInputMonitorWinCore::~UserInputMonitorWinCore() {
   DCHECK(!window_);
+  ui::KeyboardHookMonitor::GetInstance()->RemoveObserver(this);
 }
 
 void UserInputMonitorWinCore::WillDestroyCurrentMessageLoop() {
@@ -124,6 +144,54 @@ uint32_t UserInputMonitorWinCore::GetKeyPressCount() const {
 void UserInputMonitorWinCore::StartMonitor() {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
 
+  if (pause_monitoring_) {
+    start_monitoring_after_hook_removed_ = true;
+    return;
+  }
+
+  CreateRawInputWindow();
+}
+
+void UserInputMonitorWinCore::StartMonitorWithMapping(
+    base::WritableSharedMemoryMapping mapping) {
+  StartMonitor();
+  key_press_count_mapping_ =
+      std::make_unique<base::WritableSharedMemoryMapping>(std::move(mapping));
+}
+
+void UserInputMonitorWinCore::StopMonitor() {
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+
+  DestroyRawInputWindow();
+  start_monitoring_after_hook_removed_ = false;
+
+  key_press_count_mapping_.reset();
+}
+
+void UserInputMonitorWinCore::OnHookRegistered() {
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  DCHECK(!pause_monitoring_);
+  pause_monitoring_ = true;
+
+  // Don't destroy |key_press_count_mapping_| as this is a temporary block and
+  // we want to allow monitoring to continue using the same shared memory once
+  // monitoring is unblocked.
+  DestroyRawInputWindow();
+}
+
+void UserInputMonitorWinCore::OnHookUnregistered() {
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  DCHECK(pause_monitoring_);
+  pause_monitoring_ = false;
+
+  if (start_monitoring_after_hook_removed_) {
+    start_monitoring_after_hook_removed_ = false;
+    StartMonitor();
+  }
+}
+
+void UserInputMonitorWinCore::CreateRawInputWindow() {
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
   if (window_)
     return;
 
@@ -149,16 +217,8 @@ void UserInputMonitorWinCore::StartMonitor() {
   base::CurrentThread::Get()->AddDestructionObserver(this);
 }
 
-void UserInputMonitorWinCore::StartMonitorWithMapping(
-    base::WritableSharedMemoryMapping mapping) {
-  StartMonitor();
-  key_press_count_mapping_ =
-      std::make_unique<base::WritableSharedMemoryMapping>(std::move(mapping));
-}
-
-void UserInputMonitorWinCore::StopMonitor() {
+void UserInputMonitorWinCore::DestroyRawInputWindow() {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
-
   if (!window_)
     return;
 
@@ -168,11 +228,8 @@ void UserInputMonitorWinCore::StopMonitor() {
   if (!RegisterRawInputDevices(device.get(), 1, sizeof(*device))) {
     PLOG(INFO) << "RegisterRawInputDevices() failed for RIDEV_REMOVE";
   }
-
   window_ = nullptr;
 
-  key_press_count_mapping_.reset();
-
   // Stop observing message loop destruction if no event is being monitored.
   base::CurrentThread::Get()->RemoveDestructionObserver(this);
 }

ui/events/BUILD.gn

@@ -229,6 +229,8 @@ component("events") {
   if (is_win) {
     public += [
       "win/events_win_utils.h",
+      "win/keyboard_hook_monitor.h",
+      "win/keyboard_hook_observer.h",
       "win/system_event_state_lookup.h",
     ]
 
@@ -237,6 +239,8 @@ component("events") {
       "keycodes/platform_key_map_win.h",
       "win/events_win.cc",
       "win/events_win_utils.cc",
+      "win/keyboard_hook_monitor_impl.cc",
+      "win/keyboard_hook_monitor_impl.h",
       "win/keyboard_hook_win_base.cc",
       "win/keyboard_hook_win_base.h",
       "win/media_keyboard_hook_win.cc",
@@ -487,7 +491,11 @@ static_library("test_support") {
   ]
 
   if (is_win) {
-    sources += [ "test/keyboard_layout_win.cc" ]
+    sources += [
+      "test/keyboard_hook_monitor_utils.cc",
+      "test/keyboard_hook_monitor_utils.h",
+      "test/keyboard_layout_win.cc",
+    ]
   }
 
   if (is_mac) {

ui/events/test/keyboard_hook_monitor_utils.cc

+// Copyright 2021 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 "ui/events/test/keyboard_hook_monitor_utils.h"
+
+#include "ui/events/win/keyboard_hook_monitor_impl.h"
+
+namespace ui {
+
+void SimulateKeyboardHookRegistered() {
+  KeyboardHookMonitorImpl::GetInstance()->NotifyHookRegistered();
+}
+
+void SimulateKeyboardHookUnregistered() {
+  KeyboardHookMonitorImpl::GetInstance()->NotifyHookUnregistered();
+}
+
+}  // namespace ui

ui/events/test/keyboard_hook_monitor_utils.h

+// Copyright 2021 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.
+
+#ifndef UI_EVENTS_TEST_KEYBOARD_HOOK_MONITOR_UTILS_H_
+#define UI_EVENTS_TEST_KEYBOARD_HOOK_MONITOR_UTILS_H_
+
+namespace ui {
+
+// Simulates a KeyboardHook being registered by calling the appropriate method
+// on the KeyboardHookMonitorImpl instance.
+// This method is exposed to allow testing this behavior outside of //ui/events.
+void SimulateKeyboardHookRegistered();
+
+// Simulates a KeyboardHook being unregistered by calling the appropriate method
+// on the KeyboardHookMonitorImpl instance.
+// This method is exposed to allow testing this behavior outside of //ui/events.
+void SimulateKeyboardHookUnregistered();
+
+}  // namespace ui
+
+#endif  // UI_EVENTS_TEST_KEYBOARD_HOOK_MONITOR_UTILS_H_

ui/events/win/keyboard_hook_monitor.h

+// Copyright 2021 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.
+
+#ifndef UI_EVENTS_WIN_KEYBOARD_HOOK_MONITOR_H_
+#define UI_EVENTS_WIN_KEYBOARD_HOOK_MONITOR_H_
+
+#include "ui/events/events_export.h"
+
+namespace ui {
+
+class KeyboardHookObserver;
+
+// Provides a way to receive notifications when a low-level keyboard hook is
+// registered or unregistered and to query hook registration status.
+// Note that the KeyboardHookMonitor impl is bound to the thread which creates
+// it.  In the browser process, this will be the UI thread.  All methods should
+// be called on that thread and all observer methods will be run on that thread.
+class EVENTS_EXPORT KeyboardHookMonitor {
+ public:
+  static KeyboardHookMonitor* GetInstance();
+
+  // Add an observer which will receive keyboard hook event notifications.  All
+  // |observer| methods are called on the thread which creates the
+  // KeyboardHookMonitor instance.
+  virtual void AddObserver(KeyboardHookObserver* observer) = 0;
+
+  // Remove an observer so that it will no longer receive keyboard hook event
+  // notifications.
+  virtual void RemoveObserver(KeyboardHookObserver* observer) = 0;
+
+  // Indicates whether a keyboard hook has been registered and is ready to
+  // receive keyboard events.
+  virtual bool IsActive() = 0;
+
+ protected:
+  virtual ~KeyboardHookMonitor() = default;
+};
+
+}  // namespace ui
+
+#endif  // UI_EVENTS_WIN_KEYBOARD_HOOK_MONITOR_H_

ui/events/win/keyboard_hook_monitor_impl.cc

+// Copyright 2021 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 "ui/events/win/keyboard_hook_monitor_impl.h"
+
+namespace ui {
+
+KeyboardHookMonitorImpl::KeyboardHookMonitorImpl() = default;
+
+KeyboardHookMonitorImpl::~KeyboardHookMonitorImpl() = default;
+
+void KeyboardHookMonitorImpl::AddObserver(KeyboardHookObserver* observer) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  observers_.AddObserver(observer);
+}
+
+void KeyboardHookMonitorImpl::RemoveObserver(KeyboardHookObserver* observer) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  observers_.RemoveObserver(observer);
+}
+
+bool KeyboardHookMonitorImpl::IsActive() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  return is_hook_active_;
+}
+
+void KeyboardHookMonitorImpl::NotifyHookRegistered() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(!is_hook_active_);
+
+  is_hook_active_ = true;
+  for (auto& observer : observers_) {
+    observer.OnHookRegistered();
+  }
+}
+
+void KeyboardHookMonitorImpl::NotifyHookUnregistered() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(is_hook_active_);
+
+  is_hook_active_ = false;
+  for (auto& observer : observers_) {
+    observer.OnHookUnregistered();
+  }
+}
+
+// static
+KeyboardHookMonitor* KeyboardHookMonitor::GetInstance() {
+  return reinterpret_cast<KeyboardHookMonitor*>(
+      KeyboardHookMonitorImpl::GetInstance());
+}
+
+// static
+KeyboardHookMonitorImpl* KeyboardHookMonitorImpl::GetInstance() {
+  static base::NoDestructor<KeyboardHookMonitorImpl> instance;
+  return instance.get();
+}
+
+}  // namespace ui

ui/events/win/keyboard_hook_monitor_impl.h

+// Copyright 2021 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.
+
+#ifndef UI_EVENTS_WIN_KEYBOARD_HOOK_MONITOR_IMPL_H_
+#define UI_EVENTS_WIN_KEYBOARD_HOOK_MONITOR_IMPL_H_
+
+#include "base/no_destructor.h"
+#include "base/observer_list_threadsafe.h"
+#include "base/threading/thread_checker.h"
+#include "ui/events/events_export.h"
+#include "ui/events/win/keyboard_hook_monitor.h"
+#include "ui/events/win/keyboard_hook_observer.h"
+
+namespace ui {
+
+// An implementation of the KeyboardHookMonitor interface which is used to
+// notify listeners of KeyboardHook registration events and provide status.
+// Note that this class is bound to the thread which creates it.  In the browser
+// process, this will be the UI thread.  All methods should be called on that
+// thread and all observer methods will be run on that thread.
+class EVENTS_EXPORT KeyboardHookMonitorImpl : public KeyboardHookMonitor {
+ public:
+  static KeyboardHookMonitorImpl* GetInstance();
+
+  KeyboardHookMonitorImpl(const KeyboardHookMonitorImpl&) = delete;
+  KeyboardHookMonitorImpl& operator=(const KeyboardHookMonitorImpl&) = delete;
+  KeyboardHookMonitorImpl(KeyboardHookMonitorImpl&&) = delete;
+  KeyboardHookMonitorImpl& operator=(KeyboardHookMonitorImpl&&) = delete;
+  ~KeyboardHookMonitorImpl() override;
+
+  // KeyboardHookMonitor implementation.
+  void AddObserver(KeyboardHookObserver* observer) override;
+  void RemoveObserver(KeyboardHookObserver* observer) override;
+  bool IsActive() override;
+
+  // Send a notification to all listeners in |observers_| to indicate that a
+  // keyboard hook has been registered and is listening for key events.
+  void NotifyHookRegistered();
+
+  // Send a notification to all listeners in |observers_| to indicate that a
+  // keyboard hook has been removed and is no longer listening to key events.
+  void NotifyHookUnregistered();
+
+ private:
+  friend base::NoDestructor<KeyboardHookMonitorImpl>;
+  KeyboardHookMonitorImpl();
+
+  bool is_hook_active_ = false;
+  base::ObserverList<KeyboardHookObserver> observers_;
+
+  THREAD_CHECKER(thread_checker_);
+};
+
+}  // namespace ui
+
+#endif  // UI_EVENTS_WIN_KEYBOARD_HOOK_MONITOR_IMPL_H_

ui/events/win/keyboard_hook_observer.h

+// Copyright 2021 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.
+
+#ifndef UI_EVENTS_WIN_KEYBOARD_HOOK_OBSERVER_H_
+#define UI_EVENTS_WIN_KEYBOARD_HOOK_OBSERVER_H_
+
+#include "ui/events/events_export.h"
+
+namespace ui {
+
+// Used in conjunction with the KeyboardHookMonitor class to receive
+// notifications when a low-level keyboard hook is registered or unregistered.
+class EVENTS_EXPORT KeyboardHookObserver : public base::CheckedObserver {
+ public:
+  // Called when a low-level keyboard hook is registered.
+  virtual void OnHookRegistered() {}
+
+  // Called when a low-level keyboard hook is unregistered.
+  virtual void OnHookUnregistered() {}
+
+ protected:
+  ~KeyboardHookObserver() override = default;
+};
+
+}  // namespace ui
+
+#endif  // UI_EVENTS_WIN_KEYBOARD_HOOK_OBSERVER_H_

ui/events/win/modifier_keyboard_hook_win.cc

@@ -15,6 +15,7 @@
 #include "ui/events/keycodes/dom/keycode_converter.h"
 #include "ui/events/keycodes/keyboard_code_conversion.h"
 #include "ui/events/win/events_win_utils.h"
+#include "ui/events/win/keyboard_hook_monitor_impl.h"
 #include "ui/gfx/native_widget_types.h"
 
 namespace ui {
@@ -131,6 +132,8 @@ class ModifierKeyboardHookWinImpl : public KeyboardHookWinBase {
 
   void ClearModifierStates();
 
+  KeyboardHookMonitorImpl* GetKeyboardHookMonitor();
+
   static ModifierKeyboardHookWinImpl* instance_;
 
   // Tracks the last non-located key down seen in order to determine if the
@@ -165,6 +168,8 @@ ModifierKeyboardHookWinImpl::~ModifierKeyboardHookWinImpl() {
 
   DCHECK_EQ(instance_, this);
   instance_ = nullptr;
+
+  KeyboardHookMonitorImpl::GetInstance()->NotifyHookUnregistered();
 }
 
 bool ModifierKeyboardHookWinImpl::Register() {
@@ -172,6 +177,8 @@ bool ModifierKeyboardHookWinImpl::Register() {
   DCHECK(!instance_);
   instance_ = this;
 
+  KeyboardHookMonitorImpl::GetInstance()->NotifyHookRegistered();
+
   return KeyboardHookWinBase::Register(reinterpret_cast<HOOKPROC>(
       &ModifierKeyboardHookWinImpl::ProcessKeyEvent));
 }
@@ -320,6 +327,7 @@ std::unique_ptr<KeyboardHook> KeyboardHook::CreateModifierKeyboardHook(
   return keyboard_hook;
 }
 
+// static
 std::unique_ptr<KeyboardHookWinBase>
 KeyboardHookWinBase::CreateModifierKeyboardHookForTesting(
     base::Optional<base::flat_set<DomCode>> dom_codes,