StopWatchingFileDescriptor needs to free struct event.

Also, there's no point in using scoped_ptr for event_ anymore,
so removed that.
Should fix http://crbug.com/10503 "Crash in network layer"


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

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

base/message_loop_unittest.cc

@@ -13,6 +13,9 @@
 #include "base/message_pump_win.h"
 #include "base/scoped_handle.h"
 #endif
+#if defined(OS_POSIX)
+#include "base/message_pump_libevent.h"
+#endif
 
 using base::Thread;
 using base::Time;
@@ -1381,3 +1384,75 @@ TEST(MessageLoopTest, WaitForIO) {
   RunTest_WaitForIO();
 }
 #endif  // defined(OS_WIN)
+
+#if defined(OS_POSIX)
+
+namespace {
+
+class QuitDelegate : public
+    base::MessagePumpLibevent::Watcher {
+ public:
+  virtual void OnFileCanWriteWithoutBlocking(int fd) {
+    MessageLoop::current()->Quit();
+  }
+  virtual void OnFileCanReadWithoutBlocking(int fd) {
+    MessageLoop::current()->Quit();
+  }
+};
+
+}  // namespace
+
+TEST(MessageLoopTest, DISABLED_FileDescriptorWatcherOutlivesMessageLoop) {
+  // Simulate a MessageLoop that dies before an FileDescriptorWatcher.
+  // This could happen when people use the Singleton pattern or atexit.
+  // This is disabled for now because it fails (valgrind shows
+  // invalid reads), and it's not clear any code relies on this...
+  // TODO(dkegel): enable if it turns out we rely on this
+
+  // Create a file descriptor.  Doesn't need to be readable or writable,
+  // as we don't need to actually get any notifications.
+  // pipe() is just the easiest way to do it.
+  int pipefds[2];
+  int err = pipe(pipefds);
+  ASSERT_TRUE(err == 0);
+  int fd = pipefds[1];
+  {
+    // Arrange for controller to live longer than message loop.
+    base::MessagePumpLibevent::FileDescriptorWatcher controller;
+    {
+      MessageLoopForIO message_loop;
+
+      QuitDelegate delegate;
+      message_loop.WatchFileDescriptor(fd,
+          true, MessageLoopForIO::WATCH_WRITE, &controller, &delegate);
+      // and don't run the message loop, just destroy it.
+    }
+  }
+  close(pipefds[0]);
+  close(pipefds[1]);
+}
+
+TEST(MessageLoopTest, FileDescriptorWatcherDoubleStop) {
+  // Verify that it's ok to call StopWatchingFileDescriptor().
+  // (Errors only showed up in valgrind.)
+  int pipefds[2];
+  int err = pipe(pipefds);
+  ASSERT_TRUE(err == 0);
+  int fd = pipefds[1];
+  {
+    // Arrange for message loop to live longer than controller.
+    MessageLoopForIO message_loop;
+    {
+      base::MessagePumpLibevent::FileDescriptorWatcher controller;
+
+      QuitDelegate delegate;
+      message_loop.WatchFileDescriptor(fd,
+          true, MessageLoopForIO::WATCH_WRITE, &controller, &delegate);
+      controller.StopWatchingFileDescriptor();
+    }
+  }
+  close(pipefds[0]);
+  close(pipefds[1]);
+}
+
+#endif  // defined(OS_LINUX)

base/message_pump_libevent.cc

@@ -9,9 +9,28 @@
 
 #include "base/logging.h"
 #include "base/scoped_nsautorelease_pool.h"
+#include "base/scoped_ptr.h"
 #include "base/time.h"
 #include "third_party/libevent/event.h"
 
+// Lifecycle of struct event
+// Libevent uses two main data structures:
+// struct event_base (of which there is one per message pump), and
+// struct event (of which there is roughly one per socket).
+// The socket's struct event is created in
+// MessagePumpLibevent::WatchFileDescriptor(),
+// is owned by the FileDescriptorWatcher, and is destroyed in
+// StopWatchingFileDescriptor().
+// It is moved into and out of lists in struct event_base by
+// the libevent functions event_add() and event_del().
+//
+// TODO(dkegel):
+// At the moment bad things happen if a FileDescriptorWatcher
+// is active after its MessagePumpLibevent has been destroyed.
+// See MessageLoopTest.FileDescriptorWatcherOutlivesMessageLoop
+// Not clear yet whether that situation occurs in practice,
+// but if it does, we need to fix it.
+
 namespace base {
 
 // Return 0 on success
@@ -29,7 +48,7 @@ MessagePumpLibevent::FileDescriptorWatcher::FileDescriptorWatcher()
 }
 
 MessagePumpLibevent::FileDescriptorWatcher::~FileDescriptorWatcher() {
-  if (event_.get()) {
+  if (event_) {
     StopWatchingFileDescriptor();
   }
 }
@@ -37,23 +56,27 @@ MessagePumpLibevent::FileDescriptorWatcher::~FileDescriptorWatcher() {
 void MessagePumpLibevent::FileDescriptorWatcher::Init(event *e,
                                                       bool is_persistent) {
   DCHECK(e);
-  DCHECK(event_.get() == NULL);
+  DCHECK(event_ == NULL);
 
   is_persistent_ = is_persistent;
-  event_.reset(e);
+  event_ = e;
 }
 
 event *MessagePumpLibevent::FileDescriptorWatcher::ReleaseEvent() {
-  return event_.release();
+  struct event *e = event_;
+  event_ = NULL;
+  return e;
 }
 
 bool MessagePumpLibevent::FileDescriptorWatcher::StopWatchingFileDescriptor() {
-  if (event_.get() == NULL) {
+  event* e = ReleaseEvent();
+  if (e == NULL)
     return true;
-  }
 
-  // event_del() is a no-op of the event isn't active.
-  return (event_del(event_.get()) == 0);
+  // event_del() is a no-op if the event isn't active.
+  int rv = event_del(e);
+  delete e;
+  return (rv == 0);
 }
 
 // Called if a byte is received on the wakeup pipe.
@@ -169,7 +192,7 @@ bool MessagePumpLibevent::WatchFileDescriptor(int fd,
     return false;
   }
 
-  // Transfer ownership of e to controller.
+  // Transfer ownership of evt to controller.
   controller->Init(evt.release(), persistent);
   return true;
 }

base/message_pump_libevent.h

@@ -6,7 +6,6 @@
 #define BASE_MESSAGE_PUMP_LIBEVENT_H_
 
 #include "base/message_pump.h"
-#include "base/scoped_ptr.h"
 #include "base/time.h"
 
 // Declare structs we need from libevent.h rather than including it
@@ -44,7 +43,7 @@ class MessagePumpLibevent : public MessagePump {
 
     private:
      bool is_persistent_;  // false if this event is one-shot.
-     scoped_ptr<event> event_;
+     event* event_;
      DISALLOW_COPY_AND_ASSIGN(FileDescriptorWatcher);
   };