From 7b770ddad7074523d56004f59a9f3ec4592d2873 Mon Sep 17 00:00:00 2001
From: Aman Verma <amanvr@google.com>
Date: Wed, 16 Sep 2026 12:17:33 -0700
Subject: [PATCH] ui: Fix ViewAndroid child iteration UAF on reentrancy

Event handlers run during `ViewAndroid::children_` traversal can
synchronously remove or reorder children, invalidating the active list
iterator and crashing in `HitTest`.

Introduce `ScopedChildrenIter` to iterate `children_` safely:
* Active iterators register in `active_iters_` (`base::LinkedList`).
* Mutating methods call `StepIteratorsOver()` before `erase`/`splice`
  to advance any cursor sitting on the target node.
* `~ViewAndroid` detaches live iterators so `GetNext()` returns null.
* `RemoveAllChildren()` unlinks and clears `parent_` before calling
  `OnDetachedFromWindow()`, preserving reparenting from callbacks.

Bug: 553995407, 535749304
Change-Id: I07a5a8b3271620f999ea1b5428095776bf398e62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8409091
Commit-Queue: Wenyu Fu <wenyufu@chromium.org>
Auto-Submit: Aman Verma <amanvr@google.com>
Reviewed-by: Wenyu Fu <wenyufu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1699163}
---
diff --git a/ui/android/view_android.cc b/ui/android/view_android.cc
index 6352989..aefe26a 100644
--- a/ui/android/view_android.cc
+++ b/ui/android/view_android.cc
@@ -119,6 +119,9 @@
 ViewAndroid::ViewAndroid() : ViewAndroid(LayoutType::kNormal) {}
 
 ViewAndroid::~ViewAndroid() {
+  while (!active_iters_.empty()) {
+    active_iters_.head()->value()->ViewDestroyed();
+  }
   RemoveAllChildren(GetWindowAndroid() != nullptr);
   observer_list_.Notify(&ViewAndroidObserver::OnViewAndroidDestroyed);
   observer_list_.Clear();
@@ -203,7 +206,7 @@
   if (view->has_event_forwarder())
     return true;
 
-  for (ViewAndroid* child : view->children_) {
+  for (ScopedChildrenIter it(view); ViewAndroid* child = it.GetNext();) {
     if (SubtreeHasEventForwarder(child))
       return true;
   }
@@ -216,8 +219,10 @@
   CHECK(it != children_.end());
 
   // Top element is placed at the end of the list.
-  if (*it != children_.back())
+  if (*it != children_.back()) {
+    StepIteratorsOver(it);
     children_.splice(children_.end(), children_, it);
+  }
 }
 
 void ViewAndroid::MoveToBack(ViewAndroid* child) {
@@ -226,8 +231,10 @@
   CHECK(it != children_.end());
 
   // Bottom element is placed at the beginning of the list.
-  if (*it != children_.front())
+  if (*it != children_.front()) {
+    StepIteratorsOver(it);
     children_.splice(children_.begin(), children_, it);
+  }
 }
 
 void ViewAndroid::RemoveFromParent() {
@@ -301,13 +308,21 @@
 }
 
 void ViewAndroid::RemoveAllChildren(bool attached_to_window) {
-  auto it = children_.begin();
-  while (it != children_.end()) {
-    if (attached_to_window)
-      (*it)->OnDetachedFromWindow();
-    (*it)->parent_ = nullptr;
-    // erase returns a new iterator for the element following the ereased one.
-    it = children_.erase(it);
+  while (!children_.empty()) {
+    ViewAndroid* child = children_.front();
+    if (attached_to_window) {
+      child->OnDetachedFromWindow();
+    }
+    // If the callback already unlinked or reparented child, leave parent_
+    // alone.
+    if (child->parent_ == this) {
+      child->parent_ = nullptr;
+      auto it = std::ranges::find(children_, child);
+      if (it != children_.end()) {
+        StepIteratorsOver(it);
+        children_.erase(it);
+      }
+    }
   }
 }
 
@@ -320,6 +335,8 @@
   std::list<raw_ptr<ViewAndroid, CtnExperimental>>::iterator it =
       std::ranges::find(children_, child);
   CHECK(it != children_.end());
+
+  StepIteratorsOver(it);
   children_.erase(it);
   child->parent_ = nullptr;
 }
@@ -398,14 +415,14 @@
 
 void ViewAndroid::OnAttachedToWindow() {
   observer_list_.Notify(&ViewAndroidObserver::OnAttachedToWindow);
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->OnAttachedToWindow();
   }
 }
 
 void ViewAndroid::OnDetachedFromWindow() {
   observer_list_.Notify(&ViewAndroidObserver::OnDetachedFromWindow);
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->OnDetachedFromWindow();
   }
 }
