John, please review everything.
agl, please review changes to the waitable_event_watcher code.
Multiple sync channels if used in the same listener thread could result in calls completing in the
wrong context at times. This happens if there are nested calls on these sync channels, i.e
1. Call from client 1 on channel 1 to server 1. Message pumping is enabled for the corresponding message
2. Call from client 2 on channel 2 to server 2, Message pumping is enabled for the corresponding message
Now if a reply for 1 arrives, it should be queued until reply 2 arrives. This does not happen which
causes 2 to terminate prematurely leading to crashes, 1 waiting indefinitely at times, etc.
The fix for this issue is to maintain a local global stack for the send done event watcher object.
The global object is in the form of a TLS. This ensures that we only watch for completed events
on the outermost sync channel.
The changes in the Waitable event watcher object are to return the current delegate which is needed
to start watching the old send watcher once we are done and to ensure that the event member is set even
if it was already signaled when StartWatching was called.
I have added a unit test in ipc_tests for this case. I removed the old QueuedReply based unit tests as
they did not test the actual nested call case.
While debugging these issues I also found some issues in BrowserRenderProcessHost::OnChannelError where
it would delete a sync channel while it was in use. Based on a discussion with jam we decided to DeleteSoon
the sync channel pointer. However this broke some browser ui tests which relied on the timing of the OnChannelError notification.
We decided to leave the existing code as is for now. I removed the DCHECK on channel as it would fire repeatedly if the channel died
while multiple sync calls were waiting to complete leading to OnChannelError firing multiple times.
Bug=24427
Review URL: http://codereview.chromium.org/271033
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@28967 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: ac0efda36e8f20bc74364e6e74e39f451a482537
diff --git a/ipc/ipc_sync_channel_unittest.cc b/ipc/ipc_sync_channel_unittest.cc
index f6bf10e..3219000 100644
--- a/ipc/ipc_sync_channel_unittest.cc
+++ b/ipc/ipc_sync_channel_unittest.cc
@@ -134,6 +134,10 @@
Send(reply_msg);
}
+ virtual void OnNestedTestMsg(Message* reply_msg) {
+ NOTREACHED();
+ }
+
private:
base::Thread* ListenerThread() {
return overrided_thread_ ? overrided_thread_ : &listener_thread_;
@@ -179,6 +183,8 @@
IPC_MESSAGE_HANDLER_DELAY_REPLY(SyncChannelTestMsg_Double, OnDoubleDelay)
IPC_MESSAGE_HANDLER_DELAY_REPLY(SyncChannelTestMsg_AnswerToLife,
OnAnswerDelay)
+ IPC_MESSAGE_HANDLER_DELAY_REPLY(SyncChannelNestedTestMsg_String,
+ OnNestedTestMsg)
IPC_END_MESSAGE_MAP()
}
@@ -640,100 +646,101 @@
namespace {
-class QueuedReplyServer1 : public Worker {
+// This class provides server side functionality to test the case where
+// multiple sync channels are in use on the same thread on the client and
+// nested calls are issued.
+class QueuedReplyServer : public Worker {
public:
- QueuedReplyServer1(bool pump_during_send)
- : Worker("test_channel1", Channel::MODE_SERVER),
- pump_during_send_(pump_during_send) { }
- void Run() {
- SendDouble(pump_during_send_, true);
- Done();
+ QueuedReplyServer(base::Thread* listener_thread,
+ const std::string& channel_name,
+ const std::string& reply_text)
+ : Worker(channel_name, Channel::MODE_SERVER),
+ reply_text_(reply_text) {
+ Worker::OverrideThread(listener_thread);
}
- bool pump_during_send_;
-};
-
-class QueuedReplyClient1 : public Worker {
- public:
- QueuedReplyClient1(WaitableEvent* client1_msg_received,
- WaitableEvent* server2_can_reply) :
- Worker("test_channel1", Channel::MODE_CLIENT),
- client1_msg_received_(client1_msg_received),
- server2_can_reply_(server2_can_reply) { }
-
- void OnDouble(int in, int* out) {
- client1_msg_received_->Signal();
- *out = in * 2;
- server2_can_reply_->Wait();
+ virtual void OnNestedTestMsg(Message* reply_msg) {
+ LOG(INFO) << __FUNCTION__ << " Sending reply: "
+ << reply_text_.c_str();
+ SyncChannelNestedTestMsg_String::WriteReplyParams(
+ reply_msg, reply_text_);
+ Send(reply_msg);
Done();
}
private:
- WaitableEvent *client1_msg_received_, *server2_can_reply_;
+ std::string reply_text_;
};
-class QueuedReplyServer2 : public Worker {
+// The QueuedReplyClient class provides functionality to test the case where
+// multiple sync channels are in use on the same thread and they make nested
+// sync calls, i.e. while the first channel waits for a response it makes a
+// sync call on another channel.
+// The callstack should unwind correctly, i.e. the outermost call should
+// complete first, and so on.
+class QueuedReplyClient : public Worker {
public:
- explicit QueuedReplyServer2(WaitableEvent* server2_can_reply) :
- Worker("test_channel2", Channel::MODE_SERVER),
- server2_can_reply_(server2_can_reply) { }
+ QueuedReplyClient(base::Thread* listener_thread,
+ const std::string& channel_name,
+ const std::string& expected_text,
+ bool pump_during_send)
+ : Worker(channel_name, Channel::MODE_CLIENT),
+ expected_text_(expected_text),
+ pump_during_send_(pump_during_send) {
+ Worker::OverrideThread(listener_thread);
+ }
- void OnAnswer(int* result) {
- server2_can_reply_->Signal();
+ virtual void Run() {
+ std::string response;
+ SyncMessage* msg = new SyncChannelNestedTestMsg_String(&response);
+ if (pump_during_send_)
+ msg->EnableMessagePumping();
+ bool result = Send(msg);
+ DCHECK(result);
+ DCHECK(response == expected_text_);
- // give client1's reply time to reach the server listener thread
- PlatformThread::Sleep(200);
-
- *result = 42;
+ LOG(INFO) << __FUNCTION__ << " Received reply: "
+ << response.c_str();
Done();
}
- WaitableEvent *server2_can_reply_;
-};
-
-class QueuedReplyClient2 : public Worker {
- public:
- explicit QueuedReplyClient2(
- WaitableEvent* client1_msg_received, bool pump_during_send)
- : Worker("test_channel2", Channel::MODE_CLIENT),
- client1_msg_received_(client1_msg_received),
- pump_during_send_(pump_during_send){ }
-
- void Run() {
- client1_msg_received_->Wait();
- SendAnswerToLife(pump_during_send_, base::kNoTimeout, true);
- Done();
- }
-
- WaitableEvent *client1_msg_received_;
+ private:
bool pump_during_send_;
+ std::string expected_text_;
};
-void QueuedReply(bool server_pump, bool client_pump) {
+void QueuedReply(bool client_pump) {
std::vector<Worker*> workers;
- // A shared worker thread so that server1 and server2 run on one thread.
- base::Thread worker_thread("QueuedReply");
- ASSERT_TRUE(worker_thread.Start());
+ // A shared worker thread for servers
+ base::Thread server_worker_thread("QueuedReply_ServerListener");
+ ASSERT_TRUE(server_worker_thread.Start());
- WaitableEvent client1_msg_received(false, false);
- WaitableEvent server2_can_reply(false, false);
+ base::Thread client_worker_thread("QueuedReply_ClientListener");
+ ASSERT_TRUE(client_worker_thread.Start());
Worker* worker;
- worker = new QueuedReplyServer2(&server2_can_reply);
- worker->OverrideThread(&worker_thread);
+ worker = new QueuedReplyServer(&server_worker_thread,
+ "QueuedReply_Server1",
+ "Got first message");
workers.push_back(worker);
- worker = new QueuedReplyClient2(&client1_msg_received, client_pump);
+ worker = new QueuedReplyServer(&server_worker_thread,
+ "QueuedReply_Server2",
+ "Got second message");
workers.push_back(worker);
- worker = new QueuedReplyServer1(server_pump);
- worker->OverrideThread(&worker_thread);
+ worker = new QueuedReplyClient(&client_worker_thread,
+ "QueuedReply_Server1",
+ "Got first message",
+ client_pump);
workers.push_back(worker);
- worker = new QueuedReplyClient1(
- &client1_msg_received, &server2_can_reply);
+ worker = new QueuedReplyClient(&client_worker_thread,
+ "QueuedReply_Server2",
+ "Got second message",
+ client_pump);
workers.push_back(worker);
RunTest(workers);
@@ -745,11 +752,11 @@
// synchronous messages. This tests that if during the response to another
// message the reply to the original messages comes, it is queued up correctly
// and the original Send is unblocked later.
+// We also test that the send call stacks unwind correctly when the channel
+// pumps messages while waiting for a response.
TEST_F(IPCSyncChannelTest, QueuedReply) {
- QueuedReply(false, false);
- QueuedReply(false, true);
- QueuedReply(true, false);
- QueuedReply(true, true);
+ QueuedReply(false);
+ QueuedReply(true);
}
//-----------------------------------------------------------------------------