Use WeakPtr in NativeWindowOcclusionTrackerWin

Bug: 1020136
Change-Id: I313a479663dda83c4bdf7d882911b029bbbe2cf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1901466
Commit-Queue: Tomasz Moniuszko <tmoniuszko@opera.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729717}

content/browser/web_contents/web_contents_view_aura_unittest.cc

@@ -315,13 +315,7 @@ TEST_F(WebContentsViewAuraTest, DragDropFilesOriginateFromRenderer) {
 
 #if defined(OS_WIN)
 
-// Flaky crash on ASan: http://crbug.com/1020136
-#if defined(ADDRESS_SANITIZER)
-#define MAYBE_DragDropVirtualFiles DISABLED_DragDropVirtualFiles
-#else
-#define MAYBE_DragDropVirtualFiles DragDropVirtualFiles
-#endif
-TEST_F(WebContentsViewAuraTest, MAYBE_DragDropVirtualFiles) {
+TEST_F(WebContentsViewAuraTest, DragDropVirtualFiles) {
   WebContentsViewAura* view = GetView();
   auto data = std::make_unique<ui::OSExchangeData>();
 

ui/aura/native_window_occlusion_tracker_win.cc

@@ -7,7 +7,9 @@
 #include <memory>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -26,10 +28,15 @@ namespace {
 const base::TimeDelta kUpdateOcclusionDelay =
     base::TimeDelta::FromMilliseconds(16);
 
+// This global variable can be accessed only on main thread.
 NativeWindowOcclusionTrackerWin* g_tracker = nullptr;
 
 }  // namespace
 
+NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator*
+    NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::instance_ =
+        nullptr;
+
 NativeWindowOcclusionTrackerWin*
 NativeWindowOcclusionTrackerWin::GetOrCreateInstance() {
   if (!g_tracker)
@@ -38,6 +45,11 @@ NativeWindowOcclusionTrackerWin::GetOrCreateInstance() {
   return g_tracker;
 }
 
+void NativeWindowOcclusionTrackerWin::DeleteInstanceForTesting() {
+  delete g_tracker;
+  g_tracker = nullptr;
+}
+
 void NativeWindowOcclusionTrackerWin::Enable(Window* window) {
   DCHECK(window->IsRootWindow());
   if (window->HasObserver(this)) {
@@ -56,7 +68,8 @@ void NativeWindowOcclusionTrackerWin::Enable(Window* window) {
       FROM_HERE,
       base::BindOnce(
           &WindowOcclusionCalculator::EnableOcclusionTrackingForWindow,
-          base::Unretained(occlusion_calculator_.get()), root_window_hwnd));
+          base::Unretained(WindowOcclusionCalculator::GetInstance()),
+          root_window_hwnd));
 }
 
 void NativeWindowOcclusionTrackerWin::Disable(Window* window) {
@@ -70,7 +83,8 @@ void NativeWindowOcclusionTrackerWin::Disable(Window* window) {
       FROM_HERE,
       base::BindOnce(
           &WindowOcclusionCalculator::DisableOcclusionTrackingForWindow,
-          base::Unretained(occlusion_calculator_.get()), root_window_hwnd));
+          base::Unretained(WindowOcclusionCalculator::GetInstance()),
+          root_window_hwnd));
 }
 
 void NativeWindowOcclusionTrackerWin::OnWindowVisibilityChanged(Window* window,
@@ -83,18 +97,14 @@ void NativeWindowOcclusionTrackerWin::OnWindowVisibilityChanged(Window* window,
   update_occlusion_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&WindowOcclusionCalculator::HandleVisibilityChanged,
-                     base::Unretained(occlusion_calculator_.get()), visible));
+                     base::Unretained(WindowOcclusionCalculator::GetInstance()),
+                     visible));
 }
 
 void NativeWindowOcclusionTrackerWin::OnWindowDestroying(Window* window) {
   Disable(window);
 }
 
-NativeWindowOcclusionTrackerWin**
-NativeWindowOcclusionTrackerWin::GetInstanceForTesting() {
-  return &g_tracker;
-}
-
 NativeWindowOcclusionTrackerWin::NativeWindowOcclusionTrackerWin()
     :  // Use a COMSTATaskRunner so that registering and unregistering
        // event hooks will happen on the same thread, as required by Windows,
