Fix assertion when calling SendProcessMessage from non-UI thread (issue #2325)

libcef/browser/browser_host_impl.cc

@@ -1461,17 +1461,23 @@ void CefBrowserHostImpl::GetFrameNames(std::vector<CefString>& names) {
 bool CefBrowserHostImpl::SendProcessMessage(
     CefProcessId target_process,
     CefRefPtr<CefProcessMessage> message) {
+  DCHECK_EQ(PID_RENDERER, target_process);
   DCHECK(message.get());
 
   Cef_Request_Params params;
   CefProcessMessageImpl* impl =
       static_cast<CefProcessMessageImpl*>(message.get());
-  if (impl->CopyTo(params)) {
+  if (!impl->CopyTo(params))
-    return SendProcessMessage(target_process, params.name, &params.arguments,
+    return false;
-                              true);
-  }
 
-  return false;
+  DCHECK(!params.name.empty());
+
+  params.frame_id = -1;
+  params.user_initiated = true;
+  params.request_id = -1;
+  params.expect_response = false;
+
+  return Send(new CefMsg_Request(MSG_ROUTING_NONE, params));
 }
 
 // CefBrowserHostImpl public methods.
@@ -1612,7 +1618,7 @@ void CefBrowserHostImpl::Navigate(const CefNavigateParams& params) {
   request.load_flags = params.load_flags;
   request.upload_data = params.upload_data;
 
-  Send(new CefMsg_LoadRequest(routing_id(), request));
+  Send(new CefMsg_LoadRequest(MSG_ROUTING_NONE, request));
 
   OnSetFocus(FOCUS_SOURCE_NAVIGATION);
 }
@@ -1692,7 +1698,7 @@ void CefBrowserHostImpl::LoadString(int64 frame_id,
   params.arguments.AppendString(string);
   params.arguments.AppendString(url);
 
-  Send(new CefMsg_Request(routing_id(), params));
+  Send(new CefMsg_Request(MSG_ROUTING_NONE, params));
 }
 
 void CefBrowserHostImpl::SendCommand(
@@ -1722,7 +1728,7 @@ void CefBrowserHostImpl::SendCommand(
 
     params.arguments.AppendString(command);
 
-    Send(new CefMsg_Request(routing_id(), params));
+    Send(new CefMsg_Request(MSG_ROUTING_NONE, params));
   } else {
     CEF_POST_TASK(CEF_UIT, base::Bind(&CefBrowserHostImpl::SendCommand, this,
                                       frame_id, command, responseHandler));
@@ -1763,7 +1769,7 @@ void CefBrowserHostImpl::SendCode(
     params.arguments.AppendString(script_url);
     params.arguments.AppendInteger(script_start_line);
 
-    Send(new CefMsg_Request(routing_id(), params));
+    Send(new CefMsg_Request(MSG_ROUTING_NONE, params));
   } else {
     CEF_POST_TASK(CEF_UIT, base::Bind(&CefBrowserHostImpl::SendCode, this,
                                       frame_id, is_javascript, code, script_url,
@@ -1800,25 +1806,6 @@ void CefBrowserHostImpl::ExecuteJavaScriptWithUserGestureForTests(
     rfh->ExecuteJavaScriptWithUserGestureForTests(javascript);
 }
 
-bool CefBrowserHostImpl::SendProcessMessage(CefProcessId target_process,
-                                            const std::string& name,
-                                            base::ListValue* arguments,
-                                            bool user_initiated) {
-  DCHECK_EQ(PID_RENDERER, target_process);
-  DCHECK(!name.empty());
-
-  Cef_Request_Params params;
-  params.name = name;
-  if (arguments)
-    params.arguments.Swap(arguments);
-  params.frame_id = -1;
-  params.user_initiated = user_initiated;
-  params.request_id = -1;
-  params.expect_response = false;
-
-  return Send(new CefMsg_Request(routing_id(), params));
-}
-
 void CefBrowserHostImpl::ViewText(const std::string& text) {
   if (!CEF_CURRENTLY_ON_UIT()) {
     CEF_POST_TASK(CEF_UIT,
@@ -2911,33 +2898,6 @@ void CefBrowserHostImpl::OnWebContentsFocused(
   }
 }
 
-bool CefBrowserHostImpl::Send(IPC::Message* message) {
-  if (CEF_CURRENTLY_ON_UIT()) {
-    if (queue_messages_) {
-      queued_messages_.push(message);
-      return true;
-    } else if (web_contents() && web_contents()->GetRenderViewHost()) {
-      return web_contents()->GetRenderViewHost()->Send(message);
-    } else {
-      delete message;
-      return false;
-    }
-  } else {
-    CEF_POST_TASK(
-        CEF_UIT, base::Bind(base::IgnoreResult(&CefBrowserHostImpl::Send), this,
-                            message));
-    return true;
-  }
-}
-
-int CefBrowserHostImpl::routing_id() const {
-  CEF_REQUIRE_UIT();
-  if (!web_contents())
-    return MSG_ROUTING_NONE;
-
-  return web_contents()->GetRenderViewHost()->GetRoutingID();
-}
-
 void CefBrowserHostImpl::AddObserver(Observer* observer) {
   CEF_REQUIRE_UIT();
   observers_.AddObserver(observer);
@@ -3032,6 +2992,8 @@ void CefBrowserHostImpl::OnUpdateDraggableRegions(
 }
 
 void CefBrowserHostImpl::OnRequest(const Cef_Request_Params& params) {
+  CEF_REQUIRE_UIT();
+
   bool success = false;
   std::string response;
   bool expect_response_ack = false;
@@ -3059,14 +3021,16 @@ void CefBrowserHostImpl::OnRequest(const Cef_Request_Params& params) {
     response_params.success = success;
     response_params.response = response;
     response_params.expect_response_ack = expect_response_ack;
-    Send(new CefMsg_Response(routing_id(), response_params));
+    Send(new CefMsg_Response(MSG_ROUTING_NONE, response_params));
   }
 }
 
 void CefBrowserHostImpl::OnResponse(const Cef_Response_Params& params) {
+  CEF_REQUIRE_UIT();
+
   response_manager_->RunHandler(params);
   if (params.expect_response_ack)
-    Send(new CefMsg_ResponseAck(routing_id(), params.request_id));
+    Send(new CefMsg_ResponseAck(MSG_ROUTING_NONE, params.request_id));
 }
 
 void CefBrowserHostImpl::OnResponseAck(int request_id) {
@@ -3513,3 +3477,26 @@ void CefBrowserHostImpl::EnsureFileDialogManager() {
         this, platform_delegate_->CreateFileDialogRunner()));
   }
 }
+
+bool CefBrowserHostImpl::Send(IPC::Message* message) {
+  if (!CEF_CURRENTLY_ON_UIT()) {
+    CEF_POST_TASK(
+        CEF_UIT, base::Bind(base::IgnoreResult(&CefBrowserHostImpl::Send), this,
+                            message));
+    return true;
+  }
+
+  if (queue_messages_) {
+    queued_messages_.push(message);
+    return true;
+  } else if (web_contents()) {
+    content::RenderViewHost* rvh = web_contents()->GetRenderViewHost();
+    if (rvh) {
+      message->set_routing_id(rvh->GetRoutingID());
+      return rvh->Send(message);
+    }
+  }
+
+  delete message;
+  return false;
+}

libcef/browser/browser_host_impl.h

@@ -339,11 +339,6 @@ class CefBrowserHostImpl : public CefBrowserHost,
   void ExecuteJavaScriptWithUserGestureForTests(int64 frame_id,
                                                 const CefString& javascript);
 
-  bool SendProcessMessage(CefProcessId target_process,
-                          const std::string& name,
-                          base::ListValue* arguments,
-                          bool user_initiated);
-
   // Open the specified text in the default text editor.
   void ViewText(const std::string& text);
 
@@ -517,12 +512,6 @@ class CefBrowserHostImpl : public CefBrowserHost,
   void OnWebContentsFocused(
       content::RenderWidgetHost* render_widget_host) override;
 
-  // Send a message to the RenderViewHost associated with this browser.
-  // TODO(cef): With the introduction of OOPIFs, WebContents can span multiple
-  // processes. Messages should be sent to specific RenderFrameHosts instead.
-  bool Send(IPC::Message* message);
-  int routing_id() const;
-
   // Manage observer objects. The observer must either outlive this object or
   // remove itself before destruction. These methods can only be called on the
   // UI thread.
@@ -634,6 +623,11 @@ class CefBrowserHostImpl : public CefBrowserHost,
   // Create the CefFileDialogManager if it doesn't already exist.
   void EnsureFileDialogManager();
 
+  // Send a message to the RenderViewHost associated with this browser.
+  // TODO(cef): With the introduction of OOPIFs, WebContents can span multiple
+  // processes. Messages should be sent to specific RenderFrameHosts instead.
+  bool Send(IPC::Message* message);
+
   CefBrowserSettings settings_;
   CefRefPtr<CefClient> client_;
   std::unique_ptr<content::WebContents> web_contents_;

tests/ceftests/process_message_unittest.cc

@@ -2,8 +2,10 @@
 // reserved. Use of this source code is governed by a BSD-style license that
 // can be found in the LICENSE file.
 
+#include "include/base/cef_bind.h"
 #include "include/cef_process_message.h"
 #include "include/cef_task.h"
+#include "include/wrapper/cef_closure_task.h"
 #include "tests/ceftests/test_handler.h"
 #include "tests/ceftests/test_util.h"
 #include "tests/gtest/include/gtest/gtest.h"
@@ -70,7 +72,8 @@ class SendRecvRendererTest : public ClientAppRenderer::Delegate {
 // Browser side.
 class SendRecvTestHandler : public TestHandler {
  public:
-  SendRecvTestHandler() {}
+  explicit SendRecvTestHandler(cef_thread_id_t send_thread)
+      : send_thread_(send_thread) {}
 
   void RunTest() override {
     message_ = CreateTestMessage();
@@ -85,13 +88,21 @@ class SendRecvTestHandler : public TestHandler {
   void OnLoadEnd(CefRefPtr<CefBrowser> browser,
                  CefRefPtr<CefFrame> frame,
                  int httpStatusCode) override {
+    EXPECT_TRUE(CefCurrentlyOn(TID_UI));
+
     // Send the message to the renderer process.
-    EXPECT_TRUE(browser->SendProcessMessage(PID_RENDERER, message_));
+    if (!CefCurrentlyOn(send_thread_)) {
+      CefPostTask(send_thread_,
+                  base::Bind(&SendRecvTestHandler::SendMessage, this, browser));
+    } else {
+      SendMessage(browser);
+    }
   }
 
   bool OnProcessMessageReceived(CefRefPtr<CefBrowser> browser,
                                 CefProcessId source_process,
                                 CefRefPtr<CefProcessMessage> message) override {
+    EXPECT_TRUE(CefCurrentlyOn(TID_UI));
     EXPECT_TRUE(browser.get());
     EXPECT_EQ(PID_RENDERER, source_process);
     EXPECT_TRUE(message.get());
@@ -108,6 +119,20 @@ class SendRecvTestHandler : public TestHandler {
     return true;
   }
 
+ protected:
+  void DestroyTest() override {
+    EXPECT_TRUE(got_message_);
+    TestHandler::DestroyTest();
+  }
+
+ private:
+  void SendMessage(CefRefPtr<CefBrowser> browser) {
+    EXPECT_TRUE(CefCurrentlyOn(send_thread_));
+    EXPECT_TRUE(browser->SendProcessMessage(PID_RENDERER, message_));
+  }
+
+  cef_thread_id_t send_thread_;
+
   CefRefPtr<CefProcessMessage> message_;
   TrackCallback got_message_;
 
@@ -116,13 +141,17 @@ class SendRecvTestHandler : public TestHandler {
 
 }  // namespace
 
-// Verify send and recieve.
+// Verify send from the UI thread and recieve.
-TEST(ProcessMessageTest, SendRecv) {
+TEST(ProcessMessageTest, SendRecvUI) {
-  CefRefPtr<SendRecvTestHandler> handler = new SendRecvTestHandler();
+  CefRefPtr<SendRecvTestHandler> handler = new SendRecvTestHandler(TID_UI);
   handler->ExecuteTest();
+  ReleaseAndWaitForDestructor(handler);
+}
 
-  EXPECT_TRUE(handler->got_message_);
+// Verify send from the IO thread and recieve.
-
+TEST(ProcessMessageTest, SendRecvIO) {
+  CefRefPtr<SendRecvTestHandler> handler = new SendRecvTestHandler(TID_IO);
+  handler->ExecuteTest();
   ReleaseAndWaitForDestructor(handler);
 }