Bug 618810 - Qt Message Pump locks up in case of nested loops in flash plugin. r=dougt a=npodb

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

ipc/chromium/src/base/message_pump_qt.cc

@@ -7,8 +7,10 @@
 #include <qabstracteventdispatcher.h>
 #include <qevent.h>
 #include <qapplication.h>
+#include <qtimer.h>
 
 #include <fcntl.h>
+#include <limits>
 #include <math.h>
 
 #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<int>::max() ?
+    std::numeric_limits<int>::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

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_;