@@ -534,7 +551,7 @@
 void ViewAndroid::OnBrowserControlsHeightChanged() {
   if (event_handler_)
     event_handler_->OnBrowserControlsHeightChanged();
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->match_parent())
       child->OnBrowserControlsHeightChanged();
   }
@@ -577,7 +594,7 @@
   bounds_dips_.set_size(gfx::Size(std::ceil(size_device_px.width() / scale),
                                   std::ceil(size_device_px.height() / scale)));
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->match_parent())
       child->OnSizeChangedInternal(size_device_px);
   }
@@ -586,7 +603,7 @@
 void ViewAndroid::DispatchOnSizeChanged() {
   if (event_handler_)
     event_handler_->OnSizeChanged();
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->match_parent())
       child->DispatchOnSizeChanged();
   }
@@ -601,7 +618,7 @@
   if (event_handler_)
     event_handler_->OnPhysicalBackingSizeChanged(deadline_override);
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->OnPhysicalBackingSizeChanged(size, deadline_override);
   }
 }
@@ -613,7 +630,7 @@
   if (event_handler_)
     event_handler_->OnControlsResizeViewChanged();
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->OnControlsResizeViewChanged(controls_resize_view);
   }
 }
@@ -623,7 +640,7 @@
     event_handler_->OnWindowPositionChanged();
   }
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->DispatchWindowPositionChange();
   }
 }
@@ -701,7 +718,7 @@
   if (event_handler_ && event_handler_->OnGenericMotionEvent(event))
     return true;
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->OnGenericMotionEvent(event))
       return true;
   }
@@ -712,7 +729,7 @@
   if (event_handler_ && event_handler_->OnKeyUp(event))
     return true;
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->OnKeyUp(event))
       return true;
   }
@@ -723,7 +740,7 @@
   if (event_handler_ && event_handler_->DispatchKeyEvent(event))
     return true;
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->DispatchKeyEvent(event))
       return true;
   }
@@ -734,7 +751,7 @@
   if (event_handler_ && event_handler_->ScrollBy(delta_x, delta_y))
     return true;
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->ScrollBy(delta_x, delta_y))
       return true;
   }
@@ -745,7 +762,7 @@
   if (event_handler_ && event_handler_->ScrollTo(x, y))
     return true;
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     if (child->ScrollTo(x, y))
       return true;
   }
@@ -757,7 +774,7 @@
   if (event_handler_)
     event_handler_->NotifyVirtualKeyboardOverlayRect(keyboard_rect);
 
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->NotifyVirtualKeyboardOverlayRect(keyboard_rect);
   }
 }
@@ -766,7 +783,7 @@
   if (event_handler_) {
     event_handler_->ShowInterestInElement(nodeID);
   }
-  for (ViewAndroid* child : children_) {
+  for (ScopedChildrenIter it(this); ViewAndroid* child = it.GetNext();) {
     child->ShowInterestInElement(nodeID);
   }
 }
@@ -796,7 +813,9 @@
     gfx::Point int_point = gfx::ToFlooredPoint(offset_point);
 
     // Match from back to front for hit testing.
-    for (ViewAndroid* child : base::Reversed(children_)) {
+    for (ScopedChildrenIter it(this,
+                               ScopedChildrenIter::IterationType::kReverse);
+         ViewAndroid* child = it.GetNext();) {
       bool matched = child->match_parent();
       if (!matched)
         matched = child->bounds_dips_.Contains(int_point);
@@ -845,6 +864,53 @@
                                                  janky_frames);
 }
 
