Cache pending JS bridge sync IPC replies, and send in case of RenderFrame deletion.

content/browser/android/java/gin_java_bridge_dispatcher_host.cc

Index: content/browser/android/java/gin_java_bridge_dispatcher_host.cc
diff --git a/content/browser/android/java/gin_java_bridge_dispatcher_host.cc b/content/browser/android/java/gin_java_bridge_dispatcher_host.cc
index 74ccaa21aaf6b0352edd3d971c6d1630524292ae..89fe7ee35b31d08ef9ad4a39c245b74f2031066c 100644
--- a/content/browser/android/java/gin_java_bridge_dispatcher_host.cc
+++ b/content/browser/android/java/gin_java_bridge_dispatcher_host.cc
@@ -56,6 +56,7 @@ GinJavaBridgeDispatcherHost::GinJavaBridgeDispatcherHost(
 }
 
 GinJavaBridgeDispatcherHost::~GinJavaBridgeDispatcherHost() {
+  DCHECK(pending_replies_.empty());
 }
 
 void GinJavaBridgeDispatcherHost::RenderFrameCreated(
@@ -70,6 +71,18 @@ void GinJavaBridgeDispatcherHost::RenderFrameCreated(
 
 void GinJavaBridgeDispatcherHost::RenderFrameDeleted(
     RenderFrameHost* render_frame_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  PendingReplyMap::const_iterator it =
+      pending_replies_.find(render_frame_host);
+  if (it != pending_replies_.end()) {
+    IPC::Message* reply_msg = it->second;
+    base::ListValue result;
+    result.Append(base::Value::CreateNullValue());
+    IPC::WriteParam(reply_msg, result);
+    IPC::WriteParam(reply_msg, kGinJavaBridgeRenderFrameDeleted);
+    render_frame_host->Send(reply_msg);
+    pending_replies_.erase(render_frame_host);
+  }
   RemoveHolder(render_frame_host,
                GinJavaBoundObject::ObjectMap::iterator(&objects_),
                objects_.size());
@@ -356,11 +369,10 @@ void GinJavaBridgeDispatcherHost::SendReply(
     RenderFrameHost* render_frame_host,
     IPC::Message* reply_msg) {

[boliu, 2014/08/27 19:07:42]

Still don't like this one.

I think the erase should be moved to callers.

Also erase should come before Send which deletes the message, so
|pending_replies_| holds pointers to real objects at all times.

   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (IsValidRenderFrameHost(render_frame_host)) {
-    render_frame_host->Send(reply_msg);
-  } else {
-    delete reply_msg;
-  }
+  DCHECK(IsValidRenderFrameHost(render_frame_host));
+
+  render_frame_host->Send(reply_msg);
+  pending_replies_.erase(render_frame_host);
 }
 
 void GinJavaBridgeDispatcherHost::OnGetMethods(
@@ -381,20 +393,24 @@ void GinJavaBridgeDispatcherHost::OnGetMethods(
     render_frame_host->Send(reply_msg);
     return;
   }
+  DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end());
+  pending_replies_[render_frame_host] = reply_msg;
   base::PostTaskAndReplyWithResult(
       g_background_thread.Get().message_loop()->message_loop_proxy(),
       FROM_HERE,
       base::Bind(&GinJavaBoundObject::GetMethodNames, object),
       base::Bind(&GinJavaBridgeDispatcherHost::SendMethods,
                  AsWeakPtr(),
-                 render_frame_host,
-                 reply_msg));
+                 render_frame_host));
 }
 
 void GinJavaBridgeDispatcherHost::SendMethods(
     RenderFrameHost* render_frame_host,
-    IPC::Message* reply_msg,
     const std::set<std::string>& method_names) {
+  IPC::Message* reply_msg = GetPendingReply(render_frame_host);
+  if (!reply_msg) {
+    return;
+  }
   IPC::WriteParam(reply_msg, method_names);
   SendReply(render_frame_host, reply_msg);
 }
@@ -413,20 +429,24 @@ void GinJavaBridgeDispatcherHost::OnHasMethod(
     render_frame_host->Send(reply_msg);
     return;
   }
