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

4 files changed, 129 insertions, 76 deletions

diff --git a/ipc/ipc_sync_channel.cc b/ipc/ipc_sync_channel.cc
index de9b434a..c5ec446 100644
--- a/ipc/ipc_sync_channel.cc
+++ b/ipc/ipc_sync_channel.cc
@@ -148,6 +148,14 @@ class SyncChannel::ReceivedSyncMsgQueue :
     }
   }
 
+  base::WaitableEventWatcher* top_send_done_watcher() {
+    return top_send_done_watcher_;
+  }
+
+  void set_top_send_done_watcher(base::WaitableEventWatcher* watcher) {
+    top_send_done_watcher_ = watcher;
+  }
+
  private:
   // See the comment in SyncChannel::SyncChannel for why this event is created
   // as manual reset.
@@ -155,7 +163,8 @@ class SyncChannel::ReceivedSyncMsgQueue :
       dispatch_event_(true, false),
       listener_message_loop_(MessageLoop::current()),
       task_pending_(false),
-      listener_count_(0) {
+      listener_count_(0),
+      top_send_done_watcher_(NULL) {
   }
 
   // Holds information about a queued synchronous message or reply.
@@ -178,6 +187,11 @@ class SyncChannel::ReceivedSyncMsgQueue :
   Lock message_lock_;
   bool task_pending_;
   int listener_count_;
+
+  // The current send done event watcher for this thread. Used to maintain
+  // a local global stack of send done watchers to ensure that nested sync
+  // message loops complete correctly.
+  base::WaitableEventWatcher* top_send_done_watcher_;
 };
 
 base::LazyInstance<base::ThreadLocalPointer<SyncChannel::ReceivedSyncMsgQueue> >
@@ -424,15 +438,39 @@ void SyncChannel::WaitForReply(WaitableEvent* pump_messages_event) {
 }
 
 void SyncChannel::WaitForReplyWithNestedMessageLoop() {
-  WaitableEvent* old_done_event = send_done_watcher_.GetWatchedEvent();
-  send_done_watcher_.StopWatching();
-  send_done_watcher_.StartWatching(sync_context()->GetSendDoneEvent(), this);
+  base::WaitableEventWatcher send_done_watcher;
+
+  ReceivedSyncMsgQueue* sync_msg_queue = sync_context()->received_sync_msgs();
+  DCHECK(sync_msg_queue != NULL);
+
+  base::WaitableEventWatcher* old_send_done_event_watcher =
+      sync_msg_queue->top_send_done_watcher();
+
+  base::WaitableEventWatcher::Delegate* old_delegate = NULL;
+  base::WaitableEvent* old_event = NULL;
+
+  // Maintain a local global stack of send done delegates to ensure that
+  // nested sync calls complete in the correct sequence, i.e. the
+  // outermost call completes first, etc.
+  if (old_send_done_event_watcher) {
+    old_delegate = old_send_done_event_watcher->delegate();
+    old_event = old_send_done_event_watcher->GetWatchedEvent();
+    old_send_done_event_watcher->StopWatching();
+  }
+
+  sync_msg_queue->set_top_send_done_watcher(&send_done_watcher);
+
+  send_done_watcher.StartWatching(sync_context()->GetSendDoneEvent(), this);
   bool old_state = MessageLoop::current()->NestableTasksAllowed();
+
   MessageLoop::current()->SetNestableTasksAllowed(true);
   MessageLoop::current()->Run();
   MessageLoop::current()->SetNestableTasksAllowed(old_state);
-  if (old_done_event)
-    send_done_watcher_.StartWatching(old_done_event, this);
+
+  sync_msg_queue->set_top_send_done_watcher(old_send_done_event_watcher);
+  if (old_send_done_event_watcher) {
+    old_send_done_event_watcher->StartWatching(old_event, old_delegate);
+  }
 }
 
 void SyncChannel::OnWaitableEventSignaled(WaitableEvent* event) {
diff --git a/ipc/ipc_sync_channel.h b/ipc/ipc_sync_channel.h
index bfc9eac..1c7360d 100644
--- a/ipc/ipc_sync_channel.h
+++ b/ipc/ipc_sync_channel.h
@@ -92,6 +92,10 @@ class SyncChannel : public ChannelProxy,
 
     base::WaitableEvent* shutdown_event() { return shutdown_event_; }
 
+    ReceivedSyncMsgQueue* received_sync_msgs() {
+      return received_sync_msgs_;
+    }
+
    private:
     // IPC::ChannelProxy methods that we override.
 
@@ -151,7 +155,6 @@ class SyncChannel : public ChannelProxy,
   bool sync_messages_with_no_timeout_allowed_;
 
   // Used to signal events between the IPC and listener threads.
-  base::WaitableEventWatcher send_done_watcher_;
   base::WaitableEventWatcher dispatch_watcher_;
 
   DISALLOW_EVIL_CONSTRUCTORS(SyncChannel);
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 @@ class Worker : public Channel::Listener, public Message::Sender {
     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 @@ class Worker : public Channel::Listener, public Message::Sender {
      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 @@ TEST_F(IPCSyncChannelTest, Multiple) {
 
 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();
-  }
-
-  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();
+   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);
+  }
+
+  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) { }
-
-  void OnAnswer(int* result) {
-    server2_can_reply_->Signal();
-
-    // give client1's reply time to reach the server listener thread
-    PlatformThread::Sleep(200);
-
-    *result = 42;
-    Done();
+  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);
   }
 
-  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){ }
+  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_);
 
-  void Run() {
-    client1_msg_received_->Wait();
-    SendAnswerToLife(pump_during_send_, base::kNoTimeout, true);
+    LOG(INFO) << __FUNCTION__ << " Received reply: "
+              << response.c_str();
     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 @@ void QueuedReply(bool server_pump, bool client_pump) {
 // 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);
 }
 
 //-----------------------------------------------------------------------------
diff --git a/ipc/ipc_sync_message_unittest.h b/ipc/ipc_sync_message_unittest.h
index 06c9af0..f4e98c1 100644
--- a/ipc/ipc_sync_message_unittest.h
+++ b/ipc/ipc_sync_message_unittest.h
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+
 #include "ipc/ipc_message_macros.h"
 
 IPC_BEGIN_MESSAGES(Test)
@@ -14,6 +16,9 @@ IPC_BEGIN_MESSAGES(Test)
                               int /* in */,
                               int /* out */)
 
+  IPC_SYNC_MESSAGE_CONTROL0_1(SyncChannelNestedTestMsg_String,
+                              std::string)
+
   // out1 is false
   IPC_SYNC_MESSAGE_CONTROL0_1(Msg_C_0_1, bool)