Fix a crash when a window is deleted while gaining or losing activation

An observer of OnWindowActivating or OnWindowActivated may destroy
the window losing, or gaining activation.
The FocusController should handle these cases, to avoid crashes, and/or
returning dangling pointers when querying the active window.

BUG=1048870
TEST=Expanded an existing test, which crashes without the fix.

Change-Id: I804210e2f3250f69fc02b9a93eaee5d0236fca5a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2038131
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739288}

ui/aura/window.cc

@@ -117,6 +117,7 @@ Window::Window(WindowDelegate* delegate, client::WindowType type)
 }
 
 Window::~Window() {
+  is_destroying_ = true;
   WindowOcclusionTracker::ScopedPause pause_occlusion_tracking;
 
   if (layer()->owner() == this)

ui/aura/window.h

@@ -152,6 +152,7 @@ class AURA_EXPORT Window : public ui::LayerDelegate,
   // Initializes the window. This creates the window's layer.
   void Init(ui::LayerType layer_type);
 
+  bool is_destroying() const { return is_destroying_; }
   void set_owned_by_parent(bool owned_by_parent) {
     owned_by_parent_ = owned_by_parent;
   }
@@ -635,6 +636,9 @@ class AURA_EXPORT Window : public ui::LayerDelegate,
 
   client::WindowType type_;
 
+  // True if this window is being destroyed.
+  bool is_destroying_ = false;
+
   // True if the Window is owned by its parent - i.e. it will be deleted by its
   // parent during its parents destruction.
   bool owned_by_parent_ = true;

ui/wm/core/base_focus_rules.cc

@@ -52,6 +52,10 @@ bool BaseFocusRules::CanActivateWindow(const aura::Window* window) const {
   if (!window)
     return true;
 
+  // A window that is being destroyed should not be activatable.
+  if (window->is_destroying())
+    return false;
+
   // Only toplevel windows can be activated.
   if (!IsToplevelWindow(window))
     return false;

ui/wm/core/focus_controller.cc

@@ -152,6 +152,11 @@ void FocusController::OnWindowDestroying(aura::Window* window) {
   // destruction.
   window->ClearProperty(aura::client::kModalKey);
   WindowLostFocusFromDispositionChange(window, window->parent());
+
+  // We may have already stopped observing |window| if `SetActiveWindow()` was
+  // called inside `WindowLostFocusFromDispositionChange()`.
+  if (observer_manager_.IsObserving(window))
+    observer_manager_.Remove(window);
 }
 
 void FocusController::OnWindowHierarchyChanging(
@@ -208,7 +213,10 @@ void FocusController::FocusAndActivateWindow(
       focusable_window_tracker.Add(focusable);
       focusable = nullptr;
     }
-    SetActiveWindow(reason, window, activatable);
+
+    if (!SetActiveWindow(reason, window, activatable))
+      return;
+
     if (!focusable_window_tracker.windows().empty())
       focusable = focusable_window_tracker.Pop();
   } else {
@@ -278,19 +286,29 @@ void FocusController::SetFocusedWindow(aura::Window* window) {
   }
 }
 
-void FocusController::SetActiveWindow(
+// Defines a macro that is meant to be called from SetActiveWindow(), which
+// checks whether the activation was interrupted by checking whether
+// |pending_activation_| has a value or not. In this case, it early-outs from
+// the SetActiveWindow() stack.
+// clang-format off
+#define MAYBE_ACTIVATION_INTERRUPTED() \
+  if (!pending_activation_)            \
+    return false
+// clang-format on
+
+bool FocusController::SetActiveWindow(
     ActivationChangeObserver::ActivationReason reason,
     aura::Window* requested_window,
     aura::Window* window) {
   if (pending_activation_)
-    return;
+    return false;
 
   if (window == active_window_) {
     if (requested_window) {
       for (auto& observer : activation_observers_)
         observer.OnAttemptToReactivateWindow(requested_window, active_window_);
     }
-    return;
+    return true;
   }
 
   DCHECK(rules_->CanActivateWindow(window));
@@ -306,36 +324,56 @@ void FocusController::SetActiveWindow(
   if (lost_activation)
     window_tracker.Add(lost_activation);
 
-  for (auto& observer : activation_observers_)
+  // Start observing the window gaining activation at this point since it maybe
+  // destroyed at an early stage, e.g. the activating phase.
+  if (window && !observer_manager_.IsObserving(window))
+    observer_manager_.Add(window);
+
+  for (auto& observer : activation_observers_) {
     observer.OnWindowActivating(reason, window, active_window_);
 
+    MAYBE_ACTIVATION_INTERRUPTED();
+  }
+
   if (active_window_ && observer_manager_.IsObserving(active_window_) &&
       focused_window_ != active_window_) {
     observer_manager_.Remove(active_window_);
   }
+
   active_window_ = window;
-  if (active_window_ && !observer_manager_.IsObserving(active_window_))
-    observer_manager_.Add(active_window_);
+
   if (active_window_)
     StackActiveWindow();
 
+  MAYBE_ACTIVATION_INTERRUPTED();
+
   ActivationChangeObserver* observer = nullptr;
   if (window_tracker.Contains(lost_activation)) {
     observer = GetActivationChangeObserver(lost_activation);
     if (observer)
       observer->OnWindowActivated(reason, active_window_, lost_activation);
   }
+
+  MAYBE_ACTIVATION_INTERRUPTED();
+
   observer = GetActivationChangeObserver(active_window_);
   if (observer) {
     observer->OnWindowActivated(
         reason, active_window_,
         window_tracker.Contains(lost_activation) ? lost_activation : nullptr);
   }
+
+  MAYBE_ACTIVATION_INTERRUPTED();
+
   for (auto& observer : activation_observers_) {
     observer.OnWindowActivated(
         reason, active_window_,
         window_tracker.Contains(lost_activation) ? lost_activation : nullptr);
+
+    MAYBE_ACTIVATION_INTERRUPTED();
   }
+
+  return true;
 }
 
 void FocusController::StackActiveWindow() {
@@ -353,13 +391,43 @@ void FocusController::WindowLostFocusFromDispositionChange(
   // Activation adjustments are handled first in the event of a disposition
   // changed. If an activation change is necessary, focus is reset as part of
   // that process so there's no point in updating focus independently.
-  if (window == active_window_) {
+  const bool is_active_window_losing_focus = window == active_window_;
+  const bool is_pending_window_losing_focus =
+      pending_activation_ && (window == pending_activation_.value());
+  if (is_active_window_losing_focus || is_pending_window_losing_focus) {
+    if (pending_activation_) {
+      // We're in the middle of an on-going activation. We need to determine
+      // whether we need to abort this activation. This happens when the window
+      // gaining activation is destroyed at any point of the activation process.
+      if (is_pending_window_losing_focus) {
+        // Abort this on-going activation. The below call to SetActiveWindow()
+        // will attempt activating the next activatable window.
+        pending_activation_.reset();
+      } else if (is_active_window_losing_focus) {
+        // The window losing activation may have been destroyed before the
+        // window gaining active is set as the active window. We need to clear
+        // the active and focused windows temporarily, since querying the active
+        // window now should not return a dangling pointer.
+        active_window_ = nullptr;
+        SetFocusedWindow(nullptr);
+
+        // We should continue the on-going activation and leave
+        // |pending_activation_| unchanged.
+        return;
+      }
+    }
+
     aura::Window* next_activatable = rules_->GetNextActivatableWindow(window);
-    SetActiveWindow(
-        ActivationChangeObserver::ActivationReason::WINDOW_DISPOSITION_CHANGED,
-        nullptr, next_activatable);
-    if (!(active_window_ && active_window_->Contains(focused_window_)))
+    if (!SetActiveWindow(ActivationChangeObserver::ActivationReason::
+                             WINDOW_DISPOSITION_CHANGED,
+                         nullptr, next_activatable)) {
+      return;
+    }
+
+    if (window == focused_window_ || !active_window_ ||
+        !active_window_->Contains(focused_window_)) {
       SetFocusedWindow(next_activatable);
+    }
   } else if (window->Contains(focused_window_)) {
     if (pending_activation_) {
       // We're in the process of updating activation, most likely

ui/wm/core/focus_controller.h

@@ -98,7 +98,10 @@ class WM_CORE_EXPORT FocusController : public ActivationClient,
   // request (e.g. FocusWindow or ActivateWindow). It may be NULL, e.g. if
   // SetActiveWindow was not called by an external request. |activatable_window|
   // refers to the actual window to be activated, which may be different.
-  void SetActiveWindow(ActivationChangeObserver::ActivationReason reason,
+  // Returns true if activation should proceed, or false if activation was
+  // interrupted, e.g. by the destruction of the window gaining activation
+  // during the process, and therefore activation should be aborted.
+  bool SetActiveWindow(ActivationChangeObserver::ActivationReason reason,
                        aura::Window* requested_window,
                        aura::Window* activatable_window);
 

ui/wm/core/focus_controller_unittest.cc

@@ -130,6 +130,20 @@ class RecordingActivationAndFocusChangeObserver
   }
 
   // Overridden from ActivationChangeObserver:
+  void OnWindowActivating(ActivationReason reason,
+                          aura::Window* gaining_active,
+                          aura::Window* losing_active) override {
+    if (deleter_->GetDeletedWindow()) {
+      // A deleted window during activation should never be return as either the
+      // gaining or losing active windows, nor should it be returned as the
+      // currently active one.
+      auto* active_window = GetActivationClient(root_)->GetActiveWindow();
+      EXPECT_NE(active_window, deleter_->GetDeletedWindow());
+      EXPECT_NE(gaining_active, deleter_->GetDeletedWindow());
+      EXPECT_NE(losing_active, deleter_->GetDeletedWindow());
+    }
+  }
+
   void OnWindowActivated(ActivationReason reason,
                          aura::Window* gained_active,
                          aura::Window* lost_active) override {
@@ -192,25 +206,54 @@ class HideOnLoseActivationChangeObserver : public ActivationChangeObserver {
 };
 
 // ActivationChangeObserver that deletes the window losing activation.
-class DeleteOnLoseActivationChangeObserver : public ActivationChangeObserver,
-                                             public WindowDeleter {
+class DeleteOnActivationChangeObserver : public ActivationChangeObserver,
+                                         public WindowDeleter {
  public:
-  explicit DeleteOnLoseActivationChangeObserver(aura::Window* window)
+  // If |delete_on_activating| is true, |window| will be deleted when
+  // OnWindowActivating() is called, otherwise, it will be deleted when
+  // OnWindowActivated() is called.
+  // If |delete_window_losing_active| is true, |window| will be deleted if it is
+  // the window losing activation, otherwise, will be deleted if it is the one
+  // gaining activation.
+  DeleteOnActivationChangeObserver(aura::Window* window,
+                                   bool delete_on_activating,
+                                   bool delete_window_losing_active)
       : root_(window->GetRootWindow()),
         window_(window),
+        delete_on_activating_(delete_on_activating),
+        delete_window_losing_active_(delete_window_losing_active),
         did_delete_(false) {
     GetActivationClient(root_)->AddObserver(this);
   }
-  ~DeleteOnLoseActivationChangeObserver() override {
+  ~DeleteOnActivationChangeObserver() override {
     GetActivationClient(root_)->RemoveObserver(this);
   }
 
   // Overridden from ActivationChangeObserver:
+  void OnWindowActivating(ActivationReason reason,
+                          aura::Window* gaining_active,
+                          aura::Window* losing_active) override {
+    if (!delete_on_activating_)
+      return;
+
+    auto* window_to_delete =
+        delete_window_losing_active_ ? losing_active : gaining_active;
+    if (window_ && window_to_delete == window_) {
+      delete window_to_delete;
+      did_delete_ = true;
+    }
+  }
+
   void OnWindowActivated(ActivationReason reason,
                          aura::Window* gained_active,
                          aura::Window* lost_active) override {
-    if (window_ && lost_active == window_) {
-      delete lost_active;
+    if (delete_on_activating_)
+      return;
+
+    auto* window_to_delete =
+        delete_window_losing_active_ ? lost_active : gained_active;
+    if (window_ && window_to_delete == window_) {
+      delete window_to_delete;
       did_delete_ = true;
     }
   }
@@ -223,9 +266,11 @@ class DeleteOnLoseActivationChangeObserver : public ActivationChangeObserver,
  private:
   aura::Window* root_;
   aura::Window* window_;
+  const bool delete_on_activating_;
+  const bool delete_window_losing_active_;
   bool did_delete_;
 
-  DISALLOW_COPY_AND_ASSIGN(DeleteOnLoseActivationChangeObserver);
+  DISALLOW_COPY_AND_ASSIGN(DeleteOnActivationChangeObserver);
 };
 
 // FocusChangeObserver that deletes the window losing focus.
@@ -895,7 +940,10 @@ class FocusControllerDirectTestBase : public FocusControllerTestBase {
 
     {
       aura::Window* to_delete = root_window()->GetChildById(1);
-      DeleteOnLoseActivationChangeObserver observer1(to_delete);
+      DeleteOnActivationChangeObserver observer1(
+          to_delete,
+          /*delete_on_activating=*/true,
+          /*delete_window_losing_active=*/true);
       RecordingActivationAndFocusChangeObserver observer2(root_window(),
                                                           &observer1);
 
@@ -910,7 +958,9 @@ class FocusControllerDirectTestBase : public FocusControllerTestBase {
 
     {
       aura::Window* to_delete = root_window()->GetChildById(2);
-      DeleteOnLoseFocusChangeObserver observer1(to_delete);
+      DeleteOnActivationChangeObserver observer1(
+          to_delete, /*delete_on_activating=*/false,
+          /*delete_window_losing_active=*/true);
       RecordingActivationAndFocusChangeObserver observer2(root_window(),
                                                           &observer1);
 
@@ -922,6 +972,68 @@ class FocusControllerDirectTestBase : public FocusControllerTestBase {
       EXPECT_EQ(to_delete, observer1.GetDeletedWindow());
       EXPECT_FALSE(observer2.was_notified_with_deleted_window());
     }
+
+    {
+      aura::test::CreateTestWindowWithDelegate(
+          aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate(), 4,
+          gfx::Rect(125, 125, 50, 50), root_window());
+
+      EXPECT_EQ(3, GetActiveWindowId());
+      EXPECT_EQ(3, GetFocusedWindowId());
+
+      aura::Window* to_delete = root_window()->GetChildById(3);
+      DeleteOnLoseFocusChangeObserver observer1(to_delete);
+      RecordingActivationAndFocusChangeObserver observer2(root_window(),
+                                                          &observer1);
+
+      FocusWindowById(4);
+
+      EXPECT_EQ(4, GetActiveWindowId());
+      EXPECT_EQ(4, GetFocusedWindowId());
+
+      EXPECT_EQ(to_delete, observer1.GetDeletedWindow());
+      EXPECT_FALSE(observer2.was_notified_with_deleted_window());
+    }
+
+    {
+      aura::test::CreateTestWindowWithDelegate(
+          aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate(), 5,
+          gfx::Rect(125, 125, 50, 50), root_window());
+      aura::test::CreateTestWindowWithDelegate(
+          aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate(), 6,
+          gfx::Rect(125, 125, 50, 50), root_window());
+
+      EXPECT_EQ(4, GetActiveWindowId());
+      EXPECT_EQ(4, GetFocusedWindowId());
+
+      // Delete the window that is gaining activation at both the "activating"
+      // and "activated" phases. Make sure the activations were interrupted
+      // properly and the correct next activatable window is activated.
+      aura::Window* to_delete1 = root_window()->GetChildById(5);
+      DeleteOnActivationChangeObserver observer1(
+          to_delete1, /*delete_on_activating=*/false,
+          /*delete_window_losing_active=*/false);
+      RecordingActivationAndFocusChangeObserver observer2(root_window(),
+                                                          &observer1);
+      // Test a recursive scenario by having another observer that would delete
+      // the next activatable window during the "activating" phase.
+      aura::Window* to_delete2 = root_window()->GetChildById(6);
+      DeleteOnActivationChangeObserver observer3(
+          to_delete2, /*delete_on_activating=*/true,
+          /*delete_window_losing_active=*/false);
+      RecordingActivationAndFocusChangeObserver observer4(root_window(),
+                                                          &observer3);
+
+      FocusWindowById(5);
+
+      EXPECT_EQ(4, GetActiveWindowId());
+      EXPECT_EQ(4, GetFocusedWindowId());
+
+      EXPECT_EQ(to_delete1, observer1.GetDeletedWindow());
+      EXPECT_FALSE(observer2.was_notified_with_deleted_window());
+      EXPECT_EQ(to_delete2, observer3.GetDeletedWindow());
+      EXPECT_FALSE(observer4.was_notified_with_deleted_window());
+    }
   }
 
   // Test that the activation of the current active window will bring the window