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 38 matching lines @@
     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() {
+  DCHECK(pending_replies_.empty());
 }
 
 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);
+  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());
 }
 
 GinJavaBoundObject::ObjectID GinJavaBridgeDispatcherHost::AddObject(
     const base::android::JavaRef<jobject>& object,
     const base::android::JavaRef<jclass>& safe_annotation_clazz,
     bool is_named,
     RenderFrameHost* holder) {
@@ skipping 264 matching lines @@
     RenderFrameHost* render_frame_host) {
   scoped_refptr<IsValidRenderFrameHostHelper> helper =
       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) {

[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(
     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;
   }
+  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);
 }
 
 void GinJavaBridgeDispatcherHost::OnHasMethod(
     RenderFrameHost* render_frame_host,
     GinJavaBoundObject::ObjectID object_id,
     const std::string& method_name,
     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;
   }
+  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);
 }
 
 void GinJavaBridgeDispatcherHost::OnInvokeMethod(
     RenderFrameHost* render_frame_host,
     GinJavaBoundObject::ObjectID object_id,
     const std::string& method_name,
     const base::ListValue& arguments,
     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;
     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;
   }
+  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))
               .PassAs<GinJavaMethodInvocationHelper::ObjectDelegate>(),
           method_name,
           arguments);
   result->Init(this);
   g_background_thread.Get()
       .message_loop()
       ->message_loop_proxy()
       ->PostTaskAndReply(
           FROM_HERE,
           base::Bind(&GinJavaMethodInvocationHelper::Invoke, result),
           base::Bind(
               &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;
     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());
+        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(
     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);
   }
 }
 
+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