Provide an iOS message pump for IO implementation.

base/message_pump_io_ios.mm

+// Copyright 2012 The Chromium Authors. All rights reserved.

[wtc, 2012/11/21 18:31:05]

This file is named message_pump_io_ios.mm, but I think
it is pure C++ code. I suggest renaming it
message_pump_io_ios.cc to make that clear.

[stuartmorgan, 2012/11/21 21:16:13]

AFAIK our policy has always been to name Mac (and for the same reason, iOS)
files .mm, under the theory that a Mac/iOS specific file might pick up ObjC code
at any time, and having to rename a file just to add a line of two of code is a
colossal pain.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/message_pump_io_ios.h"
+
+namespace base {
+
+MessagePumpIOSForIO::FileDescriptorWatcher::FileDescriptorWatcher()
+    : is_persistent_(false),
+      fdref_(NULL),
+      callback_types_(0),
+      fd_source_(NULL),
+      pump_(NULL),
+      watcher_(NULL) {
+}
+
+MessagePumpIOSForIO::FileDescriptorWatcher::~FileDescriptorWatcher() {
+  StopWatchingFileDescriptor();
+}
+
+bool MessagePumpIOSForIO::FileDescriptorWatcher::StopWatchingFileDescriptor() {
+  if (fdref_ == NULL)
+    return true;
+
+  CFFileDescriptorDisableCallBacks(fdref_, callback_types_);
+  pump_->RemoveRunLoopSource(fd_source_);
+  fd_source_.reset();
+  fdref_.reset();
+  callback_types_ = 0;
+  pump_ = NULL;
+  watcher_ = NULL;
+  return true;
+}
+
+void MessagePumpIOSForIO::FileDescriptorWatcher::Init(
+    CFFileDescriptorRef fdref,
+    CFOptionFlags callback_types,
+    CFRunLoopSourceRef fd_source,
+    bool is_persistent) {
+  DCHECK(fdref);
+  DCHECK(!fdref_);
+
+  is_persistent_ = is_persistent;
+  fdref_.reset(fdref);
+  callback_types_ = callback_types;
+  fd_source_.reset(fd_source);
+}
+
+void MessagePumpIOSForIO::FileDescriptorWatcher::OnFileCanReadWithoutBlocking(
+    int fd,
+    MessagePumpIOSForIO* pump) {

[Mark Mentovai, 2012/11/20 22:29:27]

Do you want to DCHECK that you’re really listening for read events if you enter
this function? Likewise for write events in the next function.

[blundell, 2012/11/21 16:56:52]

Done.

+  pump->WillProcessIOEvent();
+  watcher_->OnFileCanReadWithoutBlocking(fd);
+  pump->DidProcessIOEvent();
+}
+
+void MessagePumpIOSForIO::FileDescriptorWatcher::OnFileCanWriteWithoutBlocking(
+    int fd,
+    MessagePumpIOSForIO* pump) {
+  pump->WillProcessIOEvent();
+  watcher_->OnFileCanWriteWithoutBlocking(fd);
+  pump->DidProcessIOEvent();
+}
+
+MessagePumpIOSForIO::MessagePumpIOSForIO() {
+}
+
+MessagePumpIOSForIO::~MessagePumpIOSForIO() {
+}
+
+bool MessagePumpIOSForIO::WatchFileDescriptor(
+    int fd,
+    bool persistent,
+    Mode mode,
+    FileDescriptorWatcher *controller,
+    Watcher *delegate) {
+  DCHECK_GE(fd, 0);
+  DCHECK(controller);
+  DCHECK(delegate);
+  DCHECK(mode == WATCH_READ || mode == WATCH_WRITE ||
+         mode == (WATCH_READ | WATCH_WRITE));
+
+  // WatchFileDescriptor should be called on the pump thread. It is not
+  // threadsafe, and your watcher may never be registered.
+  DCHECK(watch_file_descriptor_caller_checker_.CalledOnValidThread());
+
+  CFFileDescriptorContext source_context = {0};
+  source_context.info = controller;
+
+  CFOptionFlags callback_types = 0;
+  if ((mode & WATCH_READ) != 0) {
+    callback_types |= kCFFileDescriptorReadCallBack;
+  }
+  if ((mode & WATCH_WRITE) != 0) {
+    callback_types |= kCFFileDescriptorWriteCallBack;
+  }
+
+  CFFileDescriptorRef fdref = controller->fdref_;
+  if (fdref == NULL) {
+    base::mac::ScopedCFTypeRef<CFFileDescriptorRef> scoped_fdref(
+        CFFileDescriptorCreate(kCFAllocatorDefault, fd, false, HandleFdIOEvent,
+                               &source_context));
+    if (scoped_fdref == NULL) {
+      NOTREACHED() << "CFFileDescriptorCreate failed";
+      return false;
+    }
+
+    CFFileDescriptorEnableCallBacks(scoped_fdref, callback_types);
+
+    // TODO(wtc): what should the 'order' argument be?
+    base::mac::ScopedCFTypeRef<CFRunLoopSourceRef> scoped_fd_source(
+        CFFileDescriptorCreateRunLoopSource(kCFAllocatorDefault,
+                                            scoped_fdref,
+                                            0));
+    if (scoped_fd_source == NULL) {
+      NOTREACHED() << "CFFileDescriptorCreateRunLoopSource failed";
+      return false;
+    }
+    CFRunLoopAddSource(run_loop(), scoped_fd_source, kCFRunLoopCommonModes);
+
+    // Transfer ownership of scoped_fdref and fd_source to controller.
+    controller->Init(scoped_fdref.release(), callback_types,
+                     scoped_fd_source.release(), persistent);
+  } else {
+    // It's illegal to use this function to listen on 2 separate fds with the
+    // same |controller|.
+    if (CFFileDescriptorGetNativeDescriptor(fdref) != fd) {
+      NOTREACHED() << "FDs don't match: "
+                   << CFFileDescriptorGetNativeDescriptor(fdref)
+                   << " != " << fd;
+      return false;
+    }
+    if (persistent != controller->is_persistent_) {
+      NOTREACHED() << "persistent doesn't match";
+      return false;
+    }
+
+    // Combine old/new event masks.
+    CFFileDescriptorDisableCallBacks(fdref, controller->callback_types_);
+    controller->callback_types_ |= callback_types;

[Mark Mentovai, 2012/11/20 22:29:27]

Does this “else” block ever get entered in practice? It’d be easier to just not
support this.

If this does need to happen in practice:

Who documents that the right thing to do here is |= (add) instead of =
(replace)?

[blundell, 2012/11/21 16:56:52]

I don't know whether it ever gets entered in practice. These semantics match
those of the corresponding function in message_pump_libevent, and are documented
at the function declaration in message_pump_io_ios.h. It seems to me like it's
worth keeping consistent semantics between the two implementations.

On 2012/11/20 22:29:27, Mark Mentovai wrote:

+    CFFileDescriptorEnableCallBacks(fdref, controller->callback_types_);
+  }
+
+  controller->set_watcher(delegate);
+  controller->set_pump(this);
+
+  return true;
+}
+
+void MessagePumpIOSForIO::RemoveRunLoopSource(CFRunLoopSourceRef source) {
+  CFRunLoopRemoveSource(run_loop(), source, kCFRunLoopCommonModes);
+}
+
+void MessagePumpIOSForIO::AddIOObserver(IOObserver *obs) {
+  io_observers_.AddObserver(obs);
+}
+
+void MessagePumpIOSForIO::RemoveIOObserver(IOObserver *obs) {
+  io_observers_.RemoveObserver(obs);
+}
+
+void MessagePumpIOSForIO::WillProcessIOEvent() {
+  FOR_EACH_OBSERVER(IOObserver, io_observers_, WillProcessIOEvent());
+}
+
+void MessagePumpIOSForIO::DidProcessIOEvent() {
+  FOR_EACH_OBSERVER(IOObserver, io_observers_, DidProcessIOEvent());
+}
+
+// static
+void MessagePumpIOSForIO::HandleFdIOEvent(CFFileDescriptorRef fdref,
+                                          CFOptionFlags callback_types,
+                                          void* info) {
+  int fd = CFFileDescriptorGetNativeDescriptor(fdref);
+
+  FileDescriptorWatcher* controller =
+      static_cast<FileDescriptorWatcher*>(info);
+
+  CHECK_EQ(fdref, controller->fdref_);

[Mark Mentovai, 2012/11/20 22:29:27]

What justifies this being a CHECK and not a DCHECK? Same on line 194.

[blundell, 2012/11/21 16:56:52]

Changed.

On 2012/11/20 22:29:27, Mark Mentovai wrote:

[wtc, 2012/11/21 18:31:05]

I may have used a CHECK to try to expose bugs using release
builds. Since this code has been in use for a long time, it
is fine to lower the CHECK to a DCHECK.

+  bool persistent = controller->is_persistent_;
+
+  MessagePumpIOSForIO* pump = controller->pump();
+  if (callback_types & kCFFileDescriptorWriteCallBack) {

[Mark Mentovai, 2012/11/20 22:29:27]

Your style in this file has varied between (v & k) and ((v & k) != 0), as on
lines 92 and 95. My preference is for this form, but it doesn’t matter what I
like better as long as you’re consistent.

[blundell, 2012/11/21 16:56:52]

Changed to consistently use the (v & k) form both here and in
message_pump_libevent.

On 2012/11/20 22:29:27, Mark Mentovai wrote:

+    controller->OnFileCanWriteWithoutBlocking(fd, pump);
+  }
+  if (callback_types & kCFFileDescriptorReadCallBack) {
+    controller->OnFileCanReadWithoutBlocking(fd, pump);

[wtc, 2012/11/21 18:31:05]

I may have filed a bug report about this. |controller| may
have been deleted during the controller->OnFileCanWriteWithoutBlocking()
call above, so in general this could be problematic.

I don't know what's the best solution, and why this hasn't
affected us. willchan used the "weak pointer" to solve a
similar problem in SSLClientSocketNSS.

+  }
+
+  // Must read/write from the fd before re-enabling the callbacks.
+  // |controller| may have been deleted, so we test a copy of
+  // controller->persistent.  |fdref| may have been invalidated.

[Mark Mentovai, 2012/11/20 22:29:27]

If you’re concerned that controller may be gone but you go ahead and re-enable
callbacks anyway, then the next time the callback runs, controller will be, um,
gone. Therefore I either don’t understand the paranoia that the comment
expresses, or I don’t understand why similar paranoia hasn’t been applied to the
code itself.

I haven’t done a full analysis of the flow so this may be a silly question. You
might be able to answer it just by explaining it, without having to change any
code at all.

[blundell, 2012/11/21 16:56:52]

My understanding is that the |persistent| argument to |WatchFileDescriptor()|
specifies whether the controller is persistent. If the controller is persistent,
then it is safe to re-enable the callback. The problem is that if the controller
is not persistent it might have already gone away by the time we do the check,
hence the caching of the value. Unfortunately, that argument is not documented
in either the header of this file or message_pump_libevent.h. Wan-Teh, can you
confirm?
On 2012/11/20 22:29:27, Mark Mentovai wrote:

[wtc, 2012/11/21 18:31:05]

At this point, |controller| may have been destroyed during
the controller->OnFileCanWriteWithoutBlocking() or
controller->OnFileCanReadWithoutBlocking() call.

Note that even a persistent |controller| may have been
destroyed, if its job is done.

I think Mark pointed out a valid problem with this code
(lines 195-196). I suspect that if CFFileDescriptorIsValid(fdref)
is true, then |controller| is still alive, which is why
it is valid to check |persistent|, and should be equally
valid to just check |controller->persistent| directly.
I just studied this code and I believe this is true.
This can be verified with some debugging code. When I
worked on this code, I added a |valid| member to
|controller| and used that to check if |controller| was
still alive. You can use the same technique.

Proof:

CFFileDescriptorInvalidate is only called by
the destructor or the reset() method of the
ScopedCFFileDescriptorRef class.

ScopedCFFileDescriptorRef is only used as the fdref_ member of
FileDescriptorWatcher (the |controller|).

Therefore, if CFFileDescriptorIsValid(fdref) is true,
it should imply that the containing |controller| has not
called reset() or destroyed its fdref_ member, so
|controller| should still be valid.

+  CHECK_GT(CFGetRetainCount(fdref), 0);
+  if (CFFileDescriptorIsValid(fdref) && persistent)
+    CFFileDescriptorEnableCallBacks(fdref, callback_types);
+}
+
+}  // namespace base