Add multi-process notification class.

chrome/common/multi_process_notification_mac.mm

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/common/multi_process_notification.h"
+
+#import <Foundation/Foundation.h>
+#include <notify.h>
+#include <sys/select.h>
+#include <sys/socket.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include "base/basictypes.h"
+#include "base/eintr_wrapper.h"
+#include "base/file_path.h"
+#include "base/logging.h"
+#include "base/mac/mac_util.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
+#include "base/message_loop.h"
+#include "base/message_pump_libevent.h"
+#include "base/path_service.h"
+#include "base/stringprintf.h"
+#include "base/synchronization/lock.h"
+#include "base/sys_string_conversions.h"
+#include "base/sys_info.h"
+#include "base/threading/simple_thread.h"
+#include "chrome/common/chrome_paths.h"
+
+// Enable this to build with leopard_switchboard_thread
+#define USE_LEOPARD_SWITCHBOARD_THREAD 1

[Mark Mentovai, 2011/01/12 22:42:36]

You don’t want to check this in, do you?

+
+namespace {
+
+std::string AddPrefixToNotification(const std::string& name,
+                                    multi_process_notification::Domain domain) {
+  // The ordering of the components in the string returned by this function
+  // is important. Read "NAMESPACE CONVENTIONS" in 'man 3 notify' for details.
+  base::mac::ScopedNSAutoreleasePool pool;
+  NSBundle* bundle = base::mac::MainAppBundle();
+  NSString* ns_bundle_id = [bundle bundleIdentifier];
+  std::string bundle_id = base::SysNSStringToUTF8(ns_bundle_id);
+  std::string domain_string;
+  switch (domain) {
+    case multi_process_notification::ProfileDomain: {
+      FilePath user_data_dir;
+      if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) {
+        NOTREACHED();
+      }
+      domain_string = StringPrintf("user.uid.%u.%s.",
+                                   getuid(), user_data_dir.value().c_str());
+      break;
+    }
+
+    case multi_process_notification::UserDomain:
+      domain_string = StringPrintf("user.uid.%u.", getuid());
+      break;
+
+    case multi_process_notification::SystemDomain:
+      break;
+  }
+  return domain_string + bundle_id + "." + name;
+}
+
+bool UseLeopardSwitchboardThread() {
+#if USE_LEOPARD_SWITCHBOARD_THREAD
+  return true;
+#endif  // USE_LEOPARD_SWITCHBOARD_THREAD
+  int32 major_version, minor_version, bugfix_version;
+  base::SysInfo::OperatingSystemVersionNumbers(
+      &major_version, &minor_version, &bugfix_version);
+  return major_version <= 10 && minor_version <= 5;

[Mark Mentovai, 2011/01/12 22:42:36]

This comparison is wrong. It’s probably not wrong in a way that would affect
anything, but it’s so easy to write it correctly.

  return major_version < 10 || (major_version == 10 && minor_version <= 5);

+}
+
+}  // namespace
+
+namespace multi_process_notification {
+
+bool Post(const std::string& name, Domain domain) {
+  std::string notification = AddPrefixToNotification(name, domain);
+  uint32_t status = notify_post(notification.c_str());
+  DCHECK_EQ(status, static_cast<uint32_t>(NOTIFY_STATUS_OK));
+  return status == NOTIFY_STATUS_OK;
+}
+
+#if MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_5
+#error LeopardSwitchboardThread can be removed
+#endif  // MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_5
+
+// LeopardSwitchboardThread exists because the file descriptors returned by
+// notify_register_file_descriptor can't be monitored using kqueues on 10.5
+// ( http://openradar.appspot.com/8854692 ) and libevent uses kqueue to watch
+// file descriptors in IOMessageLoop.
+// This solution is to have a separate thread that monitors the file descriptor
+// returned by notify_register_file_descriptor using select, and then to
+// notify the MessageLoopForIO using a different file descriptor allocated by
+// socketpair that can be monitored using kqueues in libevent. This thread
+// only runs on 10.5, as 10.6 kqueues can monitor the notify file descriptors
+// without any problems.
+
+// LeopardSwitchboardThread creates three file descriptors:
+// internal_fd_: which communicates from the thread to external threads

[Mark Mentovai, 2011/01/12 22:42:36]

This is confusing because it introduces “external threads” without explaining
what they are, and because it doesn’t say what “thread” means. I’m assuming that
as you’ve used “thread” here, you mean the switchboard thread.

+// external_fd_: which communicates from external threads to the thread
+// notify_fd_: which is the file descriptor returned from
+//             notify_register_file_descriptor
+//
+// The thread itself sits in a select loop waiting on internal_fd_, and
+// notify_fd_ for input. If it gets ANY input on internal_fd_ it exits.
+// If it gets input on notify_fd_ it sends the input through to external_fd_.
+// External_fd_ is monitored by MessageLoopForIO so that the lookup of any
+// matching listeners in entries_, and the triggering of those listenerrs,
+// occurs in the MessageLoopForIO thread.
+//
+// Lookups are linear right now, and could be optimized if they ever become
+// a performance issue.
+class LeopardSwitchboardThread
+    : public base::MessagePumpLibevent::Watcher,
+      public base::SimpleThread,
+      public MessageLoop::DestructionObserver {
+ public:
+  LeopardSwitchboardThread();
+  virtual ~LeopardSwitchboardThread();
+
+  bool Init();
+
+  int AddListener(ListenerImpl* listener, const std::string& notification);
+  bool RemoveListener(ListenerImpl* listener, const std::string& notification);
+
+  bool finished() { return finished_; }

[Mark Mentovai, 2011/01/12 22:42:36]

  bool finished() const { return finished_; }

+
+  // SimpleThread overrides
+  virtual void Run();
+
+  // Watcher overrides
+  virtual void OnFileCanReadWithoutBlocking(int fd);
+  virtual void OnFileCanWriteWithoutBlocking(int fd);
+
+  // DestructionObserver overrides
+  virtual void WillDestroyCurrentMessageLoop();
+
+ private:
+  // Describe entries in our entries list for matching tokens to notifications

[Mark Mentovai, 2011/01/12 22:42:36]

“Describe entries in our entries list?” I don’t understand why you said
“describe” or why you repeated “entries” or, as always, why you even needed
“our.” I think “Used to match tokens with notifications and vice-versa.” is
enough of a comment.

+  // and vice-versa.
+  typedef struct {

[Mark Mentovai, 2011/01/12 22:42:36]

“typedef struct” is so C-ish. This can just be “struct SwitchboardEntry”.

+    int token_;
+    std::string notification_;
+    ListenerImpl* listener_;
+  } SwitchboardEntry;
+
+  enum {
+    kKillThreadMessage = 0xdecea5e

[Mark Mentovai, 2011/01/12 22:42:36]

I like this constant.

+  };
+
+  int internal_fd_;
+  int external_fd_;
+  int notify_fd_;
+  int notify_fd_token_;
+  mutable bool finished_;
+  fd_set fd_set_;
+
+  // all accesses to entries_ must be controlled by entries_lock_.
+  std::vector<SwitchboardEntry> entries_;
+  Lock entries_lock_;
+  base::MessagePumpLibevent::FileDescriptorWatcher watcher_;
+};
+
+class ListenerImpl : public base::MessagePumpLibevent::Watcher {
+ public:
+  ListenerImpl(const std::string& name,
+               Domain domain,
+               Listener::Delegate* delegate);
+  virtual ~ListenerImpl();
+
+  bool Start();
+  void OnListen();
+
+  // Watcher overrides
+  virtual void OnFileCanReadWithoutBlocking(int fd);
+  virtual void OnFileCanWriteWithoutBlocking(int fd);
+
+ private:
+  std::string name_;
+  Domain domain_;
+  Listener::Delegate* delegate_;
+  int fd_;
+  int token_;
+  Lock switchboard_lock_;
+  static LeopardSwitchboardThread* g_switchboard_thread_;
+  base::MessagePumpLibevent::FileDescriptorWatcher watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(ListenerImpl);
+};
+
+LeopardSwitchboardThread::LeopardSwitchboardThread()
+    : base::SimpleThread("LeopardSwitchboardThread"), internal_fd_(-1),
+      external_fd_(-1), notify_fd_(-1), notify_fd_token_(-1), finished_(false) {
+}
+
+LeopardSwitchboardThread::~LeopardSwitchboardThread() {
+  if (internal_fd_ != -1) {
+    close(internal_fd_);
+  }
+  if (external_fd_ != -1) {
+    close(external_fd_);
+  }
+
+  // Cancelling this notification takes care of closing notify_fd_.
+  uint32_t status = notify_cancel(notify_fd_token_);

[Mark Mentovai, 2011/01/12 22:42:36]

Is this safe if notify_register_file_descriptor in Init failed?

+  DCHECK_EQ(status, static_cast<uint32_t>(NOTIFY_STATUS_OK));
+}
+
+bool LeopardSwitchboardThread::Init() {
+  // Create a pair of sockets for communicating with the thread
+  // The file descriptors returned from socketpair can be kqueue'd on 10.5.
+  int sockets[2];
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
+    PLOG(ERROR) << "socketpair";
+    return false;
+  }
+  internal_fd_ = sockets[0];
+  external_fd_ = sockets[1];
+
+  // Register a bogus notification so that there is single notify_fd_ to
+  // monitor. This runs a small risk of overflowing the notification buffer
+  // if notifications are used heavily (see man 3 notify), however it greatly
+  // simplifies the select loop code as there are only 2 file descriptors
+  // that need to be monitored, and there is no need to add/remove file
+  // descriptors from fd_set_ as listeners are added and removed.
+  // This also keep the total fd usage on 10.5 to three for all
+  // notifications. The 10.6 implementation will use one fd per notification,
+  // but doesn't run the risk of notification buffer overflow. If fds ever
+  // become tight, the 10.6 code could be changed to use only one fd for
+  // all notifications.
+  std::string notification = StringPrintf("LeopardSwitchboardThread.%d",
+                                          getpid());
+  notification = AddPrefixToNotification(notification, ProfileDomain);
+  uint32_t status = notify_register_file_descriptor(
+      notification.c_str(), &notify_fd_, 0, &notify_fd_token_);
+  if (status != NOTIFY_STATUS_OK) {
+    return false;
+  }
+
+  FD_ZERO(&fd_set_);
+  FD_SET(internal_fd_, &fd_set_);
+  FD_SET(notify_fd_, &fd_set_);
+
+  MessageLoopForIO* io_loop = MessageLoopForIO::current();
+
+  // Watch for destruction of the MessageLoopForIO that this switchboard is
+  // registered on. If it gets destroyed the thread is joined and all current
+  // listeners become invalid. The next time a listener is created (on a new
+  // MessageLoopForIO) a new switchboard thread will be created. This isn't an
+  // issue in the app as there is always one (and only one) MessageLoopForIO,
+  // however unit tests create and kill MessageLoops with impunity and this case
+  // needs to be handled for tests to work.
+  io_loop->AddDestructionObserver(this);
+  return io_loop->WatchFileDescriptor(
+      external_fd_, true, MessageLoopForIO::WATCH_READ, &watcher_, this);
+}
+
+void LeopardSwitchboardThread::WillDestroyCurrentMessageLoop() {
+  DCHECK_EQ(MessageLoop::current(), MessageLoopForIO::current());
+  watcher_.StopWatchingFileDescriptor();
+
+  // Send the appropriate message to end our thread, and then wait for it

[Mark Mentovai, 2011/01/12 22:42:36]

our -> the

+  // to finish before continuing.
+  int message = kKillThreadMessage;
+  write(external_fd_, &message, sizeof(message));
+  Join();
+}
+
+void LeopardSwitchboardThread::Run() {
+  DCHECK(!finished_);
+  int nfds = (internal_fd_ > notify_fd_ ? internal_fd_ : notify_fd_) + 1;

[Mark Mentovai, 2011/01/12 22:42:36]

std::max from <algorithm>?

+  while (1) {
+    fd_set working_set;
+    FD_COPY(&fd_set_, &working_set);
+    int count = HANDLE_EINTR(select(nfds, &working_set, NULL, NULL, NULL));
+    if (count == 0) continue;

[Mark Mentovai, 2011/01/12 22:42:36]

I wonder if you actually expect this to happen without a timeout.

In any case, I’d put the error handling first (if (count < 0)) and then handle
the unconventional case (if (count == 0)) immediately afterwards. And I’d break
the continue out onto its own line. This is already a pretty tall function, one
more line won’t kill it.

+    if (count < 0) {
+      PLOG(ERROR) << "Exiting LeopardSwitchboardThread. Select error.";

[Mark Mentovai, 2011/01/12 22:42:36]

The messages in this function are very verbose. All of this cstring data needs
to ride along with the executable. Bloatish. Can you trim them down a bit? Keep
in mind that the LOG family will already include a filename and line number
which helps contribute to the bloat, but which will also help determine what’s
going on if you get a log snippet. This might be OK as
  PLOG(ERROR) << "select";

+      break;
+    }
+    if (FD_ISSET(notify_fd_, &working_set)) {
+      int token;
+      int status = HANDLE_EINTR(read(notify_fd_, &token, sizeof(token)));
+      if (status < 0) {
+        PLOG(ERROR) << "Exiting LeopardSwitchboardThread. Read error.";
+        break;
+      } else if (status == 0) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. Notify fd closed.";
+        break;
+      } else if (status != sizeof(token)) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. "
+                   << "Read from notify wrong size: " << status;
+        break;
+      } else if (token == notify_fd_token_) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. Invalid token sent: "

[Mark Mentovai, 2011/01/12 22:42:36]

This LOG doesn’t seem to reflect what actually happens. The thread doesn’t
actually exit.

Causing the thread to exit seems excessively punitive, anyway.

I think I’m OK with this being a LOG(WARNING) and then proceeding to write to
internal_fd_.

+                   << token;
+      }
+      status = HANDLE_EINTR(write(internal_fd_, &token, sizeof(token)));
+      if (status < 0) {
+        PLOG(ERROR) << "write";

[Mark Mentovai, 2011/01/12 22:42:36]

You gave flowery descriptions of the rest of your errors, including a note that
LeopardSwitchboardThread was exiting. This one just says “write”?

(Per the above comment, I kind of prefer the short messages, but I’m pointing
out the inconsistency in case you really really really really really like the
long ones and want to keep them.)

+        break;
+      } else if (status == 0) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. External_fd_ closed.";

