From 00116e7002fb40c113daaff72864bbe00590e28b Mon Sep 17 00:00:00 2001
From: Jonathan Ross <[email protected]>
Date: Mon, 20 Apr 2026 13:09:14 -0700
Subject: [PATCH] viz: Add hierarchy validation to HitTestAggregator

Add validation of the FrameSink Hierarchy during Aggregation.
We will now confirm that any submitted HitTestRegionList for a child
client is a valid parent-child relationship. We will remove invalid regions from the aggregated data. We will keep them in the HitTestManager as hierarchy updates from the Browser process race with CompositorFrames from the client. So the region may become valid in subsequent frames

Bug: 495852034
Change-Id: I4745481670d012972f28ea268185a2827b4c94ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7748974
Reviewed-by: Kyle Charbonneau <[email protected]>
Commit-Queue: Jonathan Ross <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1617727}
---

diff --git a/components/viz/service/frame_sinks/frame_sink_manager_impl.cc b/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
index 466b1e59..ded3f3eb 100644
--- a/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
+++ b/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
@@ -1525,4 +1525,13 @@
   return gpu_service_;
 }
 
+bool FrameSinkManagerImpl::IsChildOf(const FrameSinkId& parent,
+                                     const FrameSinkId& child) const {
+  auto it = frame_sink_source_map_.find(parent);
+  if (it == frame_sink_source_map_.end()) {
+    return false;
+  }
+  return it->second.children.contains(child);
+}
+
 }  // namespace viz
diff --git a/components/viz/service/frame_sinks/frame_sink_manager_impl.h b/components/viz/service/frame_sinks/frame_sink_manager_impl.h
index 5ab48ff..bf25d22 100644
--- a/components/viz/service/frame_sinks/frame_sink_manager_impl.h
+++ b/components/viz/service/frame_sinks/frame_sink_manager_impl.h
@@ -279,6 +279,10 @@
   void RemoveHitTestRegionObserver(HitTestRegionObserver* observer) override;
   const DisplayHitTestQueryMap& GetDisplayHitTestQuery() const override;
 
+  // HitTestAggregatorDelegate and HitTestManager::Delegate implementation:
+  bool IsChildOf(const FrameSinkId& parent,
+                 const FrameSinkId& child) const override;
+
   // CompositorFrameSinkSupport, hierarchy, and BeginFrameSource can be
   // registered and unregistered in any order with respect to each other.
   //
diff --git a/components/viz/service/hit_test/hit_test_aggregator.cc b/components/viz/service/hit_test/hit_test_aggregator.cc
index 40f0acd..9c5fa773 100644
--- a/components/viz/service/hit_test/hit_test_aggregator.cc
+++ b/components/viz/service/hit_test/hit_test_aggregator.cc
@@ -4,13 +4,20 @@
 
 #include "components/viz/service/hit_test/hit_test_aggregator.h"
 
+#include "base/feature.h"
+#include "base/feature_list.h"
 #include "base/trace_event/trace_event.h"
+#include "base/types/expected.h"
 #include "components/viz/common/hit_test/hit_test_region_list.h"
 #include "components/viz/service/hit_test/hit_test_aggregator_delegate.h"
 #include "components/viz/service/surfaces/latest_local_surface_id_lookup_delegate.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 
 namespace viz {
+namespace {
+// TODO (crbug.com/495852034): Remove once M150 hits Stable.
+BASE_FEATURE(kRejectInvalidChildRegions, base::FEATURE_ENABLED_BY_DEFAULT);
+}  // namespace
 
 HitTestAggregator::HitTestAggregator(
     const HitTestManager* hit_test_manager,
@@ -106,8 +113,16 @@
   for (const auto& region : hit_test_region_list->regions) {
     if (region_index >= hit_test_data_capacity_ - 1)
       break;
-    region_index = AppendRegion(region_index, region);
-    DCHECK_EQ(referenced_child_regions_.size(), 1u);
+    // In the call to `AppendRegion` the invalid child regions are not
+    // added to `hit_test_data_`. We need to complete the processing of the
+    // root itself. Otherwise we will have an invalid map to send to the Viz
+    // host.
+    if (auto result =
+            AppendRegion(region_index, region, surface_id.frame_sink_id());
+        result.has_value()) {
+      region_index = result.value();
+      DCHECK_EQ(referenced_child_regions_.size(), 1u);
+    }
   }
   referenced_child_regions_.erase(referenced_child_regions_.begin());
 
@@ -119,8 +134,10 @@
               child_count);
 }
 