+ViewAndroid::ScopedChildrenIter::ScopedChildrenIter(ViewAndroid* view,
+                                                    IterationType type)
+    : view_(view),
+      type_(type),
+      it_(type == IterationType::kForward ? view->children_.begin()
+                                          : view->children_.end()) {
+  CHECK(view_);
+  view_->active_iters_.Append(this);
+}
+
+ViewAndroid::ScopedChildrenIter::~ScopedChildrenIter() {
+  if (view_) {
+    RemoveFromList();
+  }
+}
+
+ViewAndroid* ViewAndroid::ScopedChildrenIter::GetNext() {
+  if (!view_) {
+    return nullptr;
+  }
+
+  // Forward leaves `it_` past the element just returned, so erasing that
+  // element cannot invalidate it. Reverse leaves `it_` on the element just
+  // returned, and relies on StepIteratorsOver() to move it off before an
+  // erase or splice.
+  if (type_ == IterationType::kForward) {
+    return it_ == view_->children_.end() ? nullptr : *it_++;
+  }
+
+  return it_ == view_->children_.begin() ? nullptr : *--it_;
+}
+
+void ViewAndroid::ScopedChildrenIter::ViewDestroyed() {
+  view_ = nullptr;
+  RemoveFromList();
+}
+
+void ViewAndroid::StepIteratorsOver(
+    std::list<raw_ptr<ViewAndroid, CtnExperimental>>::iterator child_it) {
+  for (auto* node = active_iters_.head(); node != active_iters_.end();
+       node = node->next()) {
+    if (node->value()->it_ == child_it) {
+      ++node->value()->it_;
+    }
+  }
+}
+
 }  // namespace ui
 
 DEFINE_JNI(ViewAndroidDelegate)
diff --git a/ui/android/view_android.h b/ui/android/view_android.h
index 02836cf6..0f4e0ef 100644
--- a/ui/android/view_android.h
+++ b/ui/android/view_android.h
@@ -11,6 +11,7 @@
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_weak_ref.h"
+#include "base/containers/linked_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/gtest_prod_util.h"
@@ -341,7 +342,49 @@
   const base::android::ScopedJavaLocalRef<jobject> GetViewAndroidDelegate(
       JNIEnv* env) const;
 
+  // Supports safe traversal of `children_` during synchronous mutations (e.g.
+  // child addition, removal, or reordering during event dispatch). Active
+  // iterators register themselves in `active_iters_` so mutating operations can
+  // update them in-place, preventing UAF and iterator invalidation.
+  class ScopedChildrenIter : public base::LinkNode<ScopedChildrenIter> {
+   public:
+    enum class IterationType {
+      kForward,
+      kReverse,
+    };
+    explicit ScopedChildrenIter(ViewAndroid* view,
+                                IterationType type = IterationType::kForward);
+    ~ScopedChildrenIter();
+    ScopedChildrenIter(const ScopedChildrenIter&) = delete;
+    ScopedChildrenIter& operator=(const ScopedChildrenIter&) = delete;
+
+    // Returns the next element, or nullptr if there are no more elements.
+    // Always called *after* the previous element has been processed, so the
+    // loop body is free to mutate `children_` in between.
+    ViewAndroid* GetNext();
+
+   private:
+    friend class ViewAndroid;
+
+    // Called from `~ViewAndroid` to detach iterators that outlive their view.
+    void ViewDestroyed();
+
+    raw_ptr<ViewAndroid> view_;
+    IterationType type_;
+    std::list<raw_ptr<ViewAndroid, CtnExperimental>>::iterator it_;
+  };
+
+  // Safely advances any active iterators currently pointing to `child_it`
+  // before a mutation (e.g. Remove, MoveToFront) changes the list.
+  void StepIteratorsOver(
+      std::list<raw_ptr<ViewAndroid, CtnExperimental>>::iterator child_it);
+
   std::list<raw_ptr<ViewAndroid, CtnExperimental>> children_;
+
+  // Tracks all active iterators traversing the `children_` list, allowing
+  // real-time updates when the list structure changes (e.g. RemoveChild).
+  base::LinkedList<ScopedChildrenIter> active_iters_;
+
   base::ObserverList<ViewAndroidObserver>::Unchecked observer_list_;
   scoped_refptr<cc::slim::Layer> layer_;
 
