Don't use any member variables when sending a sync call, since SyncChannel could be deleted.

BUG=20364
Review URL: http://codereview.chromium.org/455011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@33414 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 74 insertions, 46 deletions

diff --git a/ipc/ipc_sync_channel.cc b/ipc/ipc_sync_channel.cc
index 81f8dc1..3aa7a26 100644
--- a/ipc/ipc_sync_channel.cc
+++ b/ipc/ipc_sync_channel.cc
@@ -343,10 +343,15 @@ void SyncChannel::SyncContext::CancelPendingSends() {
 }
 
 void SyncChannel::SyncContext::OnWaitableEventSignaled(WaitableEvent* event) {
-  DCHECK(event == shutdown_event_);
-  // Process shut down before we can get a reply to a synchronous message.
-  // Cancel pending Send calls, which will end up setting the send done event.
-  CancelPendingSends();
+  if (event == shutdown_event_) {
+    // Process shut down before we can get a reply to a synchronous message.
+    // Cancel pending Send calls, which will end up setting the send done event.
+    CancelPendingSends();
+  } else {
+    // We got the reply, timed out or the process shutdown.
+    DCHECK(event == GetSendDoneEvent());
+    MessageLoop::current()->Quit();
+  }
 }
 
 
@@ -408,16 +413,18 @@ bool SyncChannel::SendWithTimeout(Message* message, int timeout_ms) {
   }
 
   // Wait for reply, or for any other incoming synchronous messages.
-  WaitForReply(pump_messages_event);
+  // *this* might get deleted, so only call static functions at this point.
+  WaitForReply(context, pump_messages_event);
 
   return context->Pop();
 }
 