[Mark Mentovai, 2011/01/12 22:42:36]

Don’t capitalize variable names just because they begin sentences.

+        break;
+      } else if (status != sizeof(token)) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. "
+                   << "Write from notify wrong size: " << status;

[Mark Mentovai, 2011/01/12 22:42:36]

“Read from notify” made sense, because I read it as “read from notify_fd_.” What
does “write from notify” mean?

+        break;
+      }
+    }
+    if (FD_ISSET(internal_fd_, &working_set)) {
+      int value = -1;

[Mark Mentovai, 2011/01/12 22:42:36]

No need to initialize this.

+      int status = HANDLE_EINTR(read(internal_fd_, &value, sizeof(value)));
+      if (status < 0) {
+        PLOG(ERROR) << "Exiting LeopardSwitchboardThread. Read error.";
+      } else if (status == 0) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. Internal_fd_ closed.";

[Mark Mentovai, 2011/01/12 22:42:36]

Don’t capitalize variable names just because they begin new sentences.

+      } else if (value != kKillThreadMessage) {
+        LOG(ERROR) << "Exiting LeopardSwitchboardThread. Unknown message sent: "
+                   << value;
+      }
+      break;
+    }
+  }
+  finished_ = true;
+}
+
+int LeopardSwitchboardThread::AddListener(ListenerImpl* listener,
+                                          const std::string& notification) {
+  DCHECK(!finished());
+  base::AutoLock autolock(entries_lock_);
+  for (std::vector<SwitchboardEntry>::iterator i = entries_.begin();
+       i < entries_.end(); ++i) {
+    if (i->listener_ == listener && i->notification_ == notification) {
+      LOG(ERROR) << "Listener " << listener
+                 << " already registered for '" << notification << "'.";
+      return -1;

[Mark Mentovai, 2011/01/12 22:42:36]

Are tokens just opaque junk? If so, is -1 a possible valid token value? And if
so, find another way to do this: return a bool and pass the token back in an out
parameter?

+    }
+  }
+  int token = -1;
+  uint32_t status = notify_register_file_descriptor(
+     notification.c_str(), &notify_fd_, NOTIFY_REUSE, &token);
+  if (status != NOTIFY_STATUS_OK) {
+    LOG(ERROR) << "Unable to notify_register_file_descriptor for '"
+               << notification << "' status: " << status;
+    return -1;
+  }
+  SwitchboardEntry entry;
+  entry.token_ = token;
+  entry.notification_ = notification;
+  entry.listener_ = listener;
+  entries_.push_back(entry);
+  return token;
+}
+
+bool LeopardSwitchboardThread::RemoveListener(ListenerImpl* listener,
+                                              const std::string& notification) {
+  DCHECK(!finished());
+  base::AutoLock autolock(entries_lock_);
+  for (std::vector<SwitchboardEntry>::iterator i = entries_.begin();
+       i < entries_.end(); ++i) {
+    if (i->listener_ == listener && i->notification_ == notification) {
+      uint32_t status = notify_cancel(i->token_);
+      DCHECK_EQ(status, static_cast<uint32_t>(NOTIFY_STATUS_OK));
+      entries_.erase(i);
+      return true;
+    }
+  }
+  LOG(ERROR) << "Unable to remove listener '" << listener
+             << "' for '" << notification << "'.";
+  return false;
+}
+
+void LeopardSwitchboardThread::OnFileCanReadWithoutBlocking(int fd) {
+  DCHECK_EQ(fd, external_fd_);
+  int token = 0;
+  if (HANDLE_EINTR(read(external_fd_, &token, sizeof(token))) >= 0) {

[Mark Mentovai, 2011/01/12 22:42:36]

Shouldn’t happen, but this should really check |== sizeof(token)|. |>= 0| will
catch a short read (which would be really odd) and EOF (which is less odd and
might be triggered by other bugs).

+    // Have to swap to native endianness <http://openradar.appspot.com/8821081>.
+    token = static_cast<int>(ntohl(token));
+    base::AutoLock autolock(entries_lock_);
+    bool found_token = false;
+    for (std::vector<SwitchboardEntry>::iterator i = entries_.begin();
+         i < entries_.end(); ++i) {
+      if (i->token_ == token) {
+        found_token = true;
+        i->listener_->OnListen();
+      }
+    }
+    if (!found_token) {
+      LOG(ERROR) << "read unknown token " << token;
+    }
+  }
+}
+
+void LeopardSwitchboardThread::OnFileCanWriteWithoutBlocking(int fd) {
+  NOTREACHED();
+}
+
+LeopardSwitchboardThread* ListenerImpl::g_switchboard_thread_ = NULL;
+
+ListenerImpl::ListenerImpl(
+    const std::string& name, Domain domain, Listener::Delegate* delegate)
+    : name_(name), domain_(domain), delegate_(delegate), fd_(-1), token_(-1) {
+}
+
+ListenerImpl::~ListenerImpl() {
+  if (!UseLeopardSwitchboardThread()) {
+    if (token_ != -1) {
+      uint32_t status = notify_cancel(token_);
+      DCHECK_EQ(status, static_cast<uint32_t>(NOTIFY_STATUS_OK));
+      token_ = -1;
+    }
+  }
+}
+
+bool ListenerImpl::Start() {
+  DCHECK_EQ(fd_, -1);
+  DCHECK_EQ(token_, -1);
+  std::string notification = AddPrefixToNotification(name_, domain_);
+
+  // On 10.5 use a LeopardSwitchboardThread, on 10.6 use notifications
+  // directly. See comment above the LeopardSwitchboardThread class declaration
+  // for a more detailed explanation.
+  if (UseLeopardSwitchboardThread()) {
+    if (!g_switchboard_thread_ || g_switchboard_thread_->finished()) {

[Mark Mentovai, 2011/01/12 22:42:36]

This is a little ugly. Maybe the switchboard class should have a static function
like GetSwitchboardThread, whose job it is to create and start the thread if it
doesn’t already exist and if it isn’t already running.

+      base::AutoLock autolock(switchboard_lock_);
+      if (g_switchboard_thread_ && g_switchboard_thread_->finished()) {
+        delete g_switchboard_thread_;

[Mark Mentovai, 2011/01/12 22:42:36]

I haven’t thought too much about this, but I’ll ask you to:

Is it possible to delete this while someone else might still be using it?

Can finished_ be true but the object still exist, taking notifications like
OnFileCanReadWithoutBlocking? That might get racy.

+        g_switchboard_thread_ = NULL;
+      }
+      if (!g_switchboard_thread_) {
+        g_switchboard_thread_ = new LeopardSwitchboardThread();
+        if (!g_switchboard_thread_->Init()) {
+          delete g_switchboard_thread_;
+          g_switchboard_thread_ = NULL;
+          LOG(ERROR) << "Unable to start switchboard thread";
+          return false;
+        } else {
+          g_switchboard_thread_->Start();
+        }
+      }
+    }
+    return g_switchboard_thread_->AddListener(this, notification);
+  } else {
+    uint32_t status = notify_register_file_descriptor(
+        notification.c_str(), &fd_, 0, &token_);
+    if (status != NOTIFY_STATUS_OK) {
+      LOG(ERROR) << "Unable to notify_register_file_descriptor for '"
+                 << notification << "' Status: " << status;
+      return false;
+    }
+
+    MessageLoopForIO* io_loop = MessageLoopForIO::current();
+    return io_loop->WatchFileDescriptor(
+        fd_, true, MessageLoopForIO::WATCH_READ, &watcher_, this);
+  }
+}
+
+void ListenerImpl::OnFileCanReadWithoutBlocking(int fd) {
+  DCHECK(!UseLeopardSwitchboardThread());
+  DCHECK_EQ(fd, fd_);
+  int token = 0;
+  if (HANDLE_EINTR(read(fd_, &token, sizeof(token))) >= 0) {
+    // Have to swap to native endianness <http://openradar.appspot.com/8821081>.
+    token = static_cast<int>(ntohl(token));
+    if (token == token_) {
+      delegate_->OnNotificationReceived(name_, domain_);
+    } else {
+      LOG(ERROR) << "Unexpected value " << token;
+    }
+  }
+}
+
+void ListenerImpl::OnListen() {
+  DCHECK(UseLeopardSwitchboardThread());
+  delegate_->OnNotificationReceived(name_, domain_);
+}
+
+void ListenerImpl::OnFileCanWriteWithoutBlocking(int fd) {
+  NOTREACHED();
+}
+
+Listener::Listener(
+    const std::string& name, Domain domain, Listener::Delegate* delegate)
+    : impl_(new ListenerImpl(name, domain, delegate)) {
+}
+
+Listener::~Listener() {
+}
+
+bool Listener::Start() {
+  return impl_->Start();
+}
+
+}  // namespace multi_process_notification