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

content/browser/android/java/gin_java_bridge_dispatcher_host.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/android/java/gin_java_bridge_dispatcher_host.h"
 
 #include "base/android/java_handler_thread.h"
 #include "base/android/jni_android.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/lazy_instance.h"
@@ skipping 37 matching lines @@
 GinJavaBridgeDispatcherHost::GinJavaBridgeDispatcherHost(
     WebContents* web_contents,
     jobject retained_object_set)
     : WebContentsObserver(web_contents),
       retained_object_set_(base::android::AttachCurrentThread(),
                            retained_object_set),
       allow_object_contents_inspection_(true) {
   DCHECK(retained_object_set);
 }
 
 GinJavaBridgeDispatcherHost::~GinJavaBridgeDispatcherHost() {

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

Is pending_replies_ guaranteed to be empty here? If so DCHECK, if not, should
wake up those render frames like in RenderFrameDeleted

 }
 
 void GinJavaBridgeDispatcherHost::RenderFrameCreated(
     RenderFrameHost* render_frame_host) {
   for (NamedObjectMap::const_iterator iter = named_objects_.begin();
        iter != named_objects_.end();
        ++iter) {
     render_frame_host->Send(new GinJavaBridgeMsg_AddNamedObject(
         render_frame_host->GetRoutingID(), iter->first, iter->second));
   }
 }
 
 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());
 }
 
 GinJavaBoundObject::ObjectID GinJavaBridgeDispatcherHost::AddObject(
     const base::android::JavaRef<jobject>& object,
     const base::android::JavaRef<jclass>& safe_annotation_clazz,
     bool is_named,
     RenderFrameHost* holder) {
@@ skipping 266 matching lines @@
       new IsValidRenderFrameHostHelper(render_frame_host);
   web_contents()->ForEachFrame(
       base::Bind(&IsValidRenderFrameHostHelper::OnFrame, helper));
   return helper->rfh_found();
 }
 
 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());
   }
 }
 
 void GinJavaBridgeDispatcherHost::OnGetMethods(
     RenderFrameHost* render_frame_host,
     GinJavaBoundObject::ObjectID object_id,
     IPC::Message* reply_msg) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(render_frame_host);
   if (!allow_object_contents_inspection_) {
     IPC::WriteParam(reply_msg, std::set<std::string>());
     render_frame_host->Send(reply_msg);
     return;
   }
   scoped_refptr<GinJavaBoundObject> object(*objects_.Lookup(object_id));
   if (!object) {
     LOG(ERROR) << "WebView: Unknown object: " << object_id;
     IPC::WriteParam(reply_msg, std::set<std::string>());
     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,
       base::Bind(&GinJavaBoundObject::GetMethodNames, object),
       base::Bind(&GinJavaBridgeDispatcherHost::SendMethods,
                  AsWeakPtr(),
                  render_frame_host,
                  reply_msg));
 }
 
@@ skipping 12 matching lines @@
     IPC::Message* reply_msg) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(render_frame_host);
   scoped_refptr<GinJavaBoundObject> object(*objects_.Lookup(object_id));
   if (!object) {
     LOG(ERROR) << "WebView: Unknown object: " << object_id;
     IPC::WriteParam(reply_msg, false);
     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,
       base::Bind(&GinJavaBoundObject::HasMethod, object, method_name),
       base::Bind(&GinJavaBridgeDispatcherHost::SendHasMethodReply,
                  AsWeakPtr(),
                  render_frame_host,
                  reply_msg));
 }
 
@@ skipping 16 matching lines @@
   scoped_refptr<GinJavaBoundObject> object(*objects_.Lookup(object_id));
   if (!object) {
     LOG(ERROR) << "WebView: Unknown object: " << object_id;
     base::ListValue result;
     result.Append(base::Value::CreateNullValue());
     IPC::WriteParam(reply_msg, result);
     IPC::WriteParam(reply_msg, kGinJavaBridgeUnknownObjectId);
     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))
               .PassAs<GinJavaMethodInvocationHelper::ObjectDelegate>(),
           method_name,
           arguments);
   result->Init(this);
   g_background_thread.Get()
       .message_loop()
       ->message_loop_proxy()
@@ skipping 18 matching lines @@
     SendReply(render_frame_host, reply_msg);
   } else {
     ProcessMethodInvocationObjectResult(render_frame_host, reply_msg, result);
   }
 }
 
 void GinJavaBridgeDispatcherHost::ProcessMethodInvocationObjectResult(
     RenderFrameHost* render_frame_host,
     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;
+
+  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 {
+      wrapped_result.Append(base::Value::CreateNullValue());
+    }
+    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 {
+    // 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());
   }
-  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 {
-    wrapped_result.Append(base::Value::CreateNullValue());
-  }
-  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(
     RenderFrameHost* render_frame_host,
     GinJavaBoundObject::ObjectID object_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(render_frame_host);
   if (objects_.Lookup(object_id)) {
     GinJavaBoundObject::ObjectMap::iterator iter(&objects_);
     while (!iter.IsAtEnd() && iter.GetCurrentKey() != object_id)
       iter.Advance();
     DCHECK(!iter.IsAtEnd());
     RemoveHolder(render_frame_host, iter, 1);
   }
 }
 
 }  // namespace content