Handle object deletion in FileDescriptorWatcher callback.

BUG=88134
TEST=MessagePumpLibeventTest.*


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

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

3 files changed, 112 insertions, 17 deletions

diff --git a/base/message_pump_libevent.cc b/base/message_pump_libevent.cc
index c48f490..3e3facf 100644
--- a/base/message_pump_libevent.cc
+++ b/base/message_pump_libevent.cc
@@ -8,6 +8,7 @@
 #include <fcntl.h>
 
 #include "base/auto_reset.h"
+#include "base/compiler_specific.h"
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
@@ -53,7 +54,7 @@ MessagePumpLibevent::FileDescriptorWatcher::FileDescriptorWatcher()
     : is_persistent_(false),
       event_(NULL),
       pump_(NULL),
-      watcher_(NULL) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 MessagePumpLibevent::FileDescriptorWatcher::~FileDescriptorWatcher() {
@@ -92,6 +93,10 @@ event *MessagePumpLibevent::FileDescriptorWatcher::ReleaseEvent() {
 
 void MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking(
     int fd, MessagePumpLibevent* pump) {
+  // Since OnFileCanWriteWithoutBlocking() gets called first, it can stop
+  // watching the file descriptor.
+  if (!watcher_)
+    return;
   pump->WillProcessIOEvent();
   watcher_->OnFileCanReadWithoutBlocking(fd);
   pump->DidProcessIOEvent();
@@ -99,6 +104,7 @@ void MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking(
 
 void MessagePumpLibevent::FileDescriptorWatcher::OnFileCanWriteWithoutBlocking(
     int fd, MessagePumpLibevent* pump) {
+  DCHECK(watcher_);
   pump->WillProcessIOEvent();
   watcher_->OnFileCanWriteWithoutBlocking(fd);
   pump->DidProcessIOEvent();
@@ -334,8 +340,9 @@ bool MessagePumpLibevent::Init() {
 // static
 void MessagePumpLibevent::OnLibeventNotification(int fd, short flags,
                                                  void* context) {
-  FileDescriptorWatcher* controller =
-      static_cast<FileDescriptorWatcher*>(context);
+  base::WeakPtr<FileDescriptorWatcher> controller =
+      static_cast<FileDescriptorWatcher*>(context)->weak_factory_.GetWeakPtr();
+  DCHECK(controller.get());
 
   MessagePumpLibevent* pump = controller->pump();
   pump->processed_io_events_ = true;
@@ -343,7 +350,8 @@ void MessagePumpLibevent::OnLibeventNotification(int fd, short flags,
   if (flags & EV_WRITE) {
     controller->OnFileCanWriteWithoutBlocking(fd, pump);
   }
-  if (flags & EV_READ) {
+  // Check |controller| in case it's been deleted in this callback.
+  if (controller.get() && flags & EV_READ) {
     controller->OnFileCanReadWithoutBlocking(fd, pump);
   }
 }
diff --git a/base/message_pump_libevent.h b/base/message_pump_libevent.h
index c08a4dc..fd76379 100644
--- a/base/message_pump_libevent.h
+++ b/base/message_pump_libevent.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include "base/basictypes.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_pump.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
@@ -37,8 +38,10 @@ class BASE_API MessagePumpLibevent : public MessagePump {
     virtual ~IOObserver() {}
   };
 
-  // Used with WatchFileDescptor to asynchronously monitor the I/O readiness of
-  // a File Descriptor.
+  class FileDescriptorWatcher;
+
+  // Used with WatchFileDescriptor to asynchronously monitor the I/O readiness
+  // of a file descriptor.
   class Watcher {
    public:
     virtual ~Watcher() {}
@@ -63,6 +66,7 @@ class BASE_API MessagePumpLibevent : public MessagePump {
 
    private:
     friend class MessagePumpLibevent;
+    friend class MessagePumpLibeventTest;
 
     // Called by MessagePumpLibevent, ownership of |e| is transferred to this
     // object.
@@ -83,6 +87,7 @@ class BASE_API MessagePumpLibevent : public MessagePump {
     event* event_;
     MessagePumpLibevent* pump_;
     Watcher* watcher_;
+    base::WeakPtrFactory<FileDescriptorWatcher> weak_factory_;
 
     DISALLOW_COPY_AND_ASSIGN(FileDescriptorWatcher);
   };
@@ -124,6 +129,8 @@ class BASE_API MessagePumpLibevent : public MessagePump {
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time);
 
  private:
+  friend class MessagePumpLibeventTest;
+
   void WillProcessIOEvent();
   void DidProcessIOEvent();
 
diff --git a/base/message_pump_libevent_unittest.cc b/base/message_pump_libevent_unittest.cc
index 7c6f397..2a6d7bc 100644
--- a/base/message_pump_libevent_unittest.cc
+++ b/base/message_pump_libevent_unittest.cc
@@ -8,19 +8,26 @@
 
 #include "base/message_loop.h"
 #include "base/threading/thread.h"
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace {
+#if defined(USE_SYSTEM_LIBEVENT)
+#include <event.h>
+#else
+#include "third_party/libevent/event.h"
+#endif
+
+namespace base {
 
 class MessagePumpLibeventTest : public testing::Test {
- public:
+ protected:
   MessagePumpLibeventTest()
       : ui_loop_(MessageLoop::TYPE_UI),
         io_thread_("MessagePumpLibeventTestIOThread") {}
   virtual ~MessagePumpLibeventTest() {}
 
   virtual void SetUp() {
-    base::Thread::Options options(MessageLoop::TYPE_IO, 0);
+    Thread::Options options(MessageLoop::TYPE_IO, 0);
     ASSERT_TRUE(io_thread_.StartWithOptions(options));
     ASSERT_EQ(MessageLoop::TYPE_IO, io_thread_.message_loop()->type());
   }
@@ -30,15 +37,21 @@ class MessagePumpLibeventTest : public testing::Test {
     return static_cast<MessageLoopForIO*>(io_thread_.message_loop());
   }
 
- private:
+  void OnLibeventNotification(
+      MessagePumpLibevent* pump,
+      MessagePumpLibevent::FileDescriptorWatcher* controller) {
+    pump->OnLibeventNotification(0, EV_WRITE | EV_READ, controller);
+  }
+
   MessageLoop ui_loop_;
-  base::Thread io_thread_;
-  DISALLOW_COPY_AND_ASSIGN(MessagePumpLibeventTest);
+  Thread io_thread_;
 };
 
-// Concrete implementation of base::MessagePumpLibevent::Watcher that does
+namespace {
+
+// Concrete implementation of MessagePumpLibevent::Watcher that does
 // nothing useful.
-class StupidWatcher : public base::MessagePumpLibevent::Watcher {
+class StupidWatcher : public MessagePumpLibevent::Watcher {
  public:
   virtual ~StupidWatcher() {}
 
@@ -47,14 +60,12 @@ class StupidWatcher : public base::MessagePumpLibevent::Watcher {
   virtual void OnFileCanWriteWithoutBlocking(int fd) {}
 };
 
-}  // namespace
-
 #if GTEST_HAS_DEATH_TEST
 
 // Test to make sure that we catch calling WatchFileDescriptor off of the
 // wrong thread.
 TEST_F(MessagePumpLibeventTest, TestWatchingFromBadThread) {
-  base::MessagePumpLibevent::FileDescriptorWatcher watcher;
+  MessagePumpLibevent::FileDescriptorWatcher watcher;
   StupidWatcher delegate;
 
   ASSERT_DEBUG_DEATH(io_loop()->WatchFileDescriptor(
@@ -64,3 +75,72 @@ TEST_F(MessagePumpLibeventTest, TestWatchingFromBadThread) {
 }
 
 #endif  // GTEST_HAS_DEATH_TEST
+
+class DeleteWatcher : public MessagePumpLibevent::Watcher {
+ public:
+  explicit DeleteWatcher(
+      MessagePumpLibevent::FileDescriptorWatcher* controller)
+      : controller_(controller) {
+    DCHECK(controller_);
+  }
+  virtual ~DeleteWatcher() {}
+
+  // base:MessagePumpLibevent::Watcher interface
+  virtual void OnFileCanReadWithoutBlocking(int /* fd */) {
+    NOTREACHED();
+  }
+  virtual void OnFileCanWriteWithoutBlocking(int /* fd */) {
+    delete controller_;
+  }
+
+ private:
+  MessagePumpLibevent::FileDescriptorWatcher* const controller_;
+};
+
+TEST_F(MessagePumpLibeventTest, DeleteWatcher) {
+  scoped_refptr<MessagePumpLibevent> pump(new MessagePumpLibevent);
+  MessagePumpLibevent::FileDescriptorWatcher* watcher =
+      new MessagePumpLibevent::FileDescriptorWatcher;
+  DeleteWatcher delegate(watcher);
+  pump->WatchFileDescriptor(
+      0, false, MessagePumpLibevent::WATCH_READ_WRITE, watcher, &delegate);
+
+  // Spoof a libevent notification.
+  OnLibeventNotification(pump, watcher);
+}
+
+class StopWatcher : public MessagePumpLibevent::Watcher {
+ public:
+  explicit StopWatcher(
+      MessagePumpLibevent::FileDescriptorWatcher* controller)
+      : controller_(controller) {
+    DCHECK(controller_);
+  }
+  virtual ~StopWatcher() {}
+
+  // base:MessagePumpLibevent::Watcher interface
+  virtual void OnFileCanReadWithoutBlocking(int /* fd */) {
+    NOTREACHED();
+  }
+  virtual void OnFileCanWriteWithoutBlocking(int /* fd */) {
+    controller_->StopWatchingFileDescriptor();
+  }
+
+ private:
+  MessagePumpLibevent::FileDescriptorWatcher* const controller_;
+};
+
+TEST_F(MessagePumpLibeventTest, StopWatcher) {
+  scoped_refptr<MessagePumpLibevent> pump(new MessagePumpLibevent);
+  MessagePumpLibevent::FileDescriptorWatcher watcher;
+  StopWatcher delegate(&watcher);
+  pump->WatchFileDescriptor(
+      0, false, MessagePumpLibevent::WATCH_READ_WRITE, &watcher, &delegate);
+
+  // Spoof a libevent notification.
+  OnLibeventNotification(pump, &watcher);
+}
+
+}  // namespace
+
+}  // namespace base