From b1a95e642207618fbc409a73ab5ad0d8b4ce42ac Mon Sep 17 00:00:00 2001
From: Mitsuru Oshima <[email protected]>
Date: Tue, 21 Jul 2026 23:08:37 -0700
Subject: [PATCH] Block the deletion of new/old_parent, and receiver window

during the window hierachy change notifications.

Fixed: 516427761
Test: covered by unittest
Change-Id: I72aaf6c940ed8883eecaa3efac3db3fe0d20d333
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8127340
Commit-Queue: Mitsuru Oshima <[email protected]>
Reviewed-by: Achuith Bhandarkar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1666030}
---
diff --git a/ui/aura/window.cc b/ui/aura/window.cc
index fa0951a..80574027 100644
--- a/ui/aura/window.cc
+++ b/ui/aura/window.cc
@@ -1501,7 +1501,12 @@
 
 void Window::NotifyWindowHierarchyChange(
     const WindowObserver::HierarchyChangeParams& params) {
-  ScopedDeleteBlocker blocker(this);
+
+  // Block deletion of old_parent and new_parent across all target
+  // sub-tree callbacks to ensure neither parent is destroyed before
+  // NotifyWindowHierarchyChangeUp() runs.
+  ScopedDeleteBlocker old_blocker(params.old_parent);
+  ScopedDeleteBlocker new_blocker(params.new_parent);
 
   params.target->NotifyWindowHierarchyChangeDown(params);
   switch (params.phase) {
@@ -1533,6 +1538,8 @@
 
 void Window::NotifyWindowHierarchyChangeAtReceiver(
     const WindowObserver::HierarchyChangeParams& params) {
+  ScopedDeleteBlocker blocker(this);
+
   WindowObserver::HierarchyChangeParams local_params = params;
   local_params.receiver = this;
 
diff --git a/ui/aura/window.h b/ui/aura/window.h
index ae8efc79..f743126 100644
--- a/ui/aura/window.h
+++ b/ui/aura/window.h
@@ -127,15 +127,22 @@
   // A helper class to ensure that the Window is not deleted while it is
   // notifying observers or doing other operations where re-entrant deletion
   // would be problematic. Attempting to delete the Window while a
-  // ScopedDeleteBlocker is active will cause a crash.
+  // ScopedDeleteBlocker is active will cause a crash. This is no-op if
+  // if nullptr is passed.
   class ScopedDeleteBlocker {
    public:
     explicit ScopedDeleteBlocker(aura::Window* window) : window_(window) {
-      window_->delete_block_count_++;
+      if (window_) {
+        window_->delete_block_count_++;
+      }
     }
     ScopedDeleteBlocker(const ScopedDeleteBlocker&) = delete;
     ScopedDeleteBlocker& operator=(const ScopedDeleteBlocker&) = delete;
-    ~ScopedDeleteBlocker() { window_->delete_block_count_--; }
+    ~ScopedDeleteBlocker() {
+      if (window_) {
+        window_->delete_block_count_--;
+      }
+    }
     raw_ptr<aura::Window> window_;
   };
 
diff --git a/ui/aura/window_unittest.cc b/ui/aura/window_unittest.cc
index 1324b2d..4c4538b 100644
--- a/ui/aura/window_unittest.cc
+++ b/ui/aura/window_unittest.cc
@@ -3595,6 +3595,53 @@
   EXPECT_TRUE(weak_window);
 }
 
+// WindowObserver implementation that deletes the observed window in
+// OnWindowHierarchyChanging().
+class DeleteOnHierarchyChangingObserver : public WindowObserver {
+ public:
+  explicit DeleteOnHierarchyChangingObserver(Window* window) : window_(window) {
+    window_->AddObserver(this);
+  }
+
+  DeleteOnHierarchyChangingObserver(const DeleteOnHierarchyChangingObserver&) =
+      delete;
+  DeleteOnHierarchyChangingObserver& operator=(
+      const DeleteOnHierarchyChangingObserver&) = delete;
+
+  ~DeleteOnHierarchyChangingObserver() override {
+    CHECK(window_);
+    window_->RemoveObserver(this);
+  }
+
+  // WindowObserver:
+  void OnWindowHierarchyChanging(const HierarchyChangeParams& params) override {
+    Window* window = window_;
+    window_ = nullptr;
+    window->RemoveObserver(this);
+    // This will fail with CHECK.
+    delete window;
+  }
+
+ private:
+  raw_ptr<Window> window_;
+};
+
+TEST_F(WindowDeathTest, DeleteReceiverInOnWindowHierarchyChanging) {
+  std::unique_ptr<Window> parent = std::make_unique<Window>(nullptr);
+  parent->Init(ui::LAYER_NOT_DRAWN);
+  std::unique_ptr<Window> child = std::make_unique<Window>(nullptr);
+  child->Init(ui::LAYER_NOT_DRAWN);
+  Window* grandchild = new Window(nullptr);
+  grandchild->Init(ui::LAYER_NOT_DRAWN);
+  child->AddChild(grandchild);
+
+  // |grandchild| is notified as a receiver while recursing down through the
+  // subtree rooted at |child|; deleting it from within the notification must
+  // not be allowed.
+  DeleteOnHierarchyChangingObserver observer(grandchild);
+  EXPECT_DEATH(parent->AddChild(child.get()), "");
+}
+
 // WindowObserver implementation that deletes a window in
 // OnWindowVisibilityChanged().
 class DeleteOnVisibilityChangedObserver : public WindowObserver {

diff --git a/ui/aura/window_unittest.cc b/ui/aura/window_unittest.cc
index 1324b2d..4c4538b 100644
--- a/ui/aura/window_unittest.cc
+++ b/ui/aura/window_unittest.cc
@@ -3595,6 +3595,53 @@
   EXPECT_TRUE(weak_window);
 }
 
+// WindowObserver implementation that deletes the observed window in
+// OnWindowHierarchyChanging().
+class DeleteOnHierarchyChangingObserver : public WindowObserver {
+ public:
+  explicit DeleteOnHierarchyChangingObserver(Window* window) : window_(window) {
+    window_->AddObserver(this);
+  }
+
+  DeleteOnHierarchyChangingObserver(const DeleteOnHierarchyChangingObserver&) =
+      delete;
+  DeleteOnHierarchyChangingObserver& operator=(
+      const DeleteOnHierarchyChangingObserver&) = delete;
+
+  ~DeleteOnHierarchyChangingObserver() override {
+    CHECK(window_);
+    window_->RemoveObserver(this);
+  }
+
+  // WindowObserver:
+  void OnWindowHierarchyChanging(const HierarchyChangeParams& params) override {
+    Window* window = window_;
+    window_ = nullptr;
+    window->RemoveObserver(this);
+    // This will fail with CHECK.
+    delete window;
+  }
+
+ private:
+  raw_ptr<Window> window_;
+};
+
+TEST_F(WindowDeathTest, DeleteReceiverInOnWindowHierarchyChanging) {
+  std::unique_ptr<Window> parent = std::make_unique<Window>(nullptr);
+  parent->Init(ui::LAYER_NOT_DRAWN);
+  std::unique_ptr<Window> child = std::make_unique<Window>(nullptr);
+  child->Init(ui::LAYER_NOT_DRAWN);
+  Window* grandchild = new Window(nullptr);
+  grandchild->Init(ui::LAYER_NOT_DRAWN);
+  child->AddChild(grandchild);
+
+  // |grandchild| is notified as a receiver while recursing down through the
+  // subtree rooted at |child|; deleting it from within the notification must
+  // not be allowed.
+  DeleteOnHierarchyChangingObserver observer(grandchild);
+  EXPECT_DEATH(parent->AddChild(child.get()), "");
+}
+
 // WindowObserver implementation that deletes a window in
 // OnWindowVisibilityChanged().
 class DeleteOnVisibilityChangedObserver : public WindowObserver {