Fix re-entracy problem with InvalidateFrameSinkId().

HostFrameSinkManager::InvalidateFrameSinkId() can make a synchronous IPC
to ensure whatever is drawing the platform window is destroyed before
the platform window gets destroyed. However, while waiting for the
synchronous IPC response other synchronous IPCs continue to get
processed. |frame_sink_data_map_| can get mutated in response to a
different synchronous IPC and |data| may no longer point to a valid
memory location when DestroyCompositorFrameSink() returns.

Change the order so that all modifications to |data| happen before the
synchronous IPC. Also add a comment to clarify that FrameSinkIds
shouldn't be reused after they are invalidated. We don't do this today
and it would cause more re-entrancy problems.

Also switch |frame_sink_data_map_| to use std::unordered_map instead of
base::flat_map. This map can get large (tens of elements per open tab)
so std::unordered_map is probably a better choice.

Bug: 907211
Change-Id: Iea42d1eca290f5b61f215c66da9b9021f671c1e0
Reviewed-on: https://chromium-review.googlesource.com/c/1352525Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613160}

components/viz/host/host_frame_sink_manager.cc

@@ -89,15 +89,9 @@ void HostFrameSinkManager::InvalidateFrameSinkId(
   FrameSinkData& data = frame_sink_data_map_[frame_sink_id];
   DCHECK(data.IsFrameSinkRegistered());
 
-  if (data.has_created_compositor_frame_sink && data.is_root) {
-    // This synchronous call ensures that the GL context/surface that draw to
-    // the platform window (eg. XWindow or HWND) get destroyed before the
-    // platform window is destroyed.
-    mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
-    frame_sink_manager_->DestroyCompositorFrameSink(frame_sink_id);
-  }
+  const bool destroy_synchronously =
+      data.has_created_compositor_frame_sink && data.is_root;
 
-  frame_sink_manager_->InvalidateFrameSinkId(frame_sink_id);
   data.has_created_compositor_frame_sink = false;
   data.client = nullptr;
 
@@ -106,6 +100,21 @@ void HostFrameSinkManager::InvalidateFrameSinkId(
     frame_sink_data_map_.erase(frame_sink_id);
 
   display_hit_test_query_.erase(frame_sink_id);
+
+  if (destroy_synchronously) {
+    // This synchronous call ensures that the GL context/surface that draw to
+    // the platform window (eg. XWindow or HWND) get destroyed before the
+    // platform window is destroyed.
+    mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
+    frame_sink_manager_->DestroyCompositorFrameSink(frame_sink_id);
+
+    // Other synchronous IPCs continue to get processed while
+    // DestroyCompositorFrameSink() is happening, so it's possible
+    // HostFrameSinkManager has been mutated. |data| might not be a valid
+    // reference at this point.
+  }
+
+  frame_sink_manager_->InvalidateFrameSinkId(frame_sink_id);
 }
 
 void HostFrameSinkManager::EnableSynchronizationReporting(

components/viz/host/host_frame_sink_manager.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #include "base/compiler_specific.h"
@@ -74,8 +75,12 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   // Sets a callback to be notified after Viz sent bad message to Viz host.
   void SetBadMessageReceivedFromGpuCallback(base::RepeatingClosure callback);
 
-  // Registers |frame_sink_id| will be used. This must be called before
-  // CreateCompositorFrameSink(Support) is called.
+  // Registers |frame_sink_id| so that a client can submit CompositorFrames
+  // using it. This must be called before creating a CompositorFrameSink or
+  // registering FrameSinkId hierarchy.
+  //
+  // When the client is done submitting CompositorFrames to |frame_sink_id| then
+  // InvalidateFrameSink() should be called.
   void RegisterFrameSinkId(const FrameSinkId& frame_sink_id,
                            HostFrameSinkClient* client,
                            ReportFirstSurfaceActivation report_activation);
@@ -84,10 +89,15 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   // InvalidateFrameSinkId() has not been called.
   bool IsFrameSinkIdRegistered(const FrameSinkId& frame_sink_id) const;
 
-  // Invalidates |frame_sink_id| which cleans up any dangling temporary
-  // references assigned to it. If there is a CompositorFrameSink for
-  // |frame_sink_id| then it will be destroyed and the message pipe to the
-  // client will be closed.
+  // Invalidates |frame_sink_id| when the client is done submitting
+  // CompositorFrames. If there is a CompositorFrameSink for |frame_sink_id|
+  // then it will be destroyed and the message pipe to the client will be
+  // closed.
+  //
+  // It's expected, but not enforced, that RegisterFrameSinkId() will never be
+  // called for |frame_sink_id| again. This is to avoid problems with re-entrant
+  // code. If the same client wants to submit CompositorFrames later a new
+  // FrameSinkId should be allocated.
   void InvalidateFrameSinkId(const FrameSinkId& frame_sink_id);
 
   // Tells FrameSinkManger to report when a synchronization event completes via
@@ -271,7 +281,8 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   FrameSinkManagerImpl* frame_sink_manager_impl_ = nullptr;
 
   // Per CompositorFrameSink data.
-  base::flat_map<FrameSinkId, FrameSinkData> frame_sink_data_map_;
+  std::unordered_map<FrameSinkId, FrameSinkData, FrameSinkIdHash>
+      frame_sink_data_map_;
 
   // If |frame_sink_manager_ptr_| connection was lost.
   bool connection_was_lost_ = false;