@@ -111,15 +121,24 @@ NativeWindowOcclusionTrackerWin::NativeWindowOcclusionTrackerWin()
       session_change_observer_(
           base::BindRepeating(&NativeWindowOcclusionTrackerWin::OnSessionChange,
                               base::Unretained(this))) {
-  occlusion_calculator_ = std::make_unique<WindowOcclusionCalculator>(
-      update_occlusion_task_runner_, base::SequencedTaskRunnerHandle::Get());
+  WindowOcclusionCalculator::CreateInstance(
+      update_occlusion_task_runner_, base::SequencedTaskRunnerHandle::Get(),
+      base::BindRepeating(
+          &NativeWindowOcclusionTrackerWin::UpdateOcclusionState,
+          weak_factory_.GetWeakPtr()));
 }
 
 NativeWindowOcclusionTrackerWin::~NativeWindowOcclusionTrackerWin() {
-  // This shouldn't be reached in production code, because if it is,
-  // |occlusion_calculator_| will be deleted on the ui thread, which is
-  // problematic if there tasks scheduled on the background thread.
-  // Tests are allowed to delete the instance after running all pending tasks.
+  // |occlusion_calculator_| must be deleted on its sequence because it needs
+  // to unregister event hooks on COMSTA thread.
+  // This code is intended to be used in tests and shouldn't be reached in
+  // production code because it blocks the main thread.
+  base::WaitableEvent done_event;
+  update_occlusion_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&WindowOcclusionCalculator::DeleteInstanceForTesting,
+                     &done_event));
+  done_event.Wait();
 }
 
 // static
@@ -246,8 +265,11 @@ void NativeWindowOcclusionTrackerWin::OnSessionChange(
 NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
     WindowOcclusionCalculator(
         scoped_refptr<base::SequencedTaskRunner> task_runner,
-        scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner)
-    : task_runner_(task_runner), ui_thread_task_runner_(ui_thread_task_runner) {
+        scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner,
+        UpdateOcclusionStateCallback update_occlusion_state_callback)
+    : task_runner_(task_runner),
+      ui_thread_task_runner_(ui_thread_task_runner),
+      update_occlusion_state_callback_(update_occlusion_state_callback) {
   if (base::win::GetVersion() >= base::win::Version::WIN10) {
     ::CoCreateInstance(__uuidof(VirtualDesktopManager), nullptr, CLSCTX_ALL,
                        IID_PPV_ARGS(&virtual_desktop_manager_));
@@ -257,7 +279,25 @@ NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
 
 NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
     ~WindowOcclusionCalculator() {
-  DCHECK(global_event_hooks_.empty());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  UnregisterEventHooks();
+}
+
+void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::CreateInstance(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner,
+    UpdateOcclusionStateCallback update_occlusion_state_callback) {
+  DCHECK(!instance_);
+  instance_ = new WindowOcclusionCalculator(task_runner, ui_thread_task_runner,
+                                            update_occlusion_state_callback);
+}
+
+void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
+    DeleteInstanceForTesting(base::WaitableEvent* done_event) {
+  DCHECK(instance_);
+  delete instance_;
+  instance_ = nullptr;
+  done_event->Signal();
 }
 
 void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
@@ -312,24 +352,28 @@ NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::EventHookCallback(
     LONG idChild,
     DWORD dwEventThread,
     DWORD dwmsEventTime) {
-  g_tracker->occlusion_calculator_->ProcessEventHookCallback(event, hwnd,
-                                                             idObject, idChild);
+  if (instance_)
+    instance_->ProcessEventHookCallback(event, hwnd, idObject, idChild);
 }
 
 // static
 BOOL CALLBACK NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
     ComputeNativeWindowOcclusionStatusCallback(HWND hwnd, LPARAM lParam) {
-  return g_tracker->occlusion_calculator_
-      ->ProcessComputeNativeWindowOcclusionStatusCallback(
-          hwnd, reinterpret_cast<base::flat_set<DWORD>*>(lParam));
+  if (instance_) {
+    return instance_->ProcessComputeNativeWindowOcclusionStatusCallback(
+        hwnd, reinterpret_cast<base::flat_set<DWORD>*>(lParam));
+  }
+  return FALSE;
 }
 
 // static
 BOOL CALLBACK NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
     UpdateVisibleWindowProcessIdsCallback(HWND hwnd, LPARAM lParam) {
-  g_tracker->occlusion_calculator_
-      ->ProcessUpdateVisibleWindowProcessIdsCallback(hwnd);
-  return TRUE;
+  if (instance_) {
+    instance_->ProcessUpdateVisibleWindowProcessIdsCallback(hwnd);
+    return TRUE;
+  }
+  return FALSE;
 }
 
 void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
@@ -416,10 +460,8 @@ void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
   // Post a task to the browser ui thread to update the window occlusion state
   // on the root windows.
   ui_thread_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&NativeWindowOcclusionTrackerWin::UpdateOcclusionState,
-                     base::Unretained(g_tracker),
-                     root_window_hwnds_occlusion_state_));
+      FROM_HERE, base::BindOnce(update_occlusion_state_callback_,
+                                root_window_hwnds_occlusion_state_));
 }
 
 void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
