618810 - Qt Message Pump locks up in case of nested loops in flash plugin

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Dominik Röttsches]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12
Build Identifier: mozilla-central @210237f7d626 

The Qt message pump is likely to lock up if the flash player implementation spawns nested event loops. The attached patch attempts to fix several issues with the previous message pump by aligning the implementation more closely with its glib counterpart:
- It stays in the main work-loop until message pump work is done, only then passes control to the UI framework's event loop. No tasks are lost or wait forever in the queue.
- It contains a new implementation for scheduling delayed work using a timer. Previously delayed work got stalled under certain conditions.

These two issues are probably covering the "unknown reason" that was discussed in bug 590687.

Reproducible: Always

Steps to Reproduce:
1. Go to a video page on youtube
2. Play a video
3. Right click to show context sensitive menu
4. Don't select anything.
5. Return from the menu
Actual Results:  
Observe a freeze of the content process. No links can be clicked, no navigation. 

Expected Results:  
Browser should be reactive, links clickable, navigation okay.

Comment 1

[Dominik Röttsches]

Attachment: Fix for Qt message pump, closer resemblance to glib version

Comment 2

[Oleg Romashin (:romaxa)]

Comment on attachment 497245 [details] [diff] [review]
Fix for Qt message pump, closer resemblance to glib version


> ifdef MOZ_ENABLE_QT
>-#MOCSRCS = \
>-#  moc_message_pump_qt.cc \
>-#  $(NULL)
>-#
>-#CPPSRCS += \
>-#  $(MOCSRCS) \
>-#  message_pump_qt.cc \
>-#  $(NULL)
>+MOCSRCS = \
>+  moc_message_pump_qt.cc \
>+  $(NULL)

Please attach file against mozilla-central repository...


>+    if(state_->should_quit)
>+      break;
{}, and for others


but in general looks good.

Should anyone else look at this patch? like cjones or bsmedberg?

Comment 3

[Dominik Röttsches]

Attachment: Fix for Qt message pump v2, closer resemblance to glib version, review comments addressed

Fix for Qt message pump, review comments addressed

Comment 4

[Oleg Romashin (:romaxa)]

Comment on attachment 497468 [details] [diff] [review]
Fix for Qt message pump v2, closer resemblance to glib version, review comments addressed


>+
>+  if(mTimer->isActive()) {
      ^ space should be here
>+    mTimer->stop();
>+  }
>+
>+  TimeDelta later = delayed_work_time - Time::Now();
>+  int laterMsecs = later.InMilliseconds() > INT_MAX ? INT_MAX : later.InMilliseconds();
any explanation why we may get INT_MAX here? should not we try to avoid this problem?

>+{
>+  pump.HandleDispatch();
>+}
>+
>+
2 lines, not good

> void MessagePumpForUI::Run(Delegate* delegate) {
>   RunState state;
>   state.delegate = delegate;
>   state.should_quit = false;
>   state.run_depth = state_ ? state_->run_depth + 1 : 1;
>+
any reason of this empty line?
>   // We really only do a single task for each iteration of the loop.  If we
>   // have done something, assume there is likely something more to do.  This
>   // 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 =

should be "QAbstractEventDispatcher* dispatcher ="


>+      QAbstractEventDispatcher::instance(qApp->thread());
>+    // An assertion seems too much here, as during startup,
>+    // the dispatcher might not be ready yet.
>+    if(!dispatcher) {
        ^ space

>+    if(state_->should_quit) {
        ^ same space, and the rest of file the same
>+      break;
>+    }



The rest is looks good for me.
Also tested, flash fullscreen and CSM now works great, no hangs and froze IPC anymore.
cjones: would you like to look at this patch?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 497468 [details] [diff] [review]
Fix for Qt message pump v2, closer resemblance to glib version, review comments addressed

Nah, I won't be much help with the qt stuff here.

Is this the cause of bug 590687?

We really ought to add a test for this.  There's a plugin mochitest that exercises nested event loops by accessing the clipboard, but the nptest code is gtk specific, and I don't know how functional the nptest Qt backend is besides.  Is there a simpler, deterministic way to trigger this class of bugs? An ipc/ipdl/test/cxx test might be better suited if so.

Comment 7

[Dominik Röttsches]

Attachment: Fix for Qt message pump, closer resemblance to glib version, v3

Style issues addressed, numeric_limits<int>::max() instead of MAX_INT for conversion from InMilliseconds() to start() argument for QTimer, comment added.
Previous version v2 r+'ed by romaxa.

Comment 8

[Oleg Romashin (:romaxa)]

Tested with ipdltest, that does not cover this problem... but at least now it is runnable.

Will check if it is easy to create similar clipboard test for Qt.
IIUC it is this test plugin/test/mochitest/test_copyText.html

if not then we should probably create cxx test

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 500322 [details] [diff] [review]
Fix for Qt message pump, closer resemblance to glib version, v3

This highly needed for Qt port working properly.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 500322 [details] [diff] [review]
Fix for Qt message pump, closer resemblance to glib version, v3

a=npodb

Comment 11

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/f946ed44a9a0