diff --git a/ui/android/view_android_unittest.cc b/ui/android/view_android_unittest.cc
index 0ef7559..31ef1e4 100644
--- a/ui/android/view_android_unittest.cc
+++ b/ui/android/view_android_unittest.cc
@@ -4,7 +4,11 @@
 
 #include "ui/android/view_android.h"
 
+#include <utility>
+
 #include "base/android/jni_android.h"
+#include "base/functional/callback.h"
+#include "base/test/bind.h"
 #include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/android/event_forwarder.h"
@@ -121,6 +125,24 @@
     Reset();
   }
 
+  void AddThreeStackedChildren() {
+    view1_.SetLayoutForTesting(50, 50, 400, 600);
+    view2_.SetLayoutForTesting(50, 50, 400, 600);
+    view3_.SetLayoutForTesting(50, 50, 400, 600);
+    root_.AddChild(&view1_);
+    root_.AddChild(&view2_);
+    root_.AddChild(&view3_);
+  }
+
... (file diff truncated)

diff --git a/ui/android/view_android_unittest.cc b/ui/android/view_android_unittest.cc
index 0ef7559..31ef1e4 100644
--- a/ui/android/view_android_unittest.cc
+++ b/ui/android/view_android_unittest.cc
@@ -4,7 +4,11 @@
 
 #include "ui/android/view_android.h"
 
+#include <utility>
+
 #include "base/android/jni_android.h"
+#include "base/functional/callback.h"
+#include "base/test/bind.h"
 #include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/android/event_forwarder.h"
@@ -121,6 +125,24 @@
     Reset();
   }
 
+  void AddThreeStackedChildren() {
+    view1_.SetLayoutForTesting(50, 50, 400, 600);
+    view2_.SetLayoutForTesting(50, 50, 400, 600);
+    view3_.SetLayoutForTesting(50, 50, 400, 600);
+    root_.AddChild(&view1_);
+    root_.AddChild(&view2_);
+    root_.AddChild(&view3_);
+  }
+
+  void RemoveAllChildren(ViewAndroid* view) {
+    view->RemoveAllChildren(/*attached_to_window=*/false);
+  }
+
+  // Teardown path that notifies each child, and so can re-enter.
+  void RemoveAllChildrenAttached(ViewAndroid* view) {
+    view->RemoveAllChildren(/*attached_to_window=*/true);
+  }
+
   TestViewAndroid root_;
   TestViewAndroid view1_;
   TestViewAndroid view2_;
@@ -268,6 +290,140 @@
   EXPECT_FALSE(handler3_.OnSizeCalled());
 }
 
