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

1 files changed, 32 insertions, 9 deletions

diff --git a/base/message_pump_libevent.cc b/base/message_pump_libevent.cc
index 2f828ba..f2681f3 100644
--- a/base/message_pump_libevent.cc
+++ b/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;
 }