posix: add some comments and clean up some IPC channel code.

Review URL: http://codereview.chromium.org/100237

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

6 files changed, 36 insertions, 51 deletions

diff --git a/chrome/common/ipc_channel.h b/chrome/common/ipc_channel.h
index dabfe1a..0ce037e 100644
--- a/chrome/common/ipc_channel.h
+++ b/chrome/common/ipc_channel.h
@@ -95,10 +95,6 @@ class Channel : public Message::Sender {
   // a named FIFO is used as the channel transport mechanism rather than a
   // socketpair() in which case this method returns -1 for both parameters.
   void GetClientFileDescriptorMapping(int *src_fd, int *dest_fd) const;
-
-  // Call this method on the server side of the IPC Channel once a client is
-  // connected in order to close the client side of the socketpair().
-  void OnClientConnected();
 #endif  // defined(OS_POSIX)
 
  private:
diff --git a/chrome/common/ipc_channel_posix.cc b/chrome/common/ipc_channel_posix.cc
index 522a6bc..72a1c37 100644
--- a/chrome/common/ipc_channel_posix.cc
+++ b/chrome/common/ipc_channel_posix.cc
@@ -32,14 +32,26 @@
 
 namespace IPC {
 
+// IPC channels on Windows use named pipes (CreateNamedPipe()) with
+// channel ids as the pipe names.  Channels on POSIX use anonymous
+// Unix domain sockets created via socketpair() as pipes.  These don't
+// quite line up.
+//
+// When creating a child subprocess, the parent side of the fork
+// arranges it such that the initial control channel ends up on the
+// magic file descriptor kClientChannelFd in the child.  Future
+// connections (file descriptors) can then be passed via that
+// connection via sendmsg().
+
 //------------------------------------------------------------------------------
 namespace {
 
-// When running as a browser, we install the client socket in a specific file
-// descriptor number (@kClientChannelFd). However, we also have to support the
-// case where we are running unittests in the same process.
+// The PipeMap class works around this quirk related to unit tests:
 //
-// We do not support forking without execing.
+// When running as a server, we install the client socket in a
+// specific file descriptor number (@kClientChannelFd). However, we
+// also have to support the case where we are running unittests in the
+// same process.  (We do not support forking without execing.)
 //
 // Case 1: normal running
 //   The IPC server object will install a mapping in PipeMap from the
@@ -48,7 +60,7 @@ namespace {
 //   the magic slot (@kClientChannelFd). The client will search for the
 //   mapping, but it won't find any since we are in a new process. Thus the
 //   magic fd number is returned. Once the client connects, the server will
-//   close it's copy of the client socket and remove the mapping.
+//   close its copy of the client socket and remove the mapping.
 //
 // Case 2: unittests - client and server in the same process
 //   The IPC server will install a mapping as before. The client will search
@@ -123,7 +135,7 @@ sockaddr_un sizecheck;
 const size_t kMaxPipeNameLength = sizeof(sizecheck.sun_path);
 
 // Creates a Fifo with the specified name ready to listen on.
-bool CreateServerFifo(const std::string &pipe_name, int* server_listen_fd) {
+bool CreateServerFifo(const std::string& pipe_name, int* server_listen_fd) {
   DCHECK(server_listen_fd);
   DCHECK_GT(pipe_name.length(), 0u);
   DCHECK_LT(pipe_name.length(), kMaxPipeNameLength);
@@ -251,22 +263,16 @@ Channel::ChannelImpl::ChannelImpl(const std::wstring& channel_id, Mode mode,
   }
 }
 
-const std::wstring Channel::ChannelImpl::PipeName(
-    const std::wstring& channel_id) const {
-  // TODO(playmobil): This should live in the Chrome user data directory.
-  // TODO(playmobil): Cleanup any stale fifos.
-  return L"/var/tmp/chrome_" + channel_id;
-}
-
 bool Channel::ChannelImpl::CreatePipe(const std::wstring& channel_id,
                                       Mode mode) {
   DCHECK(server_listen_pipe_ == -1 && pipe_ == -1);
-  pipe_name_ = WideToUTF8(PipeName(channel_id));
 
   if (uses_fifo_) {
-    // TODO(playmobil): Should we just change pipe_name to be a normal string
-    // everywhere?
-
+    // This only happens in unit tests; see the comment above PipeMap.
+    // TODO(playmobil): We shouldn't need to create fifos on disk.
+    // TODO(playmobil): If we do, they should be in the user data directory.
+    // TODO(playmobil): Cleanup any stale fifos.
+    pipe_name_ = "/var/tmp/chrome_" + WideToASCII(channel_id);
     if (mode == MODE_SERVER) {
       if (!CreateServerFifo(pipe_name_, &server_listen_pipe_)) {
         return false;
@@ -279,6 +285,7 @@ bool Channel::ChannelImpl::CreatePipe(const std::wstring& channel_id,
     }
   } else {
     // socketpair()
+    pipe_name_ = WideToASCII(channel_id);
     if (mode == MODE_SERVER) {
       int pipe_fds[2];
       if (socketpair(AF_UNIX, SOCK_STREAM, 0, pipe_fds) != 0) {
@@ -657,11 +664,6 @@ void Channel::ChannelImpl::GetClientFileDescriptorMapping(int *src_fd,
   *dest_fd = kClientChannelFd;
 }
 
-void Channel::ChannelImpl::OnClientConnected() {
-  // WARNING: this isn't actually called when a client connects.
-  DCHECK(mode_ == MODE_SERVER);
-}
-
 // Called by libevent when we can read from th pipe without blocking.
 void Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int fd) {
   bool send_server_hello_msg = false;
@@ -741,8 +743,10 @@ void Channel::ChannelImpl::Close() {
     client_pipe_ = -1;
   }
 
-  // Unlink the FIFO
-  unlink(pipe_name_.c_str());
+  if (uses_fifo_) {
+    // Unlink the FIFO
+    unlink(pipe_name_.c_str());
+  }
 
   while (!output_queue_.empty()) {
     Message* m = output_queue_.front();
@@ -789,9 +793,4 @@ void Channel::GetClientFileDescriptorMapping(int *src_fd, int *dest_fd) const {
   return channel_impl_->GetClientFileDescriptorMapping(src_fd, dest_fd);
 }
 
-void Channel::OnClientConnected() {
-  return channel_impl_->OnClientConnected();
-}
-
-
 }  // namespace IPC
diff --git a/chrome/common/ipc_channel_posix.h b/chrome/common/ipc_channel_posix.h
index f94b171..b45503b 100644
--- a/chrome/common/ipc_channel_posix.h
+++ b/chrome/common/ipc_channel_posix.h
@@ -18,6 +18,8 @@
 
 namespace IPC {
 
+// An implementation of ChannelImpl for POSIX systems that works via
+// socketpairs.  See the .cc file for an overview of the implementation.
 class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
  public:
   // Mirror methods of Channel, see ipc_channel.h for description.
@@ -28,17 +30,16 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   void set_listener(Listener* listener) { listener_ = listener; }
   bool Send(Message* message);
   void GetClientFileDescriptorMapping(int *src_fd, int *dest_fd) const;
-  void OnClientConnected();
 
  private:
-  const std::wstring PipeName(const std::wstring& channel_id) const;
   bool CreatePipe(const std::wstring& channel_id, Mode mode);
 
   bool ProcessIncomingMessages();
   bool ProcessOutgoingMessages();
 
-  void OnFileCanReadWithoutBlocking(int fd);
-  void OnFileCanWriteWithoutBlocking(int fd);
+  // MessageLoopForIO::Watcher implementation.
+  virtual void OnFileCanReadWithoutBlocking(int fd);
+  virtual void OnFileCanWriteWithoutBlocking(int fd);
 
   Mode mode_;
 
@@ -62,6 +63,9 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   int server_listen_pipe_;
   int pipe_;
   int client_pipe_;  // The client end of our socketpair().
+
+  // The "name" of our pipe.  On Windows this is the global identifier for
+  // the pipe.  On POSIX it's used as a key in a local map of file descriptors.
   std::string pipe_name_;
 
   Listener* listener_;
diff --git a/chrome/common/ipc_channel_proxy.cc b/chrome/common/ipc_channel_proxy.cc
index 47aa89b..7668923 100644
--- a/chrome/common/ipc_channel_proxy.cc
+++ b/chrome/common/ipc_channel_proxy.cc
@@ -292,13 +292,6 @@ void ChannelProxy::GetClientFileDescriptorMapping(int *src_fd,
   DCHECK(channel); // Channel must have been created first.
   channel->GetClientFileDescriptorMapping(src_fd, dest_fd);
 }
-
-// We assume that IP::Channel::OnClientConnected() is thread-safe.
-void ChannelProxy::OnClientConnected() {
-  Channel *channel = context_.get()->channel_;
-  DCHECK(channel); // Channel must have been created first.
-  channel->OnClientConnected();
-}
 #endif
 
 //-----------------------------------------------------------------------------
diff --git a/chrome/common/ipc_channel_proxy.h b/chrome/common/ipc_channel_proxy.h
index 2263ea6..1a68971 100644
--- a/chrome/common/ipc_channel_proxy.h
+++ b/chrome/common/ipc_channel_proxy.h
@@ -118,7 +118,6 @@ class ChannelProxy : public Message::Sender {
   // TODO(playmobil): For now this is only implemented in the case of
   // create_pipe_now = true, we need to figure this out for the latter case.
   void GetClientFileDescriptorMapping(int *src_fd, int *dest_fd) const;
-  void OnClientConnected();
 #endif  // defined(OS_POSIX)
 
  protected:
diff --git a/chrome/common/ipc_tests.cc b/chrome/common/ipc_tests.cc
index 4c44d44e..17d4d2c 100644
--- a/chrome/common/ipc_tests.cc
+++ b/chrome/common/ipc_tests.cc
@@ -99,31 +99,26 @@ base::ProcessHandle IPCChannelTest::SpawnChild(ChildType child_type,
     ret = MultiProcessTest::SpawnChild(L"RunTestClient",
                                        fds_to_map,
                                        debug_on_start);
-    channel->OnClientConnected();
     break;
   case TEST_DESCRIPTOR_CLIENT:
     ret = MultiProcessTest::SpawnChild(L"RunTestDescriptorClient",
                                        fds_to_map,
                                        debug_on_start);
-    channel->OnClientConnected();
     break;
   case TEST_DESCRIPTOR_CLIENT_SANDBOXED:
     ret = MultiProcessTest::SpawnChild(L"RunTestDescriptorClientSandboxed",
                                        fds_to_map,
                                        debug_on_start);
-    channel->OnClientConnected();
     break;
   case TEST_REFLECTOR:
     ret = MultiProcessTest::SpawnChild(L"RunReflector",
                                        fds_to_map,
                                        debug_on_start);
-    channel->OnClientConnected();
     break;
   case FUZZER_SERVER:
     ret = MultiProcessTest::SpawnChild(L"RunFuzzServer",
                                        fds_to_map,
                                        debug_on_start);
-    channel->OnClientConnected();
     break;
   default:
     return NULL;
@@ -274,7 +269,6 @@ TEST_F(IPCChannelTest, ChannelProxyTest) {
         L"RunTestClient",
         fds_to_map,
         debug_on_start);
-    chan.OnClientConnected();
 #endif  // defined(OS_POXIX)
 
     ASSERT_TRUE(process_handle);