diff options
| -rw-r--r-- | base/message_loop_unittest.cc | 75 | ||||
| -rw-r--r-- | base/message_pump_libevent.cc | 41 | ||||
| -rw-r--r-- | base/message_pump_libevent.h | 3 |
3 files changed, 108 insertions, 11 deletions
diff --git a/base/message_loop_unittest.cc b/base/message_loop_unittest.cc index d959659..ed7b0c1 100644 --- a/base/message_loop_unittest.cc +++ b/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) 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; } diff --git a/base/message_pump_libevent.h b/base/message_pump_libevent.h index aa5d5f8..a2a4e1c 100644 --- a/base/message_pump_libevent.h +++ b/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); }; |