Bug 885158 - If a message is sent on a closed IPC channel, delete it immediately. r=bent
☠☠ backed out by 4d2681627e62 ☠ ☠
authorJustin Lebar <justin.lebar@gmail.com>
Thu, 20 Jun 2013 14:58:43 -0700
changeset 147362 8c77605c9c5ad5cf848bd12f7c678a193a8c0936
parent 147282 1506080a9570cfd8026f08bab16ce4bc33dd8eb3
child 147363 4d2681627e62d559f316232164a94c8dcaecbb27
push id2697
push userbbajaj@mozilla.com
push dateMon, 05 Aug 2013 18:49:53 +0000
treeherdermozilla-beta@dfec938c7b63 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbent
bugs885158
milestone24.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 885158 - If a message is sent on a closed IPC channel, delete it immediately. r=bent Our old behavior was to enqueue it until the channel gets destructed.
ipc/chromium/src/chrome/common/ipc_channel.h
ipc/chromium/src/chrome/common/ipc_channel_posix.cc
ipc/chromium/src/chrome/common/ipc_channel_posix.h
ipc/chromium/src/chrome/common/ipc_channel_win.cc
ipc/chromium/src/chrome/common/ipc_channel_win.h
--- a/ipc/chromium/src/chrome/common/ipc_channel.h
+++ b/ipc/chromium/src/chrome/common/ipc_channel.h
@@ -91,20 +91,18 @@ class Channel : public Message::Sender {
   // Modify the Channel's listener.
   Listener* set_listener(Listener* listener);
 
   // Send a message over the Channel to the listener on the other end.
   //
   // |message| must be allocated using operator new.  This object will be
   // deleted once the contents of the Message have been sent.
   //
-  //  FIXME bug 551500: the channel does not notice failures, so if the
-  //    renderer crashes, it will silently succeed, leaking the parameter.
-  //    At least the leak will be fixed by...
-  //
+  // If you Send() a message on a Close()'d channel, we delete the message
+  // immediately.
   virtual bool Send(Message* message);
 
 #if defined(OS_POSIX)
   // On POSIX an IPC::Channel wraps a socketpair(), this method returns the
   // FD # for the client end of the socket and the equivalent FD# to use for
   // mapping it into the Child process.
   // This method may only be called on the server side of a channel.
   //
--- a/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
+++ b/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ -283,16 +283,17 @@ void Channel::ChannelImpl::Init(Mode mod
   message_send_bytes_written_ = 0;
   uses_fifo_ = false;
   server_listen_pipe_ = -1;
   pipe_ = -1;
   client_pipe_ = -1;
   listener_ = listener;
   waiting_connect_ = true;
   processing_incoming_ = false;
+  closed_ = false;
 }
 
 bool Channel::ChannelImpl::CreatePipe(const std::wstring& channel_id,
                                       Mode mode) {
   DCHECK(server_listen_pipe_ == -1 && pipe_ == -1);
 
   if (uses_fifo_) {
     // This only happens in unit tests; see the comment above PipeMap.
@@ -706,16 +707,28 @@ bool Channel::ChannelImpl::Send(Message*
              << " with type " << message->type()
              << " (" << output_queue_.size() << " in queue)";
 #endif
 
 #ifdef IPC_MESSAGE_LOG_ENABLED
   Logging::current()->OnSendMessage(message, L"");
 #endif
 
+  // If the channel has been closed, ProcessOutgoingMessages() is never going
+  // to pop anything off output_queue; output_queue will only get emptied when
+  // the channel is destructed.  We might as well delete message now, instead
+  // of waiting for the channel to be destructed.
+  if (closed_) {
+#ifdef IPC_MESSAGE_LOG_ENABLED
+    Logging::current()->OnSendMessageFailed(message, L"");
+#endif
+    delete message;
+    return false;
+  }
+
   output_queue_.push(message);
   if (!waiting_connect_) {
     if (!is_blocked_on_write_) {
       if (!ProcessOutgoingMessages())
         return false;
     }
   }
 
@@ -820,16 +833,18 @@ void Channel::ChannelImpl::Close() {
   }
 
   // Close any outstanding, received file descriptors
   for (std::vector<int>::iterator
        i = input_overflow_fds_.begin(); i != input_overflow_fds_.end(); ++i) {
     HANDLE_EINTR(close(*i));
   }
   input_overflow_fds_.clear();
+
+  closed_ = true;
 }
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
 Channel::Channel(const std::wstring& channel_id, Mode mode,
                  Listener* listener)
     : channel_impl_(new ChannelImpl(channel_id, mode, listener)) {
 }
--- a/ipc/chromium/src/chrome/common/ipc_channel_posix.h
+++ b/ipc/chromium/src/chrome/common/ipc_channel_posix.h
@@ -112,16 +112,19 @@ class Channel::ChannelImpl : public Mess
   // the connect operation in overlapped mode.
   bool waiting_connect_;
 
   // This flag is set when processing incoming messages.  It is used to
   // avoid recursing through ProcessIncomingMessages, which could cause
   // problems.  TODO(darin): make this unnecessary
   bool processing_incoming_;
 
+  // This flag is set after we've closed the channel.
+  bool closed_;
+
   ScopedRunnableMethodFactory<ChannelImpl> factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelImpl);
 };
 
 }  // namespace IPC
 
 #endif  // CHROME_COMMON_IPC_CHANNEL_POSIX_H_
