Plug hole in struct event lifecycle; should fix bug 10503

base/message_pump_libevent.cc

 // Copyright (c) 2008 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 "base/message_pump_libevent.h"
 
 #include <errno.h>
 #include <fcntl.h>
 
 #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
 // Too small a function to bother putting in a library?
 static int SetNonBlocking(int fd) {
   int flags = fcntl(fd, F_GETFL, 0);
   if (flags == -1)
     flags = 0;
   return fcntl(fd, F_SETFL, flags | O_NONBLOCK);
 }
 
 MessagePumpLibevent::FileDescriptorWatcher::FileDescriptorWatcher()
     : is_persistent_(false),
       event_(NULL) {
 }
 
 MessagePumpLibevent::FileDescriptorWatcher::~FileDescriptorWatcher() {
-  if (event_.get()) {
+  if (event_) {
     StopWatchingFileDescriptor();
   }
 }
 
 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.
 void MessagePumpLibevent::OnWakeup(int socket, short flags, void* context) {
   base::MessagePumpLibevent* that =
               static_cast<base::MessagePumpLibevent*>(context);
   DCHECK(that->wakeup_pipe_out_ == socket);
 
   // Remove and discard the wakeup byte.
   char buf;
@@ skipping 95 matching lines @@
   }
 
   // Add this socket to the list of monitored sockets.
   if (event_add(evt.get(), NULL) != 0) {
     if (should_delete_event) {
       event_del(evt.get());
     }
     return false;
   }
 
-  // Transfer ownership of e to controller.
+  // Transfer ownership of evt to controller.
   controller->Init(evt.release(), persistent);
   return true;
 }
 
 
 void MessagePumpLibevent::OnLibeventNotification(int fd, short flags,
                                                  void* context) {
   Watcher* watcher = static_cast<Watcher*>(context);
 
   if (flags & EV_WRITE) {
@@ skipping 72 matching lines @@
 }
 
 void MessagePumpLibevent::ScheduleDelayedWork(const Time& delayed_work_time) {
   // We know that we can't be blocked on Wait right now since this method can
   // only be called on the same thread as Run, so we only need to update our
   // record of how long to sleep when we do sleep.
   delayed_work_time_ = delayed_work_time;
 }
 
 }  // namespace base