+  DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end());
+  pending_replies_[render_frame_host] = reply_msg;
   base::PostTaskAndReplyWithResult(
       g_background_thread.Get().message_loop()->message_loop_proxy(),
       FROM_HERE,
       base::Bind(&GinJavaBoundObject::HasMethod, object, method_name),
       base::Bind(&GinJavaBridgeDispatcherHost::SendHasMethodReply,
                  AsWeakPtr(),
-                 render_frame_host,
-                 reply_msg));
+                 render_frame_host));
 }
 
 void GinJavaBridgeDispatcherHost::SendHasMethodReply(
     RenderFrameHost* render_frame_host,
-    IPC::Message* reply_msg,
     bool result) {
+  IPC::Message* reply_msg = GetPendingReply(render_frame_host);
+  if (!reply_msg) {
+    return;
+  }
   IPC::WriteParam(reply_msg, result);
   SendReply(render_frame_host, reply_msg);
 }
@@ -449,6 +469,8 @@ void GinJavaBridgeDispatcherHost::OnInvokeMethod(
     render_frame_host->Send(reply_msg);
     return;
   }
+  DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end());
+  pending_replies_[render_frame_host] = reply_msg;
   scoped_refptr<GinJavaMethodInvocationHelper> result =
       new GinJavaMethodInvocationHelper(
           make_scoped_ptr(new GinJavaBoundObjectDelegate(object))
@@ -466,32 +488,37 @@ void GinJavaBridgeDispatcherHost::OnInvokeMethod(
               &GinJavaBridgeDispatcherHost::ProcessMethodInvocationResult,
               AsWeakPtr(),
               render_frame_host,
-              reply_msg,
               result));
 }
 
 void GinJavaBridgeDispatcherHost::ProcessMethodInvocationResult(
     RenderFrameHost* render_frame_host,
-    IPC::Message* reply_msg,
     scoped_refptr<GinJavaMethodInvocationHelper> result) {
   if (result->HoldsPrimitiveResult()) {
+    IPC::Message* reply_msg = GetPendingReply(render_frame_host);
+    if (!reply_msg) {
+      return;
+    }
     IPC::WriteParam(reply_msg, result->GetPrimitiveResult());
     IPC::WriteParam(reply_msg, result->GetInvocationError());
     SendReply(render_frame_host, reply_msg);
   } else {
-    ProcessMethodInvocationObjectResult(render_frame_host, reply_msg, result);
+    ProcessMethodInvocationObjectResult(render_frame_host, result);
   }
 }
 
 void GinJavaBridgeDispatcherHost::ProcessMethodInvocationObjectResult(
     RenderFrameHost* render_frame_host,
-    IPC::Message* reply_msg,
     scoped_refptr<GinJavaMethodInvocationHelper> result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
   if (!IsValidRenderFrameHost(render_frame_host)) {
-    delete reply_msg;
+    // In this case, we must've already sent the reply when the render frame
+    // was destroyed.
+    DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end());
     return;
   }
+
   base::ListValue wrapped_result;
   if (!result->GetObjectResult().is_null()) {
     GinJavaBoundObject::ObjectID returned_object_id;
@@ -504,13 +531,18 @@ void GinJavaBridgeDispatcherHost::ProcessMethodInvocationObjectResult(
                                      render_frame_host);
     }
     wrapped_result.Append(
-        GinJavaBridgeValue::CreateObjectIDValue(returned_object_id).release());
+        GinJavaBridgeValue::CreateObjectIDValue(
+            returned_object_id).release());
   } else {
     wrapped_result.Append(base::Value::CreateNullValue());
   }
+  IPC::Message* reply_msg = GetPendingReply(render_frame_host);
+  if (!reply_msg) {
+    return;
+  }
   IPC::WriteParam(reply_msg, wrapped_result);
   IPC::WriteParam(reply_msg, result->GetInvocationError());
-  render_frame_host->Send(reply_msg);
+  SendReply(render_frame_host, reply_msg);
 }
 
 void GinJavaBridgeDispatcherHost::OnObjectWrapperDeleted(
@@ -527,4 +559,15 @@ void GinJavaBridgeDispatcherHost::OnObjectWrapperDeleted(
   }
 }
 
+IPC::Message* GinJavaBridgeDispatcherHost::GetPendingReply(

[boliu, 2014/08/27 19:07:42]

I think this should be split.

Have a bool HasPendingReply(...) const that can be used in DCHECKs etc.

Then also have a IPC::Message* TakePendingReply which DCHECKs it's in the map,
and also does the erase before it returns.

Works better with the above suggestion as well.

+    RenderFrameHost* render_frame_host) {
+  PendingReplyMap::const_iterator it = pending_replies_.find(render_frame_host);
+  if (!IsValidRenderFrameHost(render_frame_host)) {
+    DCHECK(it == pending_replies_.end());
+    return NULL;
+  }
+  DCHECK(it != pending_replies_.end());
+  return it->second;
+}
+
 }  // namespace content