display compositor: Fix a use-after-free when a frame sink is destroyed.

services/ui/surfaces/display_compositor.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "services/ui/surfaces/display_compositor.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
@@ skipping 33 matching lines @@
       binding_(this, std::move(request)) {
   manager_.AddObserver(this);
 }
 
 DisplayCompositor::~DisplayCompositor() {
   DCHECK(thread_checker_.CalledOnValidThread());
   manager_.RemoveObserver(this);
 }
 
 void DisplayCompositor::OnClientConnectionLost(
     const cc::FrameSinkId& frame_sink_id,

[Fady Samuel, 2017/02/03 01:05:25]

How about making this passed by value instead and adding a comment in
GpuCompositorFrameSinkDelegate as to why it needs to be passed by value?

[sadrul, 2017/02/03 01:28:25]

I went with having DestroyCompositorFrameSink() take a copy-by-value param
instead. Making the change in the delegate interface felt like the wrong thing
to do, since this is really an impl-detail of the DisplayCompositor.

     bool destroy_compositor_frame_sink) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (destroy_compositor_frame_sink)
-    compositor_frame_sinks_.erase(frame_sink_id);
+    DestroyCompositorFrameSink(frame_sink_id);
   // TODO(fsamuel): Tell the display compositor host that the client connection
   // has been lost so that it can drop its private connection and allow a new
   // client instance to create a new CompositorFrameSink.
 }
 
 void DisplayCompositor::OnPrivateConnectionLost(
     const cc::FrameSinkId& frame_sink_id,

[Fady Samuel, 2017/02/03 01:05:25]

How about making this passed by value instead and adding a comment in
GpuCompositorFrameSinkDelegate as to why it needs to be passed by value?

[sadrul, 2017/02/03 01:28:25]

ditto

     bool destroy_compositor_frame_sink) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (destroy_compositor_frame_sink)
-    compositor_frame_sinks_.erase(frame_sink_id);
+    DestroyCompositorFrameSink(frame_sink_id);
 }
 
 void DisplayCompositor::CreateDisplayCompositorFrameSink(
     const cc::FrameSinkId& frame_sink_id,
     gpu::SurfaceHandle surface_handle,
     cc::mojom::MojoCompositorFrameSinkAssociatedRequest request,
     cc::mojom::MojoCompositorFrameSinkPrivateRequest private_request,
     cc::mojom::MojoCompositorFrameSinkClientPtr client,
     cc::mojom::DisplayPrivateAssociatedRequest display_private_request) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ skipping 68 matching lines @@
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           cc::switches::kShowOverdrawFeedback);
 
   return base::MakeUnique<cc::Display>(
       nullptr /* bitmap_manager */, gpu_memory_buffer_manager_.get(), settings,
       frame_sink_id, begin_frame_source, std::move(display_output_surface),
       std::move(scheduler),
       base::MakeUnique<cc::TextureMailboxDeleter>(task_runner_.get()));
 }
 
+void DisplayCompositor::DestroyCompositorFrameSink(
+    const cc::FrameSinkId& frame_sink_id) {
+  // Make a copy of the id before removing it. This is because |frame_sink_id|
+  // is owned by the GpuCompositorFrameSink in the map. So when the sink is
+  // removed from the map, |frame_sink_id| is also destroyed. But the map can
+  // continue to iterate and try to use it. Making a copy of it avoids this.
+  cc::FrameSinkId id = frame_sink_id;
+  compositor_frame_sinks_.erase(id);
+}
+
 void DisplayCompositor::OnSurfaceCreated(const cc::SurfaceInfo& surface_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_GT(surface_info.device_scale_factor(), 0.0f);
 
   if (client_)
     client_->OnSurfaceCreated(surface_info);
 }
 
 void DisplayCompositor::OnSurfaceDamaged(const cc::SurfaceId& surface_id,
                                          bool* changed) {}
 
 }  // namespace ui