From d8bc3d837237bf050cb655a109a171cda1e7d3bc Mon Sep 17 00:00:00 2001
From: Marshall Greenblatt <magreenblatt@gmail.com>
Date: Tue, 19 Dec 2017 15:14:00 -0500
Subject: [PATCH] Fix assertion when calling SendProcessMessage from non-UI
thread (issue #2325)
---
libcef/browser/browser_host_impl.cc | 101 +++++++++------------
libcef/browser/browser_host_impl.h | 16 +---
tests/ceftests/process_message_unittest.cc | 43 +++++++--
3 files changed, 85 insertions(+), 75 deletions(-)
diff --git a/libcef/browser/browser_host_impl.cc b/libcef/browser/browser_host_impl.cc
index 7d1789dc6..f581945b4 100644
--- a/libcef/browser/browser_host_impl.cc
+++ b/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)) {
- return SendProcessMessage(target_process, params.name, ¶ms.arguments,
- true);
- }
+ if (!impl->CopyTo(params))
+ return false;
- 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;
+}
diff --git a/libcef/browser/browser_host_impl.h b/libcef/browser/browser_host_impl.h
index 12bd2a9d3..bf15a93ff 100644
--- a/libcef/browser/browser_host_impl.h
+++ b/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_;
diff --git a/tests/ceftests/process_message_unittest.cc b/tests/ceftests/process_message_unittest.cc
index 316e41141..8352f2039 100644
--- a/tests/ceftests/process_message_unittest.cc
+++ b/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.
-TEST(ProcessMessageTest, SendRecv) {
- CefRefPtr<SendRecvTestHandler> handler = new SendRecvTestHandler();
+// Verify send from the UI thread and recieve.
+TEST(ProcessMessageTest, SendRecvUI) {
+ 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);
}