Invalidate hit test data when a surface becomes active

viz::HitTestManager::submit_hit_test_region_list_index_ is used to avoid
sending hit test information to the browser if the underlying hit test
data hasn't changed. It increments the index every time a new batch of
hit test data arrives from a renderer process. However, there is another
way that hit test data can be invalidated, requiring hit test data to
be re-aggregated and sent to the browser:

HitTestAggregator::Aggregate is not a simple pass-through for the data
collected by HitTestManager. It also modifies flags on the hit test
regions, based on whether or not a surface is active (see
HitTestRegionFlags::kHitTestNotActive). "Active", in this case, does
*not* mean that the surface has submitted a compositor frame. Rather,
it means that the surface has submitted a compositor frame *and* the
frame was included in the output of SurfaceAggregator::Aggregate. It is
possible for a surface to change from inactive to active without
triggering the re-aggregation of hit test data. In particular:

- child surface submits a compositor frame with hit test data
- SurfaceAggregator omits the child surface, because the parent surface
  has not yet reported bounds for the child surface.
- HitTestAggregator marks the child surface's hit test regions as
  kHitTestNotActive.
- parent surface submits a compositor frame with bounds for the child
  surface, but without new hit test data.

In this situation, HitTestAggregator will never send new hit test data
to the browser with updated flags for the child surface's regions.

I have not seen this fail in the wild, but then again I don't generally
pay attention to viz bugs. For the bug in question, I have a separate
fix in blink; but that fix causes some changes in the order of data
sent from the renderer to viz, which exacerbates this hit test data
problem, causing failures in SitePerProcessHitTestBrowsertest.

BUG=1033746

Change-Id: Ic338ad64465a1427ed29017b20370b54ed747d76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2044435
Commit-Queue: Stefan Zager <szager@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743307}

components/viz/service/display/frame_rate_decider_unittest.cc

@@ -51,6 +51,7 @@ class FrameRateDeciderTest : public testing::Test,
       const FrameSinkId& frame_sink_id) const override {
     return base::StringPiece();
   }
+  void AggregatedFrameSinksChanged() override {}
 
  protected:
   base::WeakPtr<SurfaceClient> surface_client() {

components/viz/service/display/surface_aggregator.cc

@@ -93,6 +93,7 @@ struct SurfaceAggregator::PrewalkResult {
   // not included in a SurfaceDrawQuad.
   base::flat_set<SurfaceId> undrawn_surfaces;
   bool may_contain_video = false;
+  bool frame_sinks_changed = false;
   gfx::ContentColorUsage content_color_usage = gfx::ContentColorUsage::kSRGB;
 };
 
@@ -1318,6 +1319,8 @@ gfx::Rect SurfaceAggregator::PrewalkTree(Surface* surface,
   contained_surfaces_[surface->surface_id()] = surface->GetActiveFrameIndex();
   LocalSurfaceId& local_surface_id =
       contained_frame_sinks_[surface->surface_id().frame_sink_id()];
+  result->frame_sinks_changed |= (!previous_contained_frame_sinks_.contains(
+      surface->surface_id().frame_sink_id()));
   local_surface_id =
       std::max(surface->surface_id().local_surface_id(), local_surface_id);
 
@@ -1677,6 +1680,9 @@ CompositorFrame SurfaceAggregator::Aggregate(
       PrewalkTree(surface, false, 0, true /* will_draw */, &prewalk_result);
   root_content_color_usage_ = prewalk_result.content_color_usage;
 
+  if (prewalk_result.frame_sinks_changed)
+    manager_->AggregatedFrameSinksChanged();
+
   PropagateCopyRequestPasses();
   has_copy_requests_ = !copy_request_passes_.empty();
   frame.metadata.may_contain_video = prewalk_result.may_contain_video;

components/viz/service/display/surface_aggregator_unittest.cc

@@ -6221,5 +6221,63 @@ TEST_F(SurfaceAggregatorValidSurfaceTest,
             aggregated_frame.render_pass_list[4]->damage_rect.ToString());
 }
 
+TEST_F(SurfaceAggregatorValidSurfaceTest,
+       ContainedFrameSinkChangeInvalidatesHitTestData) {
+  auto embedded_support = std::make_unique<CompositorFrameSinkSupport>(
+      nullptr, &manager_, kArbitraryFrameSinkId1, kRootIsRoot);
+  ParentLocalSurfaceIdAllocator embedded_allocator;
+  embedded_allocator.GenerateId();
+  LocalSurfaceId embedded_local_surface_id =
+      embedded_allocator.GetCurrentLocalSurfaceIdAllocation()
+          .local_surface_id();
+  SurfaceId embedded_surface_id(embedded_support->frame_sink_id(),
+                                embedded_local_surface_id);
+  SurfaceId root_surface_id(root_sink_->frame_sink_id(),
+                            root_local_surface_id_);
+
+  // First submit a root frame which doesn't reference the embedded frame and
+  // aggregate.
+  {
+    std::vector<Quad> embedded_quads = {
+        Quad::SolidColorQuad(SK_ColorGREEN, gfx::Rect(5, 5)),
+        Quad::SolidColorQuad(SK_ColorGRAY, gfx::Rect(5, 5))};
+    std::vector<Pass> embedded_passes = {Pass(embedded_quads, SurfaceSize())};
+    SubmitCompositorFrame(embedded_support.get(), embedded_passes,
+                          embedded_local_surface_id, 1.0f);
+
+    std::vector<Quad> root_quads = {
+        Quad::SolidColorQuad(SK_ColorRED, gfx::Rect(5, 5)),
+        Quad::SolidColorQuad(SK_ColorBLUE, gfx::Rect(5, 5))};
+    std::vector<Pass> root_passes = {Pass(root_quads, SurfaceSize())};
+    SubmitCompositorFrame(root_sink_.get(), root_passes, root_local_surface_id_,
+                          1.0f);
+    AggregateFrame(root_surface_id);
+  }
+
+  const HitTestManager* hit_test_manager = manager_.hit_test_manager();
+  uint64_t hit_test_region_index =
+      hit_test_manager->submit_hit_test_region_list_index();
+
+  // Now submit a root frame that *does* reference the embedded frame, and
+  // aggregate.
+  {
+    std::vector<Quad> root_quads = {
+        Quad::SurfaceQuad(SurfaceRange(base::nullopt, embedded_surface_id),
+                          SK_ColorWHITE, gfx::Rect(5, 5), false),
+        Quad::SolidColorQuad(SK_ColorRED, gfx::Rect(5, 5)),
+        Quad::SolidColorQuad(SK_ColorBLUE, gfx::Rect(5, 5))};
+    std::vector<Pass> root_passes = {Pass(root_quads, SurfaceSize())};
+
+    SubmitCompositorFrame(root_sink_.get(), root_passes, root_local_surface_id_,
+                          1.0);
+    AggregateFrame(root_surface_id);
+  }
+
+  // Check that the HitTestManager was marked as needing to re-aggregate hit
+  // test data.
+  EXPECT_GT(hit_test_manager->submit_hit_test_region_list_index(),
+            hit_test_region_index);
+}
+
 }  // namespace
 }  // namespace viz