-size_t HitTestAggregator::AppendRegion(size_t region_index,
-                                       const HitTestRegion& region) {
+base::expected<size_t, HitTestAggregator::AggregationError>
+HitTestAggregator::AppendRegion(size_t region_index,
+                                const HitTestRegion& region,
+                                const FrameSinkId& submitting_frame_sink_id) {
   size_t parent_index = region_index++;
   if (region_index >= hit_test_data_capacity_ - 1) {
     if (hit_test_data_capacity_ > max_region_size_) {
@@ -136,8 +153,17 @@
   gfx::Transform transform = region.transform;
 
   if (region.flags & HitTestRegionFlags::kHitTestChildSurface) {
-    if (referenced_child_regions_.count(region.frame_sink_id))
+    if (referenced_child_regions_.count(region.frame_sink_id)) {
+      // This detects potential cycles within the HitTestRegions. We want to
+      // keep the single entry as a valid region.
       return parent_index;
+    }
+
+    // Verify that the child is actually a child of the submitting frame sink.
+    if (base::FeatureList::IsEnabled(kRejectInvalidChildRegions) &&
+        !delegate_->IsChildOf(submitting_frame_sink_id, region.frame_sink_id)) {
+      return base::unexpected(AggregationError::INVALID_CHILD_REGION);
+    }
 
     referenced_child_regions_.insert(region.frame_sink_id);
 
@@ -187,9 +213,17 @@
       }
 
       for (const auto& child_region : hit_test_region_list->regions) {
-        region_index = AppendRegion(region_index, child_region);
-        if (region_index >= hit_test_data_capacity_ - 1)
+        if (auto result =
+                AppendRegion(region_index, child_region, region.frame_sink_id);
+            result.has_value()) {
+          region_index = result.value();
+          if (region_index >= hit_test_data_capacity_ - 1) {
+            break;
+          }
+        } else {
+          // Invalid child region
           break;
+        }
       }
     }
     referenced_child_regions_.erase(region.frame_sink_id);
@@ -198,7 +232,7 @@
   int32_t child_count = region_index - parent_index - 1;
   SetRegionAt(parent_index, region.frame_sink_id, flags, reasons, region.rect,
               transform, child_count);
-  return region_index;
+  return base::ok(region_index);
 }
 
 void HitTestAggregator::SetRegionAt(size_t index,
diff --git a/components/viz/service/hit_test/hit_test_aggregator.h b/components/viz/service/hit_test/hit_test_aggregator.h
index 5d302fa..fd7c2168e 100644
--- a/components/viz/service/hit_test/hit_test_aggregator.h
+++ b/components/viz/service/hit_test/hit_test_aggregator.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/memory/raw_ptr.h"
+#include "base/types/expected.h"
 #include "components/viz/common/hit_test/aggregated_hit_test_region.h"
 #include "components/viz/common/hit_test/hit_test_query.h"
 #include "components/viz/common/quads/aggregated_render_pass.h"
@@ -58,6 +59,11 @@
  private:
   friend class TestHitTestAggregator;
 
+  // TODO(jonross): add the capacity error to this and handle that.
+  enum class AggregationError {
+    INVALID_CHILD_REGION,
+  };
+
   void SendHitTestData();
 
   // Appends the root element to the AggregatedHitTestRegion array.
@@ -65,8 +71,12 @@
 
   // Appends a |region| to the HitTestRegionList structure to recursively
   // build the tree. |region_index| indicates the current index of the end of
-  // the list.
-  size_t AppendRegion(size_t region_index, const HitTestRegion& region);
+  // the list. |submitting_frame_sink_id| is the FrameSinkId that submitted the
+  // HitTestRegionList containing |region|.
+  base::expected<size_t, AggregationError> AppendRegion(
+      size_t region_index,
+      const HitTestRegion& region,
+      const FrameSinkId& submitting_frame_sink_id);
 
   // Populates the HitTestRegion element at the given element |index|.
   void SetRegionAt(size_t index,
diff --git a/components/viz/service/hit_test/hit_test_aggregator_delegate.h b/components/viz/service/hit_test/hit_test_aggregator_delegate.h
index af3b1f8..53b14cb 100644
--- a/components/viz/service/hit_test/hit_test_aggregator_delegate.h
+++ b/components/viz/service/hit_test/hit_test_aggregator_delegate.h
@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_VIZ_SERVICE_HIT_TEST_HIT_TEST_AGGREGATOR_DELEGATE_H_

diff --git a/components/viz/service/hit_test/hit_test_aggregator_unittest.cc b/components/viz/service/hit_test/hit_test_aggregator_unittest.cc
index f1924f61..54b0925d 100644
--- a/components/viz/service/hit_test/hit_test_aggregator_unittest.cc
+++ b/components/viz/service/hit_test/hit_test_aggregator_unittest.cc
@@ -164,6 +164,8 @@
 
       if (depth > 0) {
         hit_test_region.flags = HitTestRegionFlags::kHitTestChildSurface;
+        frame_sink_manager()->RegisterFrameSinkHierarchy(
+            surface_id.frame_sink_id(), hit_test_region.frame_sink_id);
         client_id =
             CreateAndSubmitHitTestRegionListWith8Children(client_id, depth - 1);
       } else {
@@ -353,6 +355,11 @@
   SurfaceId c1_surface_id = MakeSurfaceId(kDisplayClientId + 1);
   SurfaceId c2_surface_id = MakeSurfaceId(kDisplayClientId + 2);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c1_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c2_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -438,6 +445,9 @@
   SurfaceId e_surface_id = MakeSurfaceId(kDisplayClientId);
   SurfaceId c_surface_id = MakeSurfaceId(kDisplayClientId + 1);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -518,6 +528,9 @@
   SurfaceId e_surface_id = MakeSurfaceId(kDisplayClientId);
   SurfaceId c_surface_id = MakeSurfaceId(kDisplayClientId + 1);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -601,6 +614,13 @@
   SurfaceId a_surface_id = MakeSurfaceId(kDisplayClientId + 2);
   SurfaceId b_surface_id = MakeSurfaceId(kDisplayClientId + 3);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      c_surface_id.frame_sink_id(), a_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      c_surface_id.frame_sink_id(), b_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -728,6 +748,13 @@
   SurfaceId c2_surface_id = MakeSurfaceId(kDisplayClientId + 2);
   SurfaceId c3_surface_id = MakeSurfaceId(kDisplayClientId + 3);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c1_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      c1_surface_id.frame_sink_id(), c2_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      c2_surface_id.frame_sink_id(), c3_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -848,6 +875,9 @@
   SurfaceId e_surface_id = MakeSurfaceId(kDisplayClientId);
   SurfaceId c_surface_id = MakeSurfaceId(kDisplayClientId + 1);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -956,6 +986,9 @@
   SurfaceId e_surface_id = MakeSurfaceId(kDisplayClientId);
   SurfaceId c_surface_id = MakeSurfaceId(kDisplayClientId + 1);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -1041,6 +1074,13 @@
   SurfaceId c2_surface_id = MakeSurfaceId(kDisplayClientId + 2);
   SurfaceId d1_surface_id = MakeSurfaceId(kDisplayClientId + 3);
 
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c1_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      e_surface_id.frame_sink_id(), c2_surface_id.frame_sink_id());
+  frame_sink_manager()->RegisterFrameSinkHierarchy(
+      c1_surface_id.frame_sink_id(), d1_surface_id.frame_sink_id());
+
   HitTestRegionList e_hit_test_region_list;
   e_hit_test_region_list.flags = HitTestRegionFlags::kHitTestMine;
   e_hit_test_region_list.bounds.SetRect(0, 0, 1024, 768);
@@ -1175,4 +1215,68 @@
   EXPECT_NE(last_index, aggregator->GetLastSubmitHitTestRegionListIndex());
 }
 
+TEST_F(HitTestAggregatorTest, InvalidChildFrameSinkIdRejected) {
+  // Setup: Parent (P) and Child (C).
+  // Browser registers C as child of P.
+  // BUT we will also have an unrelated Sibling (S).
+  FrameSinkId parent_id(1, 1);
+  FrameSinkId child_id(1, 2);
+  FrameSinkId sibling_id(1, 3);
+
+  // Register legitimate hierarchy.
+  frame_sink_manager()->RegisterFrameSinkId(parent_id, true);
+  frame_sink_manager()->RegisterFrameSinkId(sibling_id, true);
+  frame_sink_manager()->RegisterFrameSinkHierarchy(parent_id, child_id);
+  frame_sink_manager()->RegisterFrameSinkHierarchy(parent_id, sibling_id);
+
+  // Now submit hit-test data.
+  HitTestRegionList hit_test_region_list;
+  hit_test_region_list.bounds = gfx::Rect(0, 0, 100, 100);
+
+  // Invalid data for a sibling.
+  HitTestRegion spoofed_sibling;
+  spoofed_sibling.frame_sink_id = sibling_id;
+  spoofed_sibling.flags = HitTestRegionFlags::kHitTestChildSurface |
+                          HitTestRegionFlags::kHitTestMine;
+  spoofed_sibling.rect = gfx::Rect(50, 50, 50, 50);
+  hit_test_region_list.regions.push_back(spoofed_sibling);
+
+  SurfaceId child_surface_id(
+      child_id, LocalSurfaceId(1, 1, base::UnguessableToken::Create()));
+
+  // Register child in delegate.
+  local_surface_id_lookup_delegate()->SetSurfaceIdMap(child_surface_id);
+
+  // Submit CompositorFrame which also submits hit-test data.
+  // This should be accepted, because the full FrameSink hierarchy arrives
+  // asynchronously. We can only validate during aggregation.
+  auto child_support = std::make_unique<CompositorFrameSinkSupport>(
+      nullptr, frame_sink_manager(), child_id, /*is_root=*/false);
+  child_support->SubmitCompositorFrame(child_surface_id.local_surface_id(),
+                                       MakeDefaultCompositorFrame(),
+                                       std::move(hit_test_region_list), 0);
+
+  // We should have a list active before aggregation.
+  const HitTestRegionList* active_list =
+      hit_test_manager()->GetActiveHitTestRegionList(
+          local_surface_id_lookup_delegate(), child_id);
+  EXPECT_NE(nullptr, active_list);
+
+  // Aggregation will detect the non-child and omit that region.
+  // The root region itself is still added.
+  hit_test_aggregator()->Aggregate(child_surface_id);
+  EXPECT_EQ(1, hit_test_aggregator()->GetRegionCount());
+
+  // The HitTestRegionList for child_id should still exist in HitTestManager,
+  // AND its regions should still be there, so that it can be aggregated if
+  // the hierarchy changes.
+  const HitTestRegionList* post_aggregation_list =
+      hit_test_manager()->GetActiveHitTestRegionList(
+          local_surface_id_lookup_delegate(), child_id);
+  EXPECT_NE(nullptr, post_aggregation_list);
+  EXPECT_FALSE(post_aggregation_list->regions.empty());
+  EXPECT_EQ(1u, post_aggregation_list->regions.size());
+  EXPECT_EQ(sibling_id, post_aggregation_list->regions[0].frame_sink_id);
+}
+
 }  // namespace viz