viz: Remove SharedBitmaps from ServerSharedBitmapManager on disconnect

When a renderer or other viz client shuts down, the CompositorFrameSink
may not have been notified about all the SharedBitmapIds being deleted.
In that case the shared memory would stay in the display compositor's
ServerSharedBitmapManager forever, leaking that memory space. So have
each CompositorFrameSink implementation remember what SharedBitmapIds
it has told the display compositor about, and remove them all when the
implementation is destroyed.

R=kylechar@chromium.org
NOTRY=true

Bug: 730660
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ia1aff6f9035b4713ece787f49ceb1d6c0f839c15
Reviewed-on: https://chromium-review.googlesource.com/929989
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538957}

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -57,6 +57,12 @@ CompositorFrameSinkSupport::~CompositorFrameSinkSupport() {
   EvictCurrentSurface();
   frame_sink_manager_->UnregisterCompositorFrameSinkSupport(frame_sink_id_);
 
+  // The display compositor has ownership of shared memory for each
+  // SharedBitmapId that has been reported from the client. Since the client is
+  // gone that memory can be freed. If we don't then it would leak.
+  for (const auto& id : owned_bitmaps_)
+    ServerSharedBitmapManager::current()->ChildDeletedSharedBitmap(id);
+
   // No video capture clients should remain at this point.
   DCHECK(capture_clients_.empty());
 }
@@ -188,13 +194,18 @@ void CompositorFrameSinkSupport::SubmitCompositorFrame(
 bool CompositorFrameSinkSupport::DidAllocateSharedBitmap(
     mojo::ScopedSharedBufferHandle buffer,
     const SharedBitmapId& id) {
-  return ServerSharedBitmapManager::current()->ChildAllocatedSharedBitmap(
-      std::move(buffer), id);
+  if (!ServerSharedBitmapManager::current()->ChildAllocatedSharedBitmap(
+          std::move(buffer), id))
+    return false;
+
+  owned_bitmaps_.insert(id);
+  return true;
 }
 
 void CompositorFrameSinkSupport::DidDeleteSharedBitmap(
     const SharedBitmapId& id) {
   ServerSharedBitmapManager::current()->ChildDeletedSharedBitmap(id);
+  owned_bitmaps_.erase(id);
 }
 
 CompositorFrameSinkSupport::SubmitResult

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -231,6 +231,11 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   // frames.
   std::vector<CapturableFrameSink::Client*> capture_clients_;
 
+  // The set of SharedBitmapIds that have been reported as allocated to this
+  // interface. On closing this interface, the display compositor should drop
+  // ownership of the bitmaps with these ids to avoid leaking them.
+  std::set<SharedBitmapId> owned_bitmaps_;
+
   base::WeakPtrFactory<CompositorFrameSinkSupport> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkSupport);

components/viz/test/test_layer_tree_frame_sink.cc

@@ -55,6 +55,12 @@ TestLayerTreeFrameSink::TestLayerTreeFrameSink(
 
 TestLayerTreeFrameSink::~TestLayerTreeFrameSink() {
   DCHECK(copy_requests_.empty());
+
+  // The shared_bitmap_manager() has ownership of shared memory for each
+  // SharedBitmapId that has been reported from the client. Since the client is
+  // gone that memory can be freed. If we don't then it would leak.
+  for (const auto& id : owned_bitmaps_)
+    shared_bitmap_manager()->ChildDeletedSharedBitmap(id);
 }
 
 void TestLayerTreeFrameSink::SetDisplayColorSpace(
@@ -194,10 +200,12 @@ void TestLayerTreeFrameSink::DidAllocateSharedBitmap(
   bool ok = shared_bitmap_manager()->ChildAllocatedSharedBitmap(
       std::move(buffer), id);
   DCHECK(ok);
+  owned_bitmaps_.insert(id);
 }
 
 void TestLayerTreeFrameSink::DidDeleteSharedBitmap(const SharedBitmapId& id) {
   shared_bitmap_manager()->ChildDeletedSharedBitmap(id);
+  owned_bitmaps_.erase(id);
 }
 
 void TestLayerTreeFrameSink::DidReceiveCompositorFrameAck(

components/viz/test/test_layer_tree_frame_sink.h

@@ -155,6 +155,10 @@ class TestLayerTreeFrameSink : public cc::LayerTreeFrameSink,
   gfx::Size enlarge_pass_texture_amount_;
 
   std::vector<std::unique_ptr<CopyOutputRequest>> copy_requests_;
+  // The set of SharedBitmapIds that have been reported as allocated to this
+  // interface. On closing this interface, the display compositor should drop
+  // ownership of the bitmaps with these ids to avoid leaking them.
+  std::set<SharedBitmapId> owned_bitmaps_;
 
   base::WeakPtrFactory<TestLayerTreeFrameSink> weak_ptr_factory_;
 };

content/browser/renderer_host/render_widget_host_impl.cc

@@ -1841,6 +1841,12 @@ void RenderWidgetHostImpl::Destroy(bool also_delete) {
     view_.reset();
   }
 
+  // The display compositor has ownership of shared memory for each
+  // SharedBitmapId that has been reported from the client. Since the client is
+  // gone that memory can be freed. If we don't then it would leak.
+  for (const auto& id : owned_bitmaps_)
+    viz::ServerSharedBitmapManager::current()->ChildDeletedSharedBitmap(id);
+
   process_->GetSharedBitmapAllocationNotifier()->RemoveObserver(this);
   process_->RemoveWidget(this);
   process_->RemoveRoute(routing_id_);
@@ -2005,11 +2011,13 @@ void RenderWidgetHostImpl::DidAllocateSharedBitmap(
     bad_message::ReceivedBadMessage(GetProcess(),
                                     bad_message::RWH_SHARED_BITMAP);
   }
+  owned_bitmaps_.insert(id);
 }
 
 void RenderWidgetHostImpl::DidDeleteSharedBitmap(
     const viz::SharedBitmapId& id) {
   viz::ServerSharedBitmapManager::current()->ChildDeletedSharedBitmap(id);
+  owned_bitmaps_.erase(id);
 }
 
 void RenderWidgetHostImpl::OnResizeOrRepaintACK(

content/browser/renderer_host/render_widget_host_impl.h

@@ -1084,6 +1084,11 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   base::Optional<uint16_t> screen_orientation_angle_for_testing_;
   base::Optional<ScreenOrientationValues> screen_orientation_type_for_testing_;
 
+  // The set of SharedBitmapIds that have been reported as allocated to this
+  // interface. On closing this interface, the display compositor should drop
+  // ownership of the bitmaps with these ids to avoid leaking them.
+  std::set<viz::SharedBitmapId> owned_bitmaps_;
+
   bool next_resize_needs_resize_ack_ = false;
 
   std::unique_ptr<RenderFrameMetadataProvider> render_frame_metadata_provider_;