Fix leaking OffscreenSurface objects

SkiaRenderer was failing to delete OffscreenSurface objects
corresponding to RenderPasses that are drawn to texture because of a
CopyOutputRequest but not embedded in another RenderPass. No
ImageContextImpl would be created but an OffscreenSurface would be. In
SkiaOutputSurfaceImpl::RemoveRenderPassResources() it ignores
RenderPassIds with no corresponding ImageContextImpl.

This caused a memory leak for the lifetime of the SkiaOutputSurfaceImpl
unless another RenderPass with the same ID was created later. It also
likely caused some size mismatches if another RenderPass with the same
ID but a different size was created later.

Bug: 1043676
Change-Id: I3dea76bc20171146814bf47b2fc3d3ded40f8fd1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067332
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743350}

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -113,26 +113,26 @@ SkiaOutputSurfaceImpl::~SkiaOutputSurfaceImpl() {
   current_paint_.reset();
   root_recorder_.reset();
 
-  std::vector<std::unique_ptr<ImageContextImpl>> render_pass_image_contexts;
-  render_pass_image_contexts.reserve(render_pass_image_cache_.size());
-  for (auto& id_and_image_context : render_pass_image_cache_) {
-    id_and_image_context.second->clear_image();
-    render_pass_image_contexts.push_back(
-        std::move(id_and_image_context.second));
+  if (!render_pass_image_cache_.empty()) {
+    std::vector<RenderPassId> render_pass_ids;
+    render_pass_ids.reserve(render_pass_ids.size());
+    for (auto& entry : render_pass_image_cache_)
+      render_pass_ids.push_back(entry.first);
+    RemoveRenderPassResource(std::move(render_pass_ids));
   }
+  DCHECK(render_pass_image_cache_.empty());
 
+  // Post a task to destroy |impl_on_gpu_| on the GPU thread and block until
+  // that is finished.
   base::WaitableEvent event;
-  auto callback = base::BindOnce(
-      [](std::vector<std::unique_ptr<ImageContextImpl>> render_passes,
-         std::unique_ptr<SkiaOutputSurfaceImplOnGpu> impl_on_gpu,
+  auto task = base::BindOnce(
+      [](std::unique_ptr<SkiaOutputSurfaceImplOnGpu> impl_on_gpu,
          base::WaitableEvent* event) {
-        if (!render_passes.empty())
-          impl_on_gpu->RemoveRenderPassResource(std::move(render_passes));
-        impl_on_gpu = nullptr;
+        impl_on_gpu.reset();
         event->Signal();
       },
-      std::move(render_pass_image_contexts), std::move(impl_on_gpu_), &event);
-  ScheduleGpuTask(std::move(callback), {});
+      std::move(impl_on_gpu_), &event);
+  ScheduleGpuTask(std::move(task), {});
   event.Wait();
 
   gpu_task_scheduler_.reset();
@@ -636,12 +636,11 @@ void SkiaOutputSurfaceImpl::RemoveRenderPassResource(
 
   // impl_on_gpu_ is released on the GPU thread by a posted task from
   // SkiaOutputSurfaceImpl::dtor. So it is safe to use base::Unretained.
-  if (!image_contexts.empty()) {
-    auto callback = base::BindOnce(
-        &SkiaOutputSurfaceImplOnGpu::RemoveRenderPassResource,
-        base::Unretained(impl_on_gpu_.get()), std::move(image_contexts));
-    ScheduleGpuTask(std::move(callback), {});
-  }
+  auto callback =
+      base::BindOnce(&SkiaOutputSurfaceImplOnGpu::RemoveRenderPassResource,
+                     base::Unretained(impl_on_gpu_.get()), std::move(ids),
+                     std::move(image_contexts));
+  ScheduleGpuTask(std::move(callback), {});
 }
 
 void SkiaOutputSurfaceImpl::CopyOutput(

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -1122,19 +1122,23 @@ void SkiaOutputSurfaceImplOnGpu::FinishPaintRenderPass(
 }
 
 void SkiaOutputSurfaceImplOnGpu::RemoveRenderPassResource(
+    std::vector<RenderPassId> ids,
     std::vector<std::unique_ptr<ImageContextImpl>> image_contexts) {
   TRACE_EVENT0("viz", "SkiaOutputSurfaceImplOnGpu::RemoveRenderPassResource");
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  DCHECK(!image_contexts.empty());
-  for (auto& image_context : image_contexts) {
+  DCHECK(!ids.empty());
+
+  for (RenderPassId id : ids) {
     // It's possible that |offscreen_surfaces_| won't contain an entry for the
     // render pass if draw failed early.
-    auto it = offscreen_surfaces_.find(image_context->render_pass_id());
-    if (it == offscreen_surfaces_.end())
-      continue;
-    DeleteSkSurface(context_state_.get(), it->second.TakeSurface());
-    offscreen_surfaces_.erase(it);
+    auto it = offscreen_surfaces_.find(id);
+    if (it != offscreen_surfaces_.end()) {
+      DeleteSkSurface(context_state_.get(), it->second.TakeSurface());
+      offscreen_surfaces_.erase(it);
+    }
   }
+
+  // |image_contexts| will go out of scope and be destroyed now.
 }
 
 void SkiaOutputSurfaceImplOnGpu::CopyOutput(

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -154,7 +154,10 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
                              std::vector<ImageContextImpl*> image_contexts,
                              std::vector<gpu::SyncToken> sync_tokens,
                              uint64_t sync_fence_release);
+  // Deletes resources for RenderPasses in |ids|. Also takes ownership of
+  // |images_contexts| and destroys them on GPU thread.
   void RemoveRenderPassResource(
+      std::vector<RenderPassId> ids,
       std::vector<std::unique_ptr<ImageContextImpl>> image_contexts);
   void CopyOutput(RenderPassId id,
                   copy_output::RenderPassGeometry geometry,