Change CompositorFrameSink connection loss.

1. Remove the OnClientConnectionClosed() IPC from GPU to browser.  The
   browser never used the IPC so it wasn't necessary.
2. Destroy CompositorFrameSinkImpl when the client connection is closed.
   The object is useless after that point so it can be cleaned up.
3. Don't destroy RootCompositorFrameSinkImpl when the client connection
   is closed. The display compositor will continue to produce frames.

Bug: 814475
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ie9a96ee504013435c23b05cf6dbaa3fda4e0fd33
Reviewed-on: https://chromium-review.googlesource.com/993993Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548305}

components/viz/host/host_frame_sink_manager.cc

@@ -397,11 +397,6 @@ void HostFrameSinkManager::OnFirstSurfaceActivation(
     frame_sink_data.client->OnFirstSurfaceActivation(surface_info);
 }
 
-void HostFrameSinkManager::OnClientConnectionClosed(
-    const FrameSinkId& frame_sink_id) {
-  // TODO(kylechar): Notify observers.
-}
-
 void HostFrameSinkManager::OnAggregatedHitTestRegionListUpdated(
     const FrameSinkId& frame_sink_id,
     mojo::ScopedSharedBufferHandle active_handle,

components/viz/host/host_frame_sink_manager.h

@@ -223,7 +223,6 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   // mojom::FrameSinkManagerClient:
   void OnSurfaceCreated(const SurfaceId& surface_id) override;
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override;
-  void OnClientConnectionClosed(const FrameSinkId& frame_sink_id) override;
   void OnAggregatedHitTestRegionListUpdated(
       const FrameSinkId& frame_sink_id,
       mojo::ScopedSharedBufferHandle active_handle,

components/viz/service/frame_sinks/compositor_frame_sink_impl.cc

@@ -78,8 +78,12 @@ void CompositorFrameSinkImpl::DidDeleteSharedBitmap(const SharedBitmapId& id) {
 }
 
 void CompositorFrameSinkImpl::OnClientConnectionLost() {
-  support_->frame_sink_manager()->OnClientConnectionLost(
-      support_->frame_sink_id());
+  // The client that owns this CompositorFrameSink is either shutting down or
+  // has done something invalid and the connection to the client was terminated.
+  // Destroy |this| to free up resources as it's no longer useful.
+  FrameSinkId frame_sink_id = support_->frame_sink_id();
+  support_->frame_sink_manager()->DestroyCompositorFrameSink(frame_sink_id,
+                                                             base::DoNothing());
 }
 
 }  // namespace viz

components/viz/service/frame_sinks/compositor_frame_sink_impl.h

@@ -39,8 +39,6 @@ class CompositorFrameSinkImpl : public mojom::CompositorFrameSink {
                                const SharedBitmapId& id) override;
   void DidDeleteSharedBitmap(const SharedBitmapId& id) override;
 
-  CompositorFrameSinkSupport* support() const { return support_.get(); }
-
  private:
   void OnClientConnectionLost();
 

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -68,7 +68,6 @@ class MockFrameSinkManagerClient : public mojom::FrameSinkManagerClient {
   // mojom::FrameSinkManagerClient:
   MOCK_METHOD1(OnSurfaceCreated, void(const SurfaceId&));
   MOCK_METHOD1(OnFirstSurfaceActivation, void(const SurfaceInfo&));
-  void OnClientConnectionClosed(const FrameSinkId& frame_sink_id) override {}
   void OnAggregatedHitTestRegionListUpdated(
       const FrameSinkId& frame_sink_id,
       mojo::ScopedSharedBufferHandle active_handle,

components/viz/service/frame_sinks/frame_sink_manager_impl.cc

@@ -523,13 +523,6 @@ bool FrameSinkManagerImpl::ChildContains(
   return false;
 }
 
-void FrameSinkManagerImpl::OnClientConnectionLost(
-    const FrameSinkId& frame_sink_id) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  if (client_)
-    client_->OnClientConnectionClosed(frame_sink_id);
-}
-
 void FrameSinkManagerImpl::SubmitHitTestRegionList(
     const SurfaceId& surface_id,
     uint64_t frame_index,

components/viz/service/frame_sinks/frame_sink_manager_impl.h

@@ -149,8 +149,6 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl
 
   const HitTestManager* hit_test_manager() { return &hit_test_manager_; }
 
-  void OnClientConnectionLost(const FrameSinkId& frame_sink_id);
-
   void SubmitHitTestRegionList(
       const SurfaceId& surface_id,
       uint64_t frame_index,

components/viz/service/frame_sinks/frame_sink_manager_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include "base/run_loop.h"
 #include "components/viz/common/constants.h"
 #include "components/viz/common/frame_sinks/begin_frame_source.h"
 #include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
@@ -52,7 +53,7 @@ struct RootCompositorFrameSinkData {
 class FrameSinkManagerTest : public testing::Test {
  public:
   FrameSinkManagerTest()
-      : manager_(kDefaultActivationDeadlineInFrames, &provider) {}
+      : manager_(kDefaultActivationDeadlineInFrames, &display_provider_) {}
   ~FrameSinkManagerTest() override = default;
 
   std::unique_ptr<CompositorFrameSinkSupport> CreateCompositorFrameSinkSupport(
@@ -81,7 +82,7 @@ class FrameSinkManagerTest : public testing::Test {
   }
 
  protected:
-  TestDisplayProvider provider;
+  TestDisplayProvider display_provider_;
   FrameSinkManagerImpl manager_;
 };
 
@@ -115,6 +116,33 @@ TEST_F(FrameSinkManagerTest, CreateCompositorFrameSink) {
   EXPECT_FALSE(CompositorFrameSinkExists(kFrameSinkIdA));
 }
 
+TEST_F(FrameSinkManagerTest, CompositorFrameSinkConnectionLost) {
+  manager_.RegisterFrameSinkId(kFrameSinkIdA);
+
+  // Create a CompositorFrameSinkImpl.
+  MockCompositorFrameSinkClient compositor_frame_sink_client;
+  mojom::CompositorFrameSinkPtr compositor_frame_sink;
+  manager_.CreateCompositorFrameSink(
+      kFrameSinkIdA, MakeRequest(&compositor_frame_sink),
+      compositor_frame_sink_client.BindInterfacePtr());
+  EXPECT_TRUE(CompositorFrameSinkExists(kFrameSinkIdA));
+
+  // Close the connection from the renderer.
+  compositor_frame_sink.reset();
+
+  // Closing the connection will destroy the CompositorFrameSinkImpl along with
+  // the mojom::CompositorFrameSinkClient binding.
+  base::RunLoop run_loop;
+  compositor_frame_sink_client.set_connection_error_handler(
+      run_loop.QuitClosure());
+  run_loop.Run();
+
+  // Check that the CompositorFrameSinkImpl was destroyed.
+  EXPECT_FALSE(CompositorFrameSinkExists(kFrameSinkIdA));
+
+  manager_.InvalidateFrameSinkId(kFrameSinkIdA);
+}
+
 TEST_F(FrameSinkManagerTest, SingleClients) {
   auto client = CreateCompositorFrameSinkSupport(FrameSinkId(1, 1));
   auto other_client = CreateCompositorFrameSinkSupport(FrameSinkId(2, 2));

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc

@@ -170,8 +170,9 @@ void RootCompositorFrameSinkImpl::DisplayDidReceiveCALayerParams(
 void RootCompositorFrameSinkImpl::DisplayDidDrawAndSwap() {}
 
 void RootCompositorFrameSinkImpl::OnClientConnectionLost() {
-  support_->frame_sink_manager()->OnClientConnectionLost(
-      support_->frame_sink_id());
+  // TODO(kylechar): I'm not sure what we need to do here. If |this| is
+  // destroyed then |display_| will be destroyed and we'll stop producing
+  // frames.
 }
 
 BeginFrameSource* RootCompositorFrameSinkImpl::begin_frame_source() {

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.h

@@ -42,8 +42,6 @@ class RootCompositorFrameSinkImpl : public mojom::CompositorFrameSink,
 
   ~RootCompositorFrameSinkImpl() override;
 
-  CompositorFrameSinkSupport* support() const { return support_.get(); }
-
   // mojom::DisplayPrivate:
   void SetDisplayVisible(bool visible) override;
   void SetDisplayColorMatrix(const gfx::Transform& color_matrix) override;

components/viz/test/mock_compositor_frame_sink_client.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_VIZ_TEST_MOCK_COMPOSITOR_FRAME_SINK_CLIENT_H_
 #define COMPONENTS_VIZ_TEST_MOCK_COMPOSITOR_FRAME_SINK_CLIENT_H_
 
+#include "base/callback.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/viz/public/interfaces/compositing/compositor_frame_sink.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -16,6 +17,10 @@ class MockCompositorFrameSinkClient : public mojom::CompositorFrameSinkClient {
   MockCompositorFrameSinkClient();
   ~MockCompositorFrameSinkClient() override;
 
+  void set_connection_error_handler(base::OnceClosure error_handler) {
+    binding_.set_connection_error_handler(std::move(error_handler));
+  }
+
   // Returns a CompositorFrameSinkClientPtr bound to this object.
   mojom::CompositorFrameSinkClientPtr BindInterfacePtr();
 

components/viz/test/test_frame_sink_manager_client.h

@@ -38,7 +38,6 @@ class TestFrameSinkManagerClient : public mojom::FrameSinkManagerClient {
   // mojom::FrameSinkManagerClient:
   void OnSurfaceCreated(const SurfaceId& surface_id) override;
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
-  void OnClientConnectionClosed(const FrameSinkId& frame_sink_id) override {}
   void OnAggregatedHitTestRegionListUpdated(
       const FrameSinkId& frame_sink_id,
       mojo::ScopedSharedBufferHandle active_handle,

services/viz/privileged/interfaces/compositing/frame_sink_manager.mojom

@@ -147,10 +147,6 @@ interface FrameSinkManagerClient {
   // SurfaceId activates for the first time.
   OnFirstSurfaceActivation(SurfaceInfo surface_info);
 
-  // The CompositorFrameSink pipe for |frame_sink_id| was closed. The client
-  // cannot submit any CompositorFrames to viz after this occurs.
-  OnClientConnectionClosed(FrameSinkId frame_sink_id);
-
   // Sends |active_handle| and |idle_handle| along with their sizes to the
   // client when they are allocated or resized.
   OnAggregatedHitTestRegionListUpdated(FrameSinkId frame_sink_id,