+// Event handler that runs an arbitrary callback during dispatch, for reentrancy
+// tests.
+class ReentrantActionHandler : public EventHandlerAndroid {
+ public:
+  ReentrantActionHandler() = default;
+  explicit ReentrantActionHandler(base::RepeatingClosure action)
+      : action_(std::move(action)) {}
+
+  bool OnTouchEvent(const MotionEventAndroid& event) override {
+    ++count_;
+    if (action_) {
+      action_.Run();
+    }
+    return false;
+  }
+
+  void OnPhysicalBackingSizeChanged(
+      std::optional<base::TimeDelta> deadline_override) override {
+    ++count_;
+    if (action_) {
+      action_.Run();
+    }
+  }
+
+  void set_action(base::RepeatingClosure action) {
+    action_ = std::move(action);
+  }
+  int count() const { return count_; }
+
+ private:
+  base::RepeatingClosure action_;
+  int count_ = 0;
+};
+
+// Exercises StepIteratorsOver across both iteration directions:
+// - Reverse (HitTest): current element erases its next sibling, and the head
+//   element erases itself.
+// - Forward (OnPhysicalBackingSizeChanged): current element erases its next
+//   sibling (cursor sits on it and must be bumped), and the tail erases itself.
+TEST_F(ViewAndroidBoundsTest, ChildRemovalReentrancy) {
+  AddThreeStackedChildren();
+
+  // Reverse pass: view3_ erases view2_ (next); view1_ erases itself (head).
+  ReentrantActionHandler h3(
+      base::BindLambdaForTesting([&]() { view2_.RemoveFromParent(); }));
+  ReentrantActionHandler h2;
+  ReentrantActionHandler h1(
+      base::BindLambdaForTesting([&]() { view1_.RemoveFromParent(); }));
+  view3_.set_event_handler(&h3);
+  view2_.set_event_handler(&h2);
+  view1_.set_event_handler(&h1);
+
+  GenerateTouchEventAt(100.f, 100.f);
+  EXPECT_EQ(1, h3.count());
+  EXPECT_EQ(0, h2.count());
+  EXPECT_EQ(1, h1.count());
+
+  // Reset tree to [view1_, view2_, view3_] for the forward pass: view1_ erases
+  // view2_ (next); view3_ erases itself (tail).
+  RemoveAllChildren(&root_);
+  root_.AddChild(&view1_);
+  root_.AddChild(&view2_);
+  root_.AddChild(&view3_);
+  h1.set_action(
+      base::BindLambdaForTesting([&]() { view2_.RemoveFromParent(); }));
+  h3.set_action(
+      base::BindLambdaForTesting([&]() { view3_.RemoveFromParent(); }));
+
+  root_.OnPhysicalBackingSizeChanged(gfx::Size(100, 100), std::nullopt);
+  EXPECT_EQ(2, h1.count());
+  EXPECT_EQ(0, h2.count());
+  EXPECT_EQ(2, h3.count());
+}
+
+// Reordering mid-traversal via splice: StepIteratorsOver keeps the cursor from
+// following the moved node. Promoting a view moves it into already-visited
+// territory (skipped); demoting moves it ahead of the cursor (still visited).
+TEST_F(ViewAndroidBoundsTest, HitTestReorderReentrancy) {
+  AddThreeStackedChildren();
+  handler1_.SetHandleEvent(false);
+  handler2_.SetHandleEvent(false);
+
+  ReentrantActionHandler h3(base::BindLambdaForTesting([&]() {
+    root_.MoveToFront(&view1_);  // Moved behind reverse cursor -> skipped.
+    root_.MoveToBack(&view2_);   // Moved ahead of reverse cursor -> visited.
+  }));
+  view3_.set_event_handler(&h3);
+
+  GenerateTouchEventAt(100.f, 100.f);
+
+  EXPECT_EQ(1, h3.count());
+  EXPECT_TRUE(handler2_.TouchEventCalled());
+  EXPECT_FALSE(handler1_.TouchEventCalled());
+}
+
+// RemoveAllChildren() clears `parent_` on each child it detaches. If the child
+// reparents itself from its OnDetachedFromWindow() notification, that clear
+// must not run afterwards and strand the child with a null parent.
+TEST_F(ViewAndroidBoundsTest, RemoveAllChildrenKeepsReparentedChild) {
+  class ReparentingObserver : public ViewAndroidObserver {
+   public:
+    ReparentingObserver(ViewAndroid* child, ViewAndroid* new_parent)
+        : child_(child), new_parent_(new_parent) {}
+
+    void OnDetachedFromWindow() override {
+      if (reparented_) {
+        return;
+      }
+      reparented_ = true;
+      new_parent_->AddChild(child_);
+    }
+
+   private:
+    raw_ptr<ViewAndroid> child_;
+    raw_ptr<ViewAndroid> new_parent_;
+    bool reparented_ = false;
+  };
+
+  TestViewAndroid new_parent(ViewAndroid::LayoutType::kNormal);
+  root_.AddChild(&view1_);
+  root_.AddChild(&view2_);
+
+  ReparentingObserver observer(&view2_, &new_parent);
+  view2_.AddObserver(&observer);
+
+  RemoveAllChildrenAttached(&root_);
+
+  EXPECT_EQ(&new_parent, view2_.parent());
+  EXPECT_EQ(1u, new_parent.GetChildrenCountForTesting());
+  EXPECT_EQ(0u, root_.GetChildrenCountForTesting());
+
+  view2_.RemoveObserver(&observer);
+}
+
 TEST(ViewAndroidTest, ChecksMultipleEventForwarders) {
   ViewAndroid parent;
   ViewAndroid child;