Ensure RenderFrameHostImpl's RenderFrameSetupPtr doesn't die too early

RenderFrameHostImpl is creating a RenderFrameSetupPtr, binding it via
ConnectToRemoteService(), and then using it to bootstrap the per-frame
Mojo connection. However, it holds this instance in a local variable,
meaning that the pipe is closed when the local variable goes out of
scope. The closing of the pipe races with the processing of the message
write that is the result of calling
RenderFrameSetupPtr->GetServiceProviderForFrame(), as that processing occurs
on a different thread. The net outcome is that the per-frame Mojo
connection will flakily fail to be set up.

To eliminate this race, this CL changes the RenderFrameSetupPtr instance
that the RFHI holds from a local variable to a member variable.

BUG=424069
TEST=Visit about://omnibox in multiple tabs. In each tab, check that submitting
text causes output to appear.

Review URL: https://codereview.chromium.org/667683002

Cr-Commit-Position: refs/heads/master@{#300864}

2 files changed, 13 insertions, 5 deletions

diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index e2bf47b..c1c1427 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -40,7 +40,6 @@
 #include "content/common/inter_process_time_ticks_converter.h"
 #include "content/common/navigation_params.h"
 #include "content/common/platform_notification_messages.h"
-#include "content/common/render_frame_setup.mojom.h"
 #include "content/common/swapped_out_messages.h"
 #include "content/public/browser/ax_event_notification_details.h"
 #include "content/public/browser/browser_accessibility_state.h"
@@ -204,11 +203,11 @@ RenderFrameHostImpl::RenderFrameHostImpl(RenderViewHostImpl* render_view_host,
   }
 
   if (GetProcess()->GetServiceRegistry()) {
-    RenderFrameSetupPtr setup;
-    GetProcess()->GetServiceRegistry()->ConnectToRemoteService(&setup);
+    GetProcess()->GetServiceRegistry()->ConnectToRemoteService(
+        &render_frame_setup_);
     mojo::ServiceProviderPtr service_provider;
-    setup->GetServiceProviderForFrame(routing_id_,
-                                      mojo::GetProxy(&service_provider));
+    render_frame_setup_->GetServiceProviderForFrame(
+        routing_id_, mojo::GetProxy(&service_provider));
     service_registry_.BindRemoteServiceProvider(
         service_provider.PassMessagePipe());
 
diff --git a/content/browser/frame_host/render_frame_host_impl.h b/content/browser/frame_host/render_frame_host_impl.h
index 3fcb8e2..dbce027 100644
--- a/content/browser/frame_host/render_frame_host_impl.h
+++ b/content/browser/frame_host/render_frame_host_impl.h
@@ -19,6 +19,7 @@
 #include "content/common/accessibility_mode_enums.h"
 #include "content/common/content_export.h"
 #include "content/common/mojo/service_registry_impl.h"
+#include "content/common/render_frame_setup.mojom.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/common/javascript_message_type.h"
 #include "net/http/http_response_headers.h"
@@ -565,6 +566,14 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // response once it has started.
   scoped_ptr<StreamHandle> stream_handle_;
 
+  // Holds the browser-side handle to the pipe used to establish the Mojo
+  // connection between this instance and its associated render frame.
+  // TODO(blundell): Change this back to being a local variable if/once Mojo
+  // is changed to guarantee that message writes that are pending when the proxy
+  // pointer dies are delivered. See the discussion on the mojo-dev@ thread
+  // "Advice request to track down a flaky dropped message".
+  RenderFrameSetupPtr render_frame_setup_;
+
   // NOTE: This must be the last member.
   base::WeakPtrFactory<RenderFrameHostImpl> weak_ptr_factory_;