components/viz/service/frame_sinks/frame_sink_manager_impl.cc

@@ -339,6 +339,10 @@ base::StringPiece FrameSinkManagerImpl::GetFrameSinkDebugLabel(
   return base::StringPiece();
 }
 
+void FrameSinkManagerImpl::AggregatedFrameSinksChanged() {
+  hit_test_manager_.SetNeedsSubmit();
+}
+
 void FrameSinkManagerImpl::RegisterCompositorFrameSinkSupport(
     const FrameSinkId& frame_sink_id,
     CompositorFrameSinkSupport* support) {

components/viz/service/frame_sinks/frame_sink_manager_impl.h

@@ -145,6 +145,7 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl
   // SurfaceManagerDelegate implementation:
   base::StringPiece GetFrameSinkDebugLabel(
       const FrameSinkId& frame_sink_id) const override;
+  void AggregatedFrameSinksChanged() override;
 
   // CompositorFrameSinkSupport, hierarchy, and BeginFrameSource can be
   // registered and unregistered in any order with respect to each other.

components/viz/service/hit_test/hit_test_manager.h

@@ -64,6 +64,7 @@ class VIZ_SERVICE_EXPORT HitTestManager : public SurfaceObserver {
   uint64_t submit_hit_test_region_list_index() const {
     return submit_hit_test_region_list_index_;
   }
+  void SetNeedsSubmit() { submit_hit_test_region_list_index_++; }
 
  private:
   bool ValidateHitTestRegionList(const SurfaceId& surface_id,

components/viz/service/surfaces/surface_manager.cc

@@ -641,4 +641,9 @@ bool SurfaceManager::HasBlockedEmbedder(
   return false;
 }
 
+void SurfaceManager::AggregatedFrameSinksChanged() {
+  if (delegate_)
+    delegate_->AggregatedFrameSinksChanged();
+}
+
 }  // namespace viz

components/viz/service/surfaces/surface_manager.h

@@ -193,6 +193,10 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   // |frame_sink_id|.
   bool HasBlockedEmbedder(const FrameSinkId& frame_sink_id) const;
 
+  // Indicates that the set of frame sinks being aggregated for display has
+  // changed since the previous aggregation.
+  void AggregatedFrameSinksChanged();
+
  private:
   friend class CompositorFrameSinkSupportTest;
   friend class FrameSinkManagerTest;

components/viz/service/surfaces/surface_manager_delegate.h

@@ -17,6 +17,10 @@ class VIZ_SERVICE_EXPORT SurfaceManagerDelegate {
   // Returns the debug label associated with |frame_sink_id| if any.
   virtual base::StringPiece GetFrameSinkDebugLabel(
       const FrameSinkId& frame_sink_id) const = 0;
+
+  // Indicates that the set of frame sinks being aggregated for display has
+  // changed since the previous aggregation.
+  virtual void AggregatedFrameSinksChanged() = 0;
 };
 
 }  // namespace viz