viz: Assign temporary refernces with --enable-viz.

Fix the logic that decides whether or not to assign temporary references
in HostFrameSinkManager. This fixes temporary references not being
assigned owners when running with --enable-viz in desktop Chrome. Add a
test to verify temporary references are assigned with a remote
FrameSinkmanager.

Also fix some places in HostFrameSinkManager where |frame_sink_manager_|
and |frame_sink_manager_impl_| were mixed up.

Bug: 787589
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I2b40fa332f8afc8ff7fa7b62482ef174bcbd239a
Reviewed-on: https://chromium-review.googlesource.com/793976Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519861}

components/viz/host/host_frame_sink_manager.cc

@@ -26,6 +26,10 @@ void HostFrameSinkManager::SetLocalManager(
   frame_sink_manager_impl_ = frame_sink_manager_impl;
 
   frame_sink_manager_ = frame_sink_manager_impl;
+
+  // Assign temporary references if FrameSinkManagerImpl is using them.
+  assign_temporary_references_ =
+      frame_sink_manager_impl_->surface_manager()->using_surface_references();
 }
 
 void HostFrameSinkManager::BindAndSetManager(
@@ -176,13 +180,19 @@ void HostFrameSinkManager::UnregisterFrameSinkHierarchy(
     frame_sink_data_map_.erase(parent_frame_sink_id);
 }
 
+void HostFrameSinkManager::WillAssignTemporaryReferencesExternally() {
+  assign_temporary_references_ = false;
+}
+
 void HostFrameSinkManager::AssignTemporaryReference(
     const SurfaceId& surface_id,
     const FrameSinkId& frame_sink_id) {
+  DCHECK(!assign_temporary_references_);
   frame_sink_manager_->AssignTemporaryReference(surface_id, frame_sink_id);
 }
 
 void HostFrameSinkManager::DropTemporaryReference(const SurfaceId& surface_id) {
+  DCHECK(!assign_temporary_references_);
   frame_sink_manager_->DropTemporaryReference(surface_id);
 }
 
@@ -223,10 +233,14 @@ void HostFrameSinkManager::CompositorFrameSinkSupportDestroyed(
 
 void HostFrameSinkManager::PerformAssignTemporaryReference(
     const SurfaceId& surface_id) {
-  // Find the expected embedder for the new surface and assign the temporary
-  // reference to it.
   auto iter = frame_sink_data_map_.find(surface_id.frame_sink_id());
-  DCHECK(iter != frame_sink_data_map_.end());
+  if (iter == frame_sink_data_map_.end()) {
+    // We don't have any hierarchy information for what will embed the new
+    // surface, drop the temporary reference.
+    frame_sink_manager_->DropTemporaryReference(surface_id);
+    return;
+  }
+
   const FrameSinkData& data = iter->second;
 
   // Display roots don't have temporary references to assign.
@@ -244,7 +258,7 @@ void HostFrameSinkManager::PerformAssignTemporaryReference(
   for (const FrameSinkId& parent_id : data.parents) {
     const FrameSinkData& parent_data = frame_sink_data_map_[parent_id];
     if (parent_data.IsFrameSinkRegistered()) {
-      frame_sink_manager_impl_->AssignTemporaryReference(surface_id, parent_id);
+      frame_sink_manager_->AssignTemporaryReference(surface_id, parent_id);
       return;
     }
   }
@@ -264,7 +278,7 @@ void HostFrameSinkManager::OnConnectionLost() {
 
   binding_.Close();
   frame_sink_manager_ptr_.reset();
-  frame_sink_manager_impl_ = nullptr;
+  frame_sink_manager_ = nullptr;
 
   // CompositorFrameSinks are lost along with the connection to
   // mojom::FrameSinkManager.
@@ -297,23 +311,20 @@ void HostFrameSinkManager::RegisterAfterConnectionLoss() {
 
 void HostFrameSinkManager::OnFirstSurfaceActivation(
     const SurfaceInfo& surface_info) {
+  // TODO(kylechar): This needs to happen when the surface is created, not when
+  // it first activates.
+  if (assign_temporary_references_)
+    PerformAssignTemporaryReference(surface_info.id());
+
   auto it = frame_sink_data_map_.find(surface_info.id().frame_sink_id());
+
   // If we've received a bogus or stale SurfaceId from Viz then just ignore it.
-  if (it == frame_sink_data_map_.end()) {
-    // We don't have any hierarchy information for what will embed the new
-    // surface, drop the temporary reference.
-    frame_sink_manager_->DropTemporaryReference(surface_info.id());
+  if (it == frame_sink_data_map_.end())
     return;
-  }
 
   FrameSinkData& frame_sink_data = it->second;
   if (frame_sink_data.client)
     frame_sink_data.client->OnFirstSurfaceActivation(surface_info);
-
-  if (frame_sink_manager_impl_ &&
-      frame_sink_manager_impl_->surface_manager()->using_surface_references()) {
-    PerformAssignTemporaryReference(surface_info.id());
-  }
 }
 
 void HostFrameSinkManager::OnClientConnectionClosed(

components/viz/host/host_frame_sink_manager.h

@@ -44,11 +44,12 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
     : public mojom::FrameSinkManagerClient,
       public CompositorFrameSinkSupportManager {
  public:
+  using DisplayHitTestQueryMap =
+      base::flat_map<FrameSinkId, std::unique_ptr<HitTestQuery>>;
+
   HostFrameSinkManager();
   ~HostFrameSinkManager() override;
 
-  using DisplayHitTestQueryMap =
-      base::flat_map<FrameSinkId, std::unique_ptr<HitTestQuery>>;
   const DisplayHitTestQueryMap& display_hit_test_query() const {
     return display_hit_test_query_;
   }
@@ -114,9 +115,10 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   void UnregisterFrameSinkHierarchy(const FrameSinkId& parent_frame_sink_id,
                                     const FrameSinkId& child_frame_sink_id);
 
-  // These two functions should only be used by WindowServer.
+  // These three functions should only be used by WindowServer.
   // TODO(riajiang): Find a better way for HostFrameSinkManager to do the assign
   // and drop instead.
+  void WillAssignTemporaryReferencesExternally();
   void AssignTemporaryReference(const SurfaceId& surface_id,
                                 const FrameSinkId& owner);
   void DropTemporaryReference(const SurfaceId& surface_id);
@@ -227,6 +229,11 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   // If |frame_sink_manager_ptr_| connection was lost.
   bool connection_was_lost_ = false;
 
+  // If a FrameSinkId owner should be assigned when a new surface is created.
+  // This will be set false if using surface sequences or if temporary
+  // references are being assigned externally.
+  bool assign_temporary_references_ = true;
+
   base::RepeatingClosure connection_lost_callback_;
 
   DisplayHitTestQueryMap display_hit_test_query_;

components/viz/host/host_frame_sink_manager_unittest.cc

@@ -624,5 +624,33 @@ TEST_F(HostFrameSinkManagerRemoteTest, DeletedHitTestQuery) {
       kFrameSinkChild1, 1);
 }
 
+// Verify that HostFrameSinkManager assigns temporary references when connected
+// to a remote mojom::FrameSinkManager.
+TEST_F(HostFrameSinkManagerRemoteTest, AssignTemporaryReference) {
+  FakeHostFrameSinkClient host_client;
+  host().RegisterFrameSinkId(kFrameSinkParent1, &host_client);
+
+  const SurfaceId surface_id = MakeSurfaceId(kFrameSinkChild1, 1);
+  host().RegisterFrameSinkId(surface_id.frame_sink_id(), &host_client);
+  MockCompositorFrameSinkClient compositor_frame_sink_client;
+  mojom::CompositorFrameSinkPtr compositor_frame_sink;
+  host().CreateCompositorFrameSink(
+      kFrameSinkChild1, MakeRequest(&compositor_frame_sink),
+      compositor_frame_sink_client.BindInterfacePtr());
+
+  host().RegisterFrameSinkHierarchy(kFrameSinkParent1,
+                                    surface_id.frame_sink_id());
+
+  // When HostFrameSinkManager gets OnFirstSurfaceActivation() it should assign
+  // the temporary reference to the registered parent |kFrameSinkParent1|.
+  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
+      MakeSurfaceInfo(surface_id));
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(impl(), AssignTemporaryReference(surface_id, kFrameSinkParent1))
+      .WillOnce(InvokeClosure(run_loop.QuitClosure()));
+  run_loop.Run();
+}
+
 }  // namespace test
 }  // namespace viz

services/ui/ws/window_server.cc

@@ -152,6 +152,8 @@ WindowServer::WindowServer(WindowServerDelegate* delegate, bool should_host_viz)
           std::make_unique<VizHostProxyImpl>(host_frame_sink_manager_.get())),
       video_detector_(host_frame_sink_manager_.get()),
       display_creation_config_(DisplayCreationConfig::UNKNOWN) {
+  if (host_frame_sink_manager_)
+    host_frame_sink_manager_->WillAssignTemporaryReferencesExternally();
   user_id_tracker_.AddObserver(this);
   OnUserIdAdded(user_id_tracker_.active_id());
 }