--- a/ipc/chromium/src/chrome/common/ipc_channel_win.cc
+++ b/ipc/chromium/src/chrome/common/ipc_channel_win.cc
@@ -63,16 +63,17 @@ Channel::ChannelImpl::ChannelImpl(const 
   }
 }
 
 void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
   pipe_ = INVALID_HANDLE_VALUE;
   listener_ = listener;
   waiting_connect_ = (mode == MODE_SERVER);
   processing_incoming_ = false;
+  closed_ = false;
 }
 
 HANDLE Channel::ChannelImpl::GetServerPipeHandle() const {
   return pipe_;
 }
 
 void Channel::ChannelImpl::Close() {
   if (thread_check_.get()) {
@@ -99,16 +100,18 @@ void Channel::ChannelImpl::Close() {
   while (!output_queue_.empty()) {
     Message* m = output_queue_.front();
     output_queue_.pop();
     delete m;
   }
 
   if (thread_check_.get())
     thread_check_.reset();
+
+  closed_ = true;
 }
 
 bool Channel::ChannelImpl::Send(Message* message) {
   if (thread_check_.get()) {
     DCHECK(thread_check_->CalledOnValidThread());
   }
   chrome::Counters::ipc_send_counter().Increment();
 #ifdef IPC_MESSAGE_DEBUG_EXTRA
@@ -116,16 +119,24 @@ bool Channel::ChannelImpl::Send(Message*
              << " with type " << message->type()
              << " (" << output_queue_.size() << " in queue)";
 #endif
 
 #ifdef IPC_MESSAGE_LOG_ENABLED
   Logging::current()->OnSendMessage(message, L"");
 #endif
 
+  if (closed_) {
+#ifdef IPC_MESSAGE_LOG_ENABLED
+    Logging::current()->OnSendMessageFailed(message, L"");
+#endif
+    delete message;
+    return false;
+  }
+
   output_queue_.push(message);
   // ensure waiting to write
   if (!waiting_connect_) {
     if (!output_state_.is_pending) {
       if (!ProcessOutgoingMessages(NULL, 0))
         return false;
     }
   }
--- a/ipc/chromium/src/chrome/common/ipc_channel_win.h
+++ b/ipc/chromium/src/chrome/common/ipc_channel_win.h
@@ -81,16 +81,19 @@ class Channel::ChannelImpl : public Mess
   // the connect operation in overlapped mode.
   bool waiting_connect_;
 
   // This flag is set when processing incoming messages.  It is used to
   // avoid recursing through ProcessIncomingMessages, which could cause
   // problems.  TODO(darin): make this unnecessary
   bool processing_incoming_;
 
+  // This flag is set after Close() is run on the channel.
+  bool closed_;
+
   ScopedRunnableMethodFactory<ChannelImpl> factory_;
 
   scoped_ptr<NonThreadSafe> thread_check_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelImpl);
 };
 
 }  // namespace IPC