Fix a race condition in the Java Bridge when adding objects

content/browser/renderer_host/java/java_bridge_dispatcher_host.cc

Index: content/browser/renderer_host/java/java_bridge_dispatcher_host.cc
diff --git a/content/browser/renderer_host/java/java_bridge_dispatcher_host.cc b/content/browser/renderer_host/java/java_bridge_dispatcher_host.cc
index b4573054e7561eb64abcd2ad3729dc0fdbdc11bd..b923fc2c7ff8aeabae8ed0b7304951b7182bac8c 100644
--- a/content/browser/renderer_host/java/java_bridge_dispatcher_host.cc
+++ b/content/browser/renderer_host/java/java_bridge_dispatcher_host.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/renderer_host/java/java_bridge_dispatcher_host.h"
 
+#include "base/bind.h"
 #include "content/browser/renderer_host/browser_render_process_host.h"
 #include "content/browser/renderer_host/java/java_bridge_channel_host.h"
 #include "content/browser/renderer_host/render_view_host.h"
@@ -18,8 +19,8 @@ using content::BrowserThread;
 
 JavaBridgeDispatcherHost::JavaBridgeDispatcherHost(
     RenderViewHost* render_view_host)
-    : render_view_host_(render_view_host) {
-  DCHECK(render_view_host_);
+    : RenderViewHostObserver(render_view_host),
+      is_renderer_initialized_(false) {
 }
 
 JavaBridgeDispatcherHost::~JavaBridgeDispatcherHost() {
@@ -27,16 +28,14 @@ JavaBridgeDispatcherHost::~JavaBridgeDispatcherHost() {
 
 void JavaBridgeDispatcherHost::AddNamedObject(const string16& name,
                                               NPObject* object) {
-  // Post to the WEBKIT thread, as that's where we want injected objects to
-  // live.
-  WebKit::WebBindings::retainObject(object);
-  BrowserThread::PostTask(
-      BrowserThread::WEBKIT,
-      FROM_HERE,
-      NewRunnableMethod(this,
-                        &JavaBridgeDispatcherHost::DoAddNamedObject,
-                        name,
-                        object));
+  NPVariant_Param variant_param;
+  CreateNPVariantParam(object, &variant_param);
+
+  if (!is_renderer_initialized_) {
+    is_renderer_initialized_ = true;
+    Send(new JavaBridgeMsg_Init(routing_id()));
+  }
+  Send(new JavaBridgeMsg_AddNamedObject(routing_id(), name, variant_param));
 }
 
 void JavaBridgeDispatcherHost::RemoveNamedObject(const string16& name) {
@@ -45,47 +44,86 @@ void JavaBridgeDispatcherHost::RemoveNamedObject(const string16& name) {
   // removed, the proxy object will delete its NPObjectProxy, which will cause
   // the NPObjectStub to be deleted, which will drop its reference to the
   // original NPObject.
-  render_view_host_->Send(new JavaBridgeMsg_RemoveNamedObject(
-      render_view_host_->routing_id(), name));
+  Send(new JavaBridgeMsg_RemoveNamedObject(routing_id(), name));
 }
 
-void JavaBridgeDispatcherHost::DoAddNamedObject(const string16& name,
-                                                NPObject* object) {
-  EnsureChannelIsSetUp();
-
-  NPVariant variant;
-  OBJECT_TO_NPVARIANT(object, variant);
-  DCHECK_EQ(variant.type, NPVariantType_Object);
-
-  // Create an NPVariantParam suitable for serialization over IPC from our
-  // NPVariant. Internally, this will create an NPObjectStub, which takes a
-  // reference to the underlying NPObject. We pass release = true to release
-  // the ref added in AddNamedObject(). We don't need the containing window or
-  // the page URL, as we don't do re-entrant sync IPC.
-  NPVariant_Param variant_param;
-  CreateNPVariantParam(variant, channel_, &variant_param, true, 0, GURL());
-  DCHECK_EQ(variant_param.type, NPVARIANT_PARAM_SENDER_OBJECT_ROUTING_ID);
+bool JavaBridgeDispatcherHost::Send(IPC::Message* msg) {
+  return RenderViewHostObserver::Send(msg);
+}
 
-  render_view_host_->Send(new JavaBridgeMsg_AddNamedObject(
-      render_view_host_->routing_id(), name, variant_param));
+bool JavaBridgeDispatcherHost::OnMessageReceived(const IPC::Message& msg) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(JavaBridgeDispatcherHost, msg)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(JavaBridgeHostMsg_GetChannelHandle,
+                                    OnGetChannelHandle)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
 }
 
-void JavaBridgeDispatcherHost::EnsureChannelIsSetUp() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
-  if (channel_) {
-    return;
+void JavaBridgeDispatcherHost::OnGetChannelHandle(IPC::Message* reply_msg) {
+  // In renderer-in-process mode, this sync IPC is coming from the WEBKIT
+  // thread, so we can't post to the WEBKIT thread.
+  if (RenderProcessHost::run_renderer_in_process()) {

[Steve Block, 2011/12/08 13:46:04]

Given the ongoing discussion about renderer-in-process mode in
http://code.google.com/p/chromium/issues/detail?id=106839, I could remove this
and replace with a TODO referencing
http://code.google.com/p/chromium/issues/detail?id=106838. This would allow this
patch to be landed without waiting for resolution on renderer-in-process mode.

[joth, 2011/12/08 18:15:46]

I don't mind either way, given
http://codereview.chromium.org/8890018/diff/1/content/browser/renderer_host/j...
will land next anyway

[Steve Block, 2011/12/09 12:27:32]

Added a TODO.

+    GetChannelHandle(reply_msg);
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::WEBKIT,
+        FROM_HERE,
+        base::Bind(&JavaBridgeDispatcherHost::GetChannelHandle, this,
+                   reply_msg));
   }
+}
 
-  channel_ = JavaBridgeChannelHost::GetJavaBridgeChannelHost(
-      render_view_host_->process()->id(),
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
-
+void JavaBridgeDispatcherHost::GetChannelHandle(IPC::Message* reply_msg) {
   // The channel creates the channel handle based on the renderer ID we passed
   // to GetJavaBridgeChannelHost() and, on POSIX, the file descriptor used by
   // the underlying channel.
-  IPC::ChannelHandle channel_handle = channel_->channel_handle();
+  JavaBridgeHostMsg_GetChannelHandle::WriteReplyParams(
+      reply_msg,
+      channel_->channel_handle());
+  Send(reply_msg);
+}
+
+void JavaBridgeDispatcherHost::CreateNPVariantParam(NPObject* object,
+                                                    NPVariant_Param* param) {
+  // The JavaBridgeChannelHost needs to be created on the WEBKIT thread, as
+  // that is where Java objects will live, and CreateNPVariantParam() needs the
+  // channel to create the NPObjectStub. To avoid blocking here until the
+  // channel is ready, create the NPVariant_Param by hand, then post a message
+  // to the WEBKIT thread to set up the channel and create the corresponding
+  // NPObjectStub. Post that message before doing any IPC, to make sure that
+  // the channel and object proxies are ready before responses are received
+  // from the renderer.
+
+  // Create an NPVariantParam suitable for serialization over IPC from our
+  // NPVariant. See CreateNPVariantParam() in npobject_utils.
+  param->type = NPVARIANT_PARAM_SENDER_OBJECT_ROUTING_ID;
+  int route_id = JavaBridgeChannelHost::ThreadsafeGenerateRouteID();
+  param->npobject_routing_id = route_id;
+
+  WebKit::WebBindings::retainObject(object);
+  BrowserThread::PostTask(
+      BrowserThread::WEBKIT,
+      FROM_HERE,
+      base::Bind(&JavaBridgeDispatcherHost::CreateObjectStub, this, object,
+                 route_id));
+}
+
+void JavaBridgeDispatcherHost::CreateObjectStub(NPObject* object,
+                                                int route_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
+
+  if (!channel_) {
+    channel_ = JavaBridgeChannelHost::GetJavaBridgeChannelHost(
+        render_view_host()->process()->id(),
+        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
+  }
 
-  bool sent = render_view_host_->Send(new JavaBridgeMsg_Init(
-      render_view_host_->routing_id(), channel_handle));
-  DCHECK(sent);
+  // We don't need the containing window or the page URL, as we don't do
+  // re-entrant sync IPC.
+  new NPObjectStub(object, channel_, route_id, 0, GURL());
+  // The NPObjectStub takes a reference to the NPObject. Release the ref added
+  // in CreateNPVariantParam().
+  WebKit::WebBindings::releaseObject(object);
 }