-void SyncChannel::WaitForReply(WaitableEvent* pump_messages_event) {
+void SyncChannel::WaitForReply(
+    SyncContext* context, WaitableEvent* pump_messages_event) {
   while (true) {
     WaitableEvent* objects[] = {
-      sync_context()->GetDispatchEvent(),
-      sync_context()->GetSendDoneEvent(),
+      context->GetDispatchEvent(),
+      context->GetSendDoneEvent(),
       pump_messages_event
     };
 
@@ -426,22 +433,22 @@ void SyncChannel::WaitForReply(WaitableEvent* pump_messages_event) {
     if (result == 0 /* dispatch event */) {
       // We're waiting for a reply, but we received a blocking synchronous
       // call.  We must process it or otherwise a deadlock might occur.
-      sync_context()->GetDispatchEvent()->Reset();
-      sync_context()->DispatchMessages();
+      context->GetDispatchEvent()->Reset();
+      context->DispatchMessages();
       continue;
     }
 
     if (result == 2 /* pump_messages_event */)
-      WaitForReplyWithNestedMessageLoop();  // Start a nested message loop.
+      WaitForReplyWithNestedMessageLoop(context);  // Run a nested message loop.
 
     break;
   }
 }
 
-void SyncChannel::WaitForReplyWithNestedMessageLoop() {
+void SyncChannel::WaitForReplyWithNestedMessageLoop(SyncContext* context) {
   base::WaitableEventWatcher send_done_watcher;
 
-  ReceivedSyncMsgQueue* sync_msg_queue = sync_context()->received_sync_msgs();
+  ReceivedSyncMsgQueue* sync_msg_queue = context->received_sync_msgs();
   DCHECK(sync_msg_queue != NULL);
 
   base::WaitableEventWatcher* old_send_done_event_watcher =
@@ -461,7 +468,7 @@ void SyncChannel::WaitForReplyWithNestedMessageLoop() {
 
   sync_msg_queue->set_top_send_done_watcher(&send_done_watcher);
 
-  send_done_watcher.StartWatching(sync_context()->GetSendDoneEvent(), this);
+  send_done_watcher.StartWatching(context->GetSendDoneEvent(), context);
   bool old_state = MessageLoop::current()->NestableTasksAllowed();
 
   MessageLoop::current()->SetNestableTasksAllowed(true);
@@ -475,18 +482,12 @@ void SyncChannel::WaitForReplyWithNestedMessageLoop() {
 }
 
 void SyncChannel::OnWaitableEventSignaled(WaitableEvent* event) {
-  WaitableEvent* dispatch_event = sync_context()->GetDispatchEvent();
-  if (event == dispatch_event) {
-    // The call to DispatchMessages might delete this object, so reregister
-    // the object watcher first.
-    dispatch_event->Reset();
-    dispatch_watcher_.StartWatching(dispatch_event, this);
-    sync_context()->DispatchMessages();
-  } else {
-    // We got the reply, timed out or the process shutdown.
-    DCHECK(event == sync_context()->GetSendDoneEvent());
-    MessageLoop::current()->Quit();
-  }
+  DCHECK(event == sync_context()->GetDispatchEvent());
+  // The call to DispatchMessages might delete this object, so reregister
+  // the object watcher first.
+  event->Reset();
+  dispatch_watcher_.StartWatching(event, this);
+  sync_context()->DispatchMessages();
 }
 
 }  // namespace IPC
diff --git a/ipc/ipc_sync_channel.h b/ipc/ipc_sync_channel.h
index cde293a58..cb88f47 100644
--- a/ipc/ipc_sync_channel.h
+++ b/ipc/ipc_sync_channel.h
@@ -145,11 +145,12 @@ class SyncChannel : public ChannelProxy,
 
   // Both these functions wait for a reply, timeout or process shutdown.  The
   // latter one also runs a nested message loop in the meantime.
-  void WaitForReply(base::WaitableEvent* pump_messages_event);
+  static void WaitForReply(
+      SyncContext* context, base::WaitableEvent* pump_messages_event);
 
   // Runs a nested message loop until a reply arrives, times out, or the process
   // shuts down.
-  void WaitForReplyWithNestedMessageLoop();
+  static void WaitForReplyWithNestedMessageLoop(SyncContext* context);
 
   bool sync_messages_with_no_timeout_allowed_;
 
diff --git a/ipc/ipc_sync_channel_unittest.cc b/ipc/ipc_sync_channel_unittest.cc
index 007f354..c9a5c88 100644
--- a/ipc/ipc_sync_channel_unittest.cc
+++ b/ipc/ipc_sync_channel_unittest.cc
@@ -104,8 +104,8 @@ class Worker : public Channel::Listener, public Message::Sender {
     if (pump)
       msg->EnableMessagePumping();
     bool result = SendWithTimeout(msg, timeout);
-    DCHECK(result == succeed);
-    DCHECK(answer == (succeed ? 42 : 0));
+    DCHECK_EQ(result, succeed);
+    DCHECK_EQ(answer, (succeed ? 42 : 0));
     return result;
   }
   bool SendDouble(bool pump, bool succeed) {
@@ -114,12 +114,13 @@ class Worker : public Channel::Listener, public Message::Sender {
     if (pump)
       msg->EnableMessagePumping();
     bool result = Send(msg);
-    DCHECK(result == succeed);
-    DCHECK(answer == (succeed ? 10 : 0));
+    DCHECK_EQ(result, succeed);
+    DCHECK_EQ(answer, (succeed ? 10 : 0));
     return result;
   }
   Channel::Mode mode() { return mode_; }
   WaitableEvent* done_event() { return done_.get(); }
+  void ResetChannel() { channel_.reset(); }
 
  protected:
   // Derived classes need to call this when they've completed their part of
@@ -389,19 +390,34 @@ namespace {
 
 class UnblockServer : public Worker {
  public:
-  UnblockServer(bool pump_during_send)
+  UnblockServer(bool pump_during_send, bool delete_during_send)
     : Worker(Channel::MODE_SERVER, "unblock_server"),
-      pump_during_send_(pump_during_send) { }
+      pump_during_send_(pump_during_send),
+      delete_during_send_(delete_during_send) { }
   void Run() {
-    SendAnswerToLife(pump_during_send_, base::kNoTimeout, true);
+    if (delete_during_send_) {
+      // Use custom code since race conditions mean the answer may or may not be
+      // available.
+      int answer = 0;
+      SyncMessage* msg = new SyncChannelTestMsg_AnswerToLife(&answer);
+      if (pump_during_send_)
+        msg->EnableMessagePumping();
+      Send(msg);
+    } else {
+      SendAnswerToLife(pump_during_send_, base::kNoTimeout, true);
+    }
     Done();
   }
 
-  void OnDouble(int in, int* out) {
-    *out = in * 2;
+  void OnDoubleDelay(int in, Message* reply_msg) {
+    SyncChannelTestMsg_Double::WriteReplyParams(reply_msg, in * 2);
+    Send(reply_msg);
+    if (delete_during_send_)
+      ResetChannel();
   }
 
   bool pump_during_send_;
+  bool delete_during_send_;
 };
 
 class UnblockClient : public Worker {
@@ -419,9 +435,9 @@ class UnblockClient : public Worker {
   bool pump_during_send_;
 };
 
-void Unblock(bool server_pump, bool client_pump) {
+void Unblock(bool server_pump, bool client_pump, bool delete_during_send) {
   std::vector<Worker*> workers;
-  workers.push_back(new UnblockServer(server_pump));
+  workers.push_back(new UnblockServer(server_pump, delete_during_send));
   workers.push_back(new UnblockClient(client_pump));
   RunTest(workers);
 }
@@ -430,10 +446,20 @@ void Unblock(bool server_pump, bool client_pump) {
 
 // Tests that the caller unblocks to answer a sync message from the receiver.
 TEST_F(IPCSyncChannelTest, Unblock) {
-  Unblock(false, false);
-  Unblock(false, true);
-  Unblock(true, false);
-  Unblock(true, true);
+  Unblock(false, false, false);
+  Unblock(false, true, false);
+  Unblock(true, false, false);
+  Unblock(true, true, false);
+}
+
+//-----------------------------------------------------------------------------
+
+// Tests that the the IPC::SyncChannel object can be deleted during a Send.
+TEST_F(IPCSyncChannelTest, ChannelDeleteDuringSend) {
+  Unblock(false, false, true);
+  Unblock(false, true, true);
+  Unblock(true, false, true);
+  Unblock(true, true, true);
 }
 
 //-----------------------------------------------------------------------------
@@ -710,7 +736,7 @@ class QueuedReplyClient : public Worker {
       msg->EnableMessagePumping();
     bool result = Send(msg);
     DCHECK(result);
-    DCHECK(response == expected_text_);
+    DCHECK_EQ(response, expected_text_);
 
     LOG(INFO) << __FUNCTION__ << " Received reply: "
               << response.c_str();
@@ -801,7 +827,7 @@ class BadServer : public Worker {
     // Need to send another message to get the client to call Done().
     result = Send(new SyncChannelTestMsg_AnswerToLife(&answer));
     DCHECK(result);
-    DCHECK(answer == 42);
+    DCHECK_EQ(answer, 42);
 
     Done();
   }
@@ -848,7 +874,7 @@ class ChattyClient : public Worker {
 
 void ChattyServer(bool pump_during_send) {
   std::vector<Worker*> workers;
-  workers.push_back(new UnblockServer(pump_during_send));
+  workers.push_back(new UnblockServer(pump_during_send, false));
   workers.push_back(new ChattyClient());
   RunTest(workers);
 }