Plug hole in struct event lifecycle; should fix bug 10503

base/message_pump_libevent.cc

Index: base/message_pump_libevent.cc
===================================================================
--- base/message_pump_libevent.cc	(revision 14186)
+++ base/message_pump_libevent.cc	(working copy)
@@ -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() {
-  if (event_.get()) {
+  if (event_) {
     StopWatchingFileDescriptor();
   }
 }
@@ -37,23 +56,27 @@
 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 @@
     return false;
   }
 
-  // Transfer ownership of e to controller.
+  // Transfer ownership of evt to controller.
   controller->Init(evt.release(), persistent);
   return true;
 }