From 58eaaab9b9efd635bc679a78d6a9a1d54052e105 Mon Sep 17 00:00:00 2001 From: Dominik Roettsches Date: Fri, 7 Jan 2011 14:18:29 +0200 Subject: [PATCH] Bug 618810 - Qt Message Pump locks up in case of nested loops in flash plugin. r=dougt a=npodb --- dom/plugins/PluginInstanceChild.cpp | 3 + ipc/chromium/src/base/message_pump_qt.cc | 126 ++++++++++++++++------- ipc/chromium/src/base/message_pump_qt.h | 11 +- 3 files changed, 98 insertions(+), 42 deletions(-) diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp index 544d6cca684..760678374ec 100644 --- a/dom/plugins/PluginInstanceChild.cpp +++ b/dom/plugins/PluginInstanceChild.cpp @@ -624,6 +624,9 @@ PluginInstanceChild::AnswerNPP_HandleEvent(const NPRemoteEvent& event, return true; #endif + // XXX A previous call to mPluginIface->event might block, e.g. right click + // for context menu. Still, we might get here again, calling into the plugin + // a second time while it's in the previous call. if (!mPluginIface->event) *handled = false; else diff --git a/ipc/chromium/src/base/message_pump_qt.cc b/ipc/chromium/src/base/message_pump_qt.cc index d7e7852efd9..36a8b7c9e18 100644 --- a/ipc/chromium/src/base/message_pump_qt.cc +++ b/ipc/chromium/src/base/message_pump_qt.cc @@ -7,8 +7,10 @@ #include #include #include +#include #include +#include #include #include "base/eintr_wrapper.h" @@ -33,7 +35,7 @@ MessagePumpForUI::~MessagePumpForUI() { } MessagePumpQt::MessagePumpQt(MessagePumpForUI &aPump) - : pump(aPump) + : pump(aPump), mTimer(new QTimer(this)) { // Register our custom event type, to use in qApp event loop #if (QT_VERSION >= QT_VERSION_CHECK(4, 4, 0)) @@ -41,10 +43,14 @@ MessagePumpQt::MessagePumpQt(MessagePumpForUI &aPump) #else sPokeEvent = QEvent::User+5000; #endif + connect(mTimer, SIGNAL(timeout()), this, SLOT(dispatchDelayed())); + mTimer->setSingleShot(true); } MessagePumpQt::~MessagePumpQt() { + mTimer->stop(); + delete mTimer; } bool @@ -57,6 +63,31 @@ MessagePumpQt::event(QEvent *e) return false; } +void +MessagePumpQt::scheduleDelayedIfNeeded(const Time& delayed_work_time) +{ + if (delayed_work_time.is_null()) { + return; + } + + if (mTimer->isActive()) { + mTimer->stop(); + } + + TimeDelta later = delayed_work_time - Time::Now(); + // later.InMilliseconds() returns an int64, QTimer only accepts int's for start(), + // std::min only works on exact same types. + int laterMsecs = later.InMilliseconds() > std::numeric_limits::max() ? + std::numeric_limits::max() : later.InMilliseconds(); + mTimer->start(laterMsecs); +} + +void +MessagePumpQt::dispatchDelayed() +{ + pump.HandleDispatch(); +} + void MessagePumpForUI::Run(Delegate* delegate) { RunState state; state.delegate = delegate; @@ -67,54 +98,75 @@ void MessagePumpForUI::Run(Delegate* delegate) { // will mean that we don't block on the message pump until there was nothing // more to do. We also set this to true to make sure not to block on the // first iteration of the loop, so RunAllPending() works correctly. - state.more_work_is_plausible = true; + bool more_work_is_plausible = true; RunState* previous_state = state_; state_ = &state; - // We run our own loop instead of using g_main_loop_quit in one of the - // callbacks. This is so we only quit our own loops, and we don't quit - // nested loops run by others. TODO(deanm): Is this what we want? + for(;;) { + QEventLoop::ProcessEventsFlags block = QEventLoop::AllEvents; + if (!more_work_is_plausible) { + block |= QEventLoop::WaitForMoreEvents; + } - while (!state_->should_quit) { - QAbstractEventDispatcher *dispatcher = QAbstractEventDispatcher::instance(qApp->thread()); - if (!dispatcher) + QAbstractEventDispatcher* dispatcher = + QAbstractEventDispatcher::instance(qApp->thread()); + // An assertion seems too much here, as during startup, + // the dispatcher might not be ready yet. + if (!dispatcher) { return; - dispatcher->processEvents(QEventLoop::AllEvents | QEventLoop::WaitForMoreEvents); + } + + // processEvent's returns true if an event has been processed. + more_work_is_plausible = dispatcher->processEvents(block); + + if (state_->should_quit) { + break; + } + + more_work_is_plausible |= state_->delegate->DoWork(); + if (state_->should_quit) { + break; + } + + more_work_is_plausible |= + state_->delegate->DoDelayedWork(&delayed_work_time_); + if (state_->should_quit) { + break; + } + + qt_pump.scheduleDelayedIfNeeded(delayed_work_time_); + + if (more_work_is_plausible) { + continue; + } + + more_work_is_plausible = state_->delegate->DoIdleWork(); + if (state_->should_quit) { + break; + } } state_ = previous_state; } void MessagePumpForUI::HandleDispatch() { - // We should only ever have a single message on the wakeup pipe, since we - // are only signaled when the queue went from empty to non-empty. The qApp - // poll will tell us whether there was data, so this read shouldn't block. - if (state_->should_quit) + if (state_->should_quit) { return; + } - state_->more_work_is_plausible = false; + if (state_->delegate->DoWork()) { + // there might be more, see more_work_is_plausible + // variable above, that's why we ScheduleWork() to keep going. + ScheduleWork(); + } - if (state_->delegate->DoWork()) - state_->more_work_is_plausible = true; - - if (state_->should_quit) + if (state_->should_quit) { return; + } - if (state_->delegate->DoDelayedWork(&delayed_work_time_)) - state_->more_work_is_plausible = true; - if (state_->should_quit) - return; - - // Don't do idle work if we think there are more important things - // that we could be doing. - if (state_->more_work_is_plausible) - return; - - if (state_->delegate->DoIdleWork()) - state_->more_work_is_plausible = true; - if (state_->should_quit) - return; + state_->delegate->DoDelayedWork(&delayed_work_time_); + qt_pump.scheduleDelayedIfNeeded(delayed_work_time_); } void MessagePumpForUI::Quit() { @@ -126,18 +178,16 @@ void MessagePumpForUI::Quit() { } void MessagePumpForUI::ScheduleWork() { - // This can be called on any thread, so we don't want to touch any state - // variables as we would then need locks all over. This ensures that if - // we are sleeping in a poll that we will wake up. QCoreApplication::postEvent(&qt_pump, new QEvent((QEvent::Type) sPokeEvent)); } void MessagePumpForUI::ScheduleDelayedWork(const Time& delayed_work_time) { - // We need to wake up the loop in case the poll timeout needs to be - // adjusted. This will cause us to try to do work, but that's ok. + // On GLib implementation, a work source is defined which explicitly checks the + // time that has passed. Here, on Qt we can use a QTimer that enqueues our + // event signal in an event queue. delayed_work_time_ = delayed_work_time; - ScheduleWork(); + qt_pump.scheduleDelayedIfNeeded(delayed_work_time_); } } // namespace base diff --git a/ipc/chromium/src/base/message_pump_qt.h b/ipc/chromium/src/base/message_pump_qt.h index 24126b72b08..2ecbb07f96d 100644 --- a/ipc/chromium/src/base/message_pump_qt.h +++ b/ipc/chromium/src/base/message_pump_qt.h @@ -15,6 +15,8 @@ #include "base/message_pump.h" #include "base/time.h" +class QTimer; + namespace base { class MessagePumpForUI; @@ -27,9 +29,14 @@ class MessagePumpQt : public QObject { ~MessagePumpQt(); virtual bool event (QEvent *e); + void scheduleDelayedIfNeeded(const Time& delayed_work_time); + + public slots: + void dispatchDelayed(); private: base::MessagePumpForUI &pump; + QTimer* mTimer; }; // This class implements a MessagePump needed for TYPE_UI MessageLoops on @@ -61,10 +68,6 @@ class MessagePumpForUI : public MessagePump { // Used to count how many Run() invocations are on the stack. int run_depth; - - // Used internally for controlling whether we want a message pump - // iteration to be blocking or not. - bool more_work_is_plausible; }; RunState* state_;