Fix another unsafe base::Unretained() in RenderProcessHostImpl.

This CL converts an unsafe base::Unretained() to target the
MediaStreamTrackMetricsHost object directly which is deleted on the
same thread as the callback.  This comes at the cost of pre-allocating
the object instead of lazily creating it when the first receiver is
bound.  This cost does not seem too great since the object consists of
an empty std::map and mojo::ReceiverSet.

In addition, the CL attempts to document why the remaining
base::Unretained() usage is safe.

Bug: 1040396
Change-Id: I094091c4672c412086059c56804efb3d743a2c11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2005291
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732710}

content/browser/renderer_host/render_process_host_impl.cc

@@ -1934,6 +1934,8 @@ void RenderProcessHostImpl::BindFileSystemManager(
     const url::Origin& origin,
     mojo::PendingReceiver<blink::mojom::FileSystemManager> receiver) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Note, the base::Unretained() is safe because the target object has an IO
+  // thread deleter and the callback is also targeting the IO thread.
   // TODO(https://crbug.com/873661): Pass origin to FileSystemManager.
   base::PostTask(
       FROM_HERE, {BrowserThread::IO},
@@ -2216,6 +2218,10 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
         base::BindRepeating(&RenderProcessHostImpl::BindPushMessagingManager,
                             weak_factory_.GetWeakPtr()));
   } else {
+    // Note, the base::Unretained() is safe because the target object has an IO
+    // thread deleter and the callback is also targeting the IO thread.  When
+    // the RPHI is destroyed it also triggers the destruction of the registry
+    // on the IO thread.
     registry->AddInterface(
         base::BindRepeating(&PushMessagingManager::AddPushMessagingReceiver,
                             base::Unretained(push_messaging_manager_.get())));
@@ -2227,6 +2233,12 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
   // This interface is still exposed by the RenderProcessHost's registry so
   // that it can be accessed by PepperFileSystemHost. Blink accesses this
   // interface through RenderFrameHost/RendererInterfaceBinders.
+  //
+  // Note, the base::Unretained() is safe because the target object has an IO
+  // thread deleter and the callback is also targeting the IO thread.  When
+  // the RPHI is destroyed it also triggers the destruction of the registry
+  // on the IO thread.
+  //
   // TODO(https://crbug.com/873661): Make PepperFileSystemHost access this with
   // the RenderFrameHost's registry, and remove this registration.
   registry->AddInterface(
@@ -2255,9 +2267,14 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
       base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
                                        base::TaskPriority::USER_VISIBLE}));
 
+  // Note, the base::Unretained() is safe because the target object has an IO
+  // thread deleter and the callback is also targeting the IO thread.  When
+  // the RPHI is destroyed it also triggers the destruction of the registry
+  // on the IO thread.
+  media_stream_track_metrics_host_.reset(new MediaStreamTrackMetricsHost());
   registry->AddInterface(base::BindRepeating(
-      &RenderProcessHostImpl::CreateMediaStreamTrackMetricsHost,
-      base::Unretained(this)));
+      &MediaStreamTrackMetricsHost::BindReceiver,
+      base::Unretained(media_stream_track_metrics_host_.get())));
 
   AddUIThreadInterface(
       registry.get(),
@@ -2303,6 +2320,9 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
       std::make_unique<blink::AssociatedInterfaceRegistry>();
   blink::AssociatedInterfaceRegistry* associated_registry =
       associated_interfaces_.get();
+
+  // This base::Unretained() usage is safe since the associated_registry is
+  // owned by this RPHI.
   associated_registry->AddInterface(base::BindRepeating(
       &RenderProcessHostImpl::BindRouteProvider, base::Unretained(this)));
   associated_registry->AddInterface(base::BindRepeating(
@@ -4800,14 +4820,6 @@ RenderProcessHostImpl::FindReusableProcessHostForSiteInstance(
   return nullptr;
 }
 
-void RenderProcessHostImpl::CreateMediaStreamTrackMetricsHost(
-    mojo::PendingReceiver<blink::mojom::MediaStreamTrackMetricsHost> receiver) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (!media_stream_track_metrics_host_)
-    media_stream_track_metrics_host_.reset(new MediaStreamTrackMetricsHost());
-  media_stream_track_metrics_host_->BindReceiver(std::move(receiver));
-}
-
 void RenderProcessHostImpl::CreateAgentMetricsCollectorHost(
     mojo::PendingReceiver<blink::mojom::AgentMetricsCollectorHost> receiver) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

content/browser/renderer_host/render_process_host_impl.h

@@ -830,10 +830,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
   static RenderProcessHost* FindReusableProcessHostForSiteInstance(
       SiteInstanceImpl* site_instance);
 
-  void CreateMediaStreamTrackMetricsHost(
-      mojo::PendingReceiver<blink::mojom::MediaStreamTrackMetricsHost>
-          receiver);
-
   void CreateAgentMetricsCollectorHost(
       mojo::PendingReceiver<blink::mojom::AgentMetricsCollectorHost> receiver);
 
@@ -906,6 +902,18 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // if the request isn't handled on the IO thread.
   void OnBindHostReceiver(mojo::GenericPendingReceiver receiver);
 
+  // IOThreadHostImpl owns some IO-thread state associated with this
+  // RenderProcessHostImpl. This is mainly to allow various IPCs from the
+  // renderer to be handled on the IO thread without a hop to the UI thread.
+  //
+  // Declare this early to ensure it triggers the destruction of the
+  // IOThreadHostImpl prior to other objects with an IO thread deleter.  This
+  // is necessary to ensure those objects stop receiving mojo messages before
+  // their destruction.
+  class IOThreadHostImpl;
+  friend class IOThreadHostImpl;
+  base::Optional<base::SequenceBound<IOThreadHostImpl>> io_thread_host_impl_;
+
   mojo::OutgoingInvitation mojo_invitation_;
 
   size_t keep_alive_ref_count_;
@@ -1119,13 +1127,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
 
   IpcSendWatcher ipc_send_watcher_for_testing_;
 
-  // IOThreadHostImpl owns some IO-thread state associated with this
-  // RenderProcessHostImpl. This is mainly to allow various IPCs from the
-  // renderer to be handled on the IO thread without a hop to the UI thread.
-  class IOThreadHostImpl;
-  friend class IOThreadHostImpl;
-  base::Optional<base::SequenceBound<IOThreadHostImpl>> io_thread_host_impl_;
-
   // Keeps this process registered with the tracing subsystem.
   std::unique_ptr<TracingServiceController::ClientRegistration>
       tracing_registration_;