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

1 files changed, 44 insertions, 6 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) {