Fix event monitoring teardown on window destruction

Add WindowMonitorAura; removes pre-target handler in OnWindowDestroying.
Fixes UAF when EventMonitors are destroyed late in window destruction.
(avoid EventTarget::RemovePreTargetHandler calls later in destruction)

Add a simple unit test.

Bug: 932922
Test: No crash when quitting Chrome with a fullscreen tab.
Change-Id: I2ace83b9de127d8fa70a57021dda94561f5bd4c4
Reviewed-on: https://chromium-review.googlesource.com/c/1487353Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635284}

ui/views/event_monitor_aura.cc

@@ -4,7 +4,10 @@
 
 #include "ui/views/event_monitor_aura.h"
 
+#include <memory>
+
 #include "base/logging.h"
+#include "base/scoped_observer.h"
 #include "ui/aura/env.h"
 #include "ui/aura/window.h"
 #include "ui/events/event_observer.h"
@@ -12,6 +15,38 @@
 
 namespace views {
 
+namespace {
+
+// An EventMonitorAura that removes its event observer on window destruction.
+class WindowMonitorAura : public EventMonitorAura, public aura::WindowObserver {
+ public:
+  WindowMonitorAura(aura::Env* env,
+                    ui::EventObserver* event_observer,
+                    aura::Window* target_window,
+                    const std::set<ui::EventType>& types)
+      : EventMonitorAura(env, event_observer, target_window, types),
+        target_window_(target_window) {
+    window_observer_.Add(target_window);
+  }
+  ~WindowMonitorAura() override = default;
+
+  // aura::WindowObserver:
+  void OnWindowDestroying(aura::Window* window) override {
+    DCHECK_EQ(window, target_window_);
+    window_observer_.Remove(target_window_);
+    target_window_ = nullptr;
+    TearDown();
+  }
+
+ private:
+  aura::Window* target_window_;
+  ScopedObserver<aura::Window, aura::WindowObserver> window_observer_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(WindowMonitorAura);
+};
+
+}  // namespace
+
 // static
 std::unique_ptr<EventMonitor> EventMonitor::CreateApplicationMonitor(
     ui::EventObserver* event_observer,
@@ -26,7 +61,7 @@ std::unique_ptr<EventMonitor> EventMonitor::CreateWindowMonitor(
     ui::EventObserver* event_observer,
     gfx::NativeWindow target_window,
     const std::set<ui::EventType>& types) {
-  return std::make_unique<EventMonitorAura>(
+  return std::make_unique<WindowMonitorAura>(
       target_window->env(), event_observer, target_window, types);
 }
 
@@ -42,11 +77,17 @@ EventMonitorAura::EventMonitorAura(aura::Env* env,
 }
 
 EventMonitorAura::~EventMonitorAura() {
-  env_->RemoveEventObserver(event_observer_);
+  TearDown();
 }
 
 gfx::Point EventMonitorAura::GetLastMouseLocation() {
   return env_->last_mouse_location();
 }
 
+void EventMonitorAura::TearDown() {
+  if (event_observer_)
+    env_->RemoveEventObserver(event_observer_);
+  event_observer_ = nullptr;
+}
+
 }  // namespace views

ui/views/event_monitor_aura.h

@@ -18,6 +18,7 @@ class EventTarget;
 
 namespace views {
 
+// Observes events by installing a pre-target handler on the ui::EventTarget.
 class EventMonitorAura : public EventMonitor {
  public:
   EventMonitorAura(aura::Env* env,
@@ -29,6 +30,10 @@ class EventMonitorAura : public EventMonitor {
   // EventMonitor:
   gfx::Point GetLastMouseLocation() override;
 
+ protected:
+  // Removes the pre-target handler. Called by window monitors on window close.
+  void TearDown();
+
  private:
   aura::Env* env_;                     // Weak.
   ui::EventObserver* event_observer_;  // Weak. Owned by our owner.

ui/views/event_monitor_unittest.cc

@@ -112,6 +112,18 @@ TEST_F(EventMonitorTest, ShouldOnlyReceiveRequestedEventTypes) {
   monitor.reset();
 }
 
+TEST_F(EventMonitorTest, WindowMonitorTornDownOnWindowClose) {
+  Widget* widget2 = CreateTopLevelNativeWidget();
+  widget2->Show();
+
+  std::unique_ptr<EventMonitor> monitor(EventMonitor::CreateWindowMonitor(
+      &observer_, widget2->GetNativeWindow(), {ui::ET_MOUSE_PRESSED}));
+
+  // Closing the widget before destroying the monitor should not crash.
+  widget2->CloseNow();
+  monitor.reset();
+}
+
 namespace {
 class DeleteOtherOnEventObserver : public ui::EventObserver {
  public: