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..57f37357b386742fdd715c3966ffd4337440a459 100644
--- a/content/browser/android/java/gin_java_bridge_dispatcher_host.cc
+++ b/content/browser/android/java/gin_java_bridge_dispatcher_host.cc
@@ -70,6 +70,18 @@ void GinJavaBridgeDispatcherHost::RenderFrameCreated(
 
 void GinJavaBridgeDispatcherHost::RenderFrameDeleted(
     RenderFrameHost* render_frame_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  std::map<RenderFrameHost*, IPC::Message*>::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, kGinJavaBridgeObjectIsGone);

[mnaganov (inactive), 2014/08/24 13:31:49]

Maybe, add a specific error code / message for this case?

[benm (inactive), 2014/08/26 15:42:40]

Done.

+    render_frame_host->Send(reply_msg);
+    pending_replies_.erase(render_frame_host);
+  }
   RemoveHolder(render_frame_host,
                GinJavaBoundObject::ObjectMap::iterator(&objects_),
                objects_.size());
@@ -356,10 +368,15 @@ void GinJavaBridgeDispatcherHost::SendReply(
     RenderFrameHost* render_frame_host,
     IPC::Message* reply_msg) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
   if (IsValidRenderFrameHost(render_frame_host)) {
+    DCHECK(pending_replies_.find(render_frame_host) != pending_replies_.end());
     render_frame_host->Send(reply_msg);
+    pending_replies_.erase(render_frame_host);
   } else {
-    delete reply_msg;
+    // In this case, we must've already sent the reply when the redner frame
+    // was destroyed.
+    DCHECK(pending_replies_.find(render_frame_host) == pending_replies_.end());
   }
 }
 
@@ -381,6 +398,7 @@ void GinJavaBridgeDispatcherHost::OnGetMethods(
     render_frame_host->Send(reply_msg);
     return;
   }
+  pending_replies_[render_frame_host] = reply_msg;

[boliu, 2014/08/22 21:05:17]

DCHECK(pending_replies_.find(render_frame_host) == end())

and below

[benm (inactive), 2014/08/26 15:42:40]

good call.

   base::PostTaskAndReplyWithResult(
       g_background_thread.Get().message_loop()->message_loop_proxy(),
       FROM_HERE,
@@ -413,6 +431,7 @@ void GinJavaBridgeDispatcherHost::OnHasMethod(
     render_frame_host->Send(reply_msg);
     return;
   }
+  pending_replies_[render_frame_host] = reply_msg;
   base::PostTaskAndReplyWithResult(
       g_background_thread.Get().message_loop()->message_loop_proxy(),
       FROM_HERE,
@@ -449,6 +468,7 @@ void GinJavaBridgeDispatcherHost::OnInvokeMethod(
     render_frame_host->Send(reply_msg);
     return;
   }
+  pending_replies_[render_frame_host] = reply_msg;
   scoped_refptr<GinJavaMethodInvocationHelper> result =
       new GinJavaMethodInvocationHelper(
           make_scoped_ptr(new GinJavaBoundObjectDelegate(object))
@@ -488,29 +508,33 @@ void GinJavaBridgeDispatcherHost::ProcessMethodInvocationObjectResult(
     IPC::Message* reply_msg,
     scoped_refptr<GinJavaMethodInvocationHelper> result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

[boliu, 2014/08/22 21:05:17]

nit: general chrome style is do the shorter branch first as an early out, then
no need to add indent, so...

if (!IsValidRenderFrameHost(render_frame_host)) {
  DCHECK(...);
  SendReply(...);
  return;
}

// RFH valid here.
  

[benm (inactive), 2014/08/26 15:42:40]

Done.

-  if (!IsValidRenderFrameHost(render_frame_host)) {
-    delete reply_msg;
-    return;
-  }
-  base::ListValue wrapped_result;
-  if (!result->GetObjectResult().is_null()) {
-    GinJavaBoundObject::ObjectID returned_object_id;
-    if (FindObjectId(result->GetObjectResult(), &returned_object_id)) {
-      (*objects_.Lookup(returned_object_id))->AddHolder(render_frame_host);
+
+  if (IsValidRenderFrameHost(render_frame_host)) {
+    base::ListValue wrapped_result;
+    if (!result->GetObjectResult().is_null()) {
+      GinJavaBoundObject::ObjectID returned_object_id;
+      if (FindObjectId(result->GetObjectResult(), &returned_object_id)) {
+        (*objects_.Lookup(returned_object_id))->AddHolder(render_frame_host);
+      } else {
+        returned_object_id = AddObject(result->GetObjectResult(),
+                                       result->GetSafeAnnotationClass(),
+                                       false,
+                                       render_frame_host);
+      }
+      wrapped_result.Append(
+          GinJavaBridgeValue::CreateObjectIDValue(
+              returned_object_id).release());
     } else {
-      returned_object_id = AddObject(result->GetObjectResult(),
-                                     result->GetSafeAnnotationClass(),
-                                     false,
-                                     render_frame_host);
+      wrapped_result.Append(base::Value::CreateNullValue());
     }
-    wrapped_result.Append(
-        GinJavaBridgeValue::CreateObjectIDValue(returned_object_id).release());
+    IPC::WriteParam(reply_msg, wrapped_result);
+    IPC::WriteParam(reply_msg, result->GetInvocationError());

[boliu, 2014/08/22 21:05:17]

Erase rfh from pending_replies_ in this branch, and also DCHECK it's there in
the map

[benm (inactive), 2014/08/26 15:42:40]

We do that in SendReply already.

   } else {
-    wrapped_result.Append(base::Value::CreateNullValue());
+    // 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());
   }
-  IPC::WriteParam(reply_msg, wrapped_result);
-  IPC::WriteParam(reply_msg, result->GetInvocationError());
-  render_frame_host->Send(reply_msg);
+  SendReply(render_frame_host, reply_msg);

[boliu, 2014/08/22 21:05:17]

SendReply will just delete the message if rfh is invalid, so that's not
different. I guess that's intended and you just wanted to merge the code paths?
I say don't bother, just add the DCHECK :D

[benm (inactive), 2014/08/26 15:42:40]

Hold up; we shouldn't get into the case now where deleting the message is
required: we should send it in all cases (either as a consequence of RFDeleted,
or the normal SendReply flow. But yeah, you're right we can just remove the
SendReply now I refactored the short path up tho the top.

 }
 
 void GinJavaBridgeDispatcherHost::OnObjectWrapperDeleted(