@@ -598,7 +640,7 @@ void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::
       FROM_HERE,
       base::BindOnce(
           &WindowOcclusionCalculator::ScheduleOcclusionCalculationIfNeeded,
-          base::Unretained(this)));
+          weak_factory_.GetWeakPtr()));
 }
 
 void NativeWindowOcclusionTrackerWin::WindowOcclusionCalculator::

ui/aura/native_window_occlusion_tracker_win.h

@@ -13,8 +13,10 @@
 #include <memory>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
@@ -23,13 +25,16 @@
 #include "ui/aura/window_observer.h"
 #include "ui/base/win/session_change_observer.h"
 
-namespace aura {
+namespace base {
+class WaitableEvent;
+}
 
-// Required to declare a friend class.
-namespace test {
-class AuraTestHelper;
+namespace gfx {
+class Rect;
 }
 
+namespace aura {
+
 // This class keeps track of whether any HWNDs are occluding any app windows.
 // It notifies the host of any app window whose occlusion state changes. Most
 // code should not need to use this; it's an implementation detail.
@@ -37,6 +42,8 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
  public:
   static NativeWindowOcclusionTrackerWin* GetOrCreateInstance();
 
+  static void DeleteInstanceForTesting();
+
   // Enables notifying the host of |window| via SetNativeWindowOcclusionState()
   // when the occlusion state has been computed.
   void Enable(Window* window);
@@ -53,20 +60,27 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
 
  private:
   friend class NativeWindowOcclusionTrackerTest;
-  friend class test::AuraTestHelper;
-
-  // Returns a pointer to global instance.
-  static NativeWindowOcclusionTrackerWin** GetInstanceForTesting();
 
   // This class computes the occlusion state of the tracked windows.
   // It runs on a separate thread, and notifies the main thread of
   // the occlusion state of the tracked windows.
-  class AURA_EXPORT WindowOcclusionCalculator {
+  class WindowOcclusionCalculator {
    public:
-    WindowOcclusionCalculator(
+    using UpdateOcclusionStateCallback = base::RepeatingCallback<void(
+        const base::flat_map<HWND, Window::OcclusionState>&)>;
+
+    // Creates WindowOcclusionCalculator instance. Must be called on UI thread.
+    static void CreateInstance(
         scoped_refptr<base::SequencedTaskRunner> task_runner,
-        scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner);
-    ~WindowOcclusionCalculator();
+        scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner,
+        UpdateOcclusionStateCallback update_occlusion_state_callback);
+
+    // Returns existing WindowOcclusionCalculator instance.
+    static WindowOcclusionCalculator* GetInstance() { return instance_; }
+
+    // Deletes |instance_| and signals |done_event|. Must be called on COMSTA
+    // thread.
+    static void DeleteInstanceForTesting(base::WaitableEvent* done_event);
 
     void EnableOcclusionTrackingForWindow(HWND hwnd);
     void DisableOcclusionTrackingForWindow(HWND hwnd);
@@ -75,8 +89,11 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
     void HandleVisibilityChanged(bool visible);
 
    private:
-    friend class NativeWindowOcclusionTrackerTest;
-    friend class test::AuraTestHelper;
+    WindowOcclusionCalculator(
+        scoped_refptr<base::SequencedTaskRunner> task_runner,
+        scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner,
+        UpdateOcclusionStateCallback update_occlusion_state_callback);
+    ~WindowOcclusionCalculator();
 
     // Registers event hooks, if not registered.
     void MaybeRegisterEventHooks();
@@ -164,6 +181,8 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
     // if we we can't tell for sure.
     base::Optional<bool> IsWindowOnCurrentVirtualDesktop(HWND hwnd);
 
+    static WindowOcclusionCalculator* instance_;
+
     // Task runner for our thread.
     scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
@@ -171,6 +190,9 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
     // task is posted to this task runner.
     const scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner_;
 
+    // Callback used to update occlusion state on UI thread.
+    UpdateOcclusionStateCallback update_occlusion_state_callback_;
+
     // Map of root app window hwnds and their occlusion state. This contains
     // both visible and hidden windows.
     base::flat_map<HWND, Window::OcclusionState>
@@ -214,6 +236,8 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
 
     SEQUENCE_CHECKER(sequence_checker_);
 
+    base::WeakPtrFactory<WindowOcclusionCalculator> weak_factory_{this};
+
     DISALLOW_COPY_AND_ASSIGN(WindowOcclusionCalculator);
   };
 
@@ -246,14 +270,14 @@ class AURA_EXPORT NativeWindowOcclusionTrackerWin : public WindowObserver {
   // This is set by UpdateOcclusionState. It is currently only used by tests.
   int num_visible_root_windows_ = 0;
 
-  std::unique_ptr<WindowOcclusionCalculator> occlusion_calculator_;
-
   // Manages observation of Windows Session Change messages.
   ui::SessionChangeObserver session_change_observer_;
 
   // If the screen is locked, windows are considered occluded.
   bool screen_locked_ = false;
 
+  base::WeakPtrFactory<NativeWindowOcclusionTrackerWin> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(NativeWindowOcclusionTrackerWin);
 };
 

ui/aura/test/aura_test_helper.cc

@@ -34,8 +34,6 @@
 
 #if defined(OS_WIN)
 #include "base/sequenced_task_runner.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/test/bind_test_util.h"
 #include "ui/aura/native_window_occlusion_tracker_win.h"
 #endif
 
@@ -168,7 +166,7 @@ void AuraTestHelper::TearDown() {
   // task runner. Since ThreadPool is destroyed together with TaskEnvironment,
   // NativeWindowOcclusionTrackerWin instance must be deleted as well and
   // recreated on demand in other test.
-  DeleteNativeWindowOcclusionTrackerWin();
+  NativeWindowOcclusionTrackerWin::DeleteInstanceForTesting();
 #endif
 }
 
@@ -187,32 +185,5 @@ Env* AuraTestHelper::GetEnv() {
   return env_ ? env_.get() : Env::HasInstance() ? Env::GetInstance() : nullptr;
 }
 
-#if defined(OS_WIN)
-void AuraTestHelper::DeleteNativeWindowOcclusionTrackerWin() {
-  NativeWindowOcclusionTrackerWin** global_ptr =
-      NativeWindowOcclusionTrackerWin::GetInstanceForTesting();
-  if (NativeWindowOcclusionTrackerWin* tracker = *global_ptr) {
-    // WindowOcclusionCalculator must be deleted on its sequence. Wait until
-    // it's deleted and then delete the tracker.
-    base::WaitableEvent waitable_event;
-    DCHECK(
-        !tracker->update_occlusion_task_runner_->RunsTasksInCurrentSequence());
-    tracker->update_occlusion_task_runner_->PostTask(
-        FROM_HERE, base::BindLambdaForTesting([tracker, &waitable_event]() {
-          if (tracker->occlusion_calculator_) {
-            tracker->occlusion_calculator_->root_window_hwnds_occlusion_state_
-                .clear();
-            tracker->occlusion_calculator_->UnregisterEventHooks();
-            tracker->occlusion_calculator_.reset();
-          }
-          waitable_event.Signal();
-        }));
-    waitable_event.Wait();
-    delete tracker;
-    *global_ptr = nullptr;
-  }
-}
-#endif  // defined(OS_WIN)
-
 }  // namespace test
 }  // namespace aura

ui/aura/test/aura_test_helper.h

@@ -70,11 +70,6 @@ class AuraTestHelper {
   Env* GetEnv();
 
  private:
-#if defined(OS_WIN)
-  // Deletes existing NativeWindowOcclusionTrackerWin instance.
-  void DeleteNativeWindowOcclusionTrackerWin();
-#endif  // defined(OS_WIN)
-
   bool setup_called_ = false;
   bool teardown_called_ = false;
   ui::ContextFactory* context_factory_to_restore_ = nullptr;