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

Make a copy of the sink-id before removing it from the map. This is
because the id variable is owned by the GpuCompositorFrameSink in the
map. So when the sink is removed from the map, the id is also destroyed.
But the map can continue to iterate and try to use it. Making a copy of
it avoids this.

BUG=none

Review-Url: https://codereview.chromium.org/2673823002
Cr-Commit-Position: refs/heads/master@{#447898}

services/ui/surfaces/display_compositor.cc

@@ -55,7 +55,7 @@ void DisplayCompositor::OnClientConnectionLost(
     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.
@@ -66,7 +66,7 @@ void DisplayCompositor::OnPrivateConnectionLost(
     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(
@@ -155,6 +155,10 @@ std::unique_ptr<cc::Display> DisplayCompositor::CreateDisplay(
       base::MakeUnique<cc::TextureMailboxDeleter>(task_runner_.get()));
 }
 
+void DisplayCompositor::DestroyCompositorFrameSink(cc::FrameSinkId sink_id) {
+  compositor_frame_sinks_.erase(sink_id);
+}
+
 void DisplayCompositor::OnSurfaceCreated(const cc::SurfaceInfo& surface_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_GT(surface_info.device_scale_factor(), 0.0f);

services/ui/surfaces/display_compositor.h

@@ -99,6 +99,13 @@ class DisplayCompositor
       cc::mojom::MojoCompositorFrameSinkClientPtr client,
       cc::mojom::DisplayPrivateRequest display_private_request);
 
+  // It is necessary to pass |frame_sink_id| by value because the id
+  // is owned by the GpuCompositorFrameSink in the map. When the sink is
+  // removed from the map, |frame_sink_id| would also be destroyed if it were a
+  // reference. But the map can continue to iterate and try to use it. Passing
+  // by value avoids this.
+  void DestroyCompositorFrameSink(cc::FrameSinkId frame_sink_id);
+
   // cc::SurfaceObserver implementation.
   void OnSurfaceCreated(const cc::SurfaceInfo& surface_info) override;
   void OnSurfaceDamaged(const cc::SurfaceId& surface_id,