480220 - Infinite, intermittent WM_QUIT loop in nsAppShell

Status: NEW

(Core :: Widget: Win32, defect)

Description

[Chad Austin]

Several months ago we had to apply a fix to our local copy of the XULRunner source.  This fix is a modification of bug 445030's fix.  I don't remember all of the details, but here is what I wrote in bug 445030:

The previous patch was not a complete fix in our embedding scenario.  We would occasionally get into a situation where WM_QUIT would not make it out of the Mozilla message pump, because PostQuitMessage marks WM_QUIT as a "lazy" message, meaning all other messages preempt it.  To prevent preemption, we used PostThreadMessage, which actually puts WM_QUIT in the message queue.

Patch is attached.

Not sure how to write a test.  We would hit the problem intermittently.  I don't know why.

Comment 1

[Chad Austin]

Attachment: Patch

Hm, somehow the patch wasn't attached.  Here it is again.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 364204 [details] [diff] [review]
Patch

Makes sense to me. It does mean that at application exit there might be messages that don't get processed, but I can't see how that would be a problem in practice.

Comment 3

[Rob Arnold [:robarnold]]

Why does the patch move the location of the call to after Exit instead of before?

Comment 4

[Chad Austin]

This is going to sound silly, but I do remember intentionally making that change back when I had the whole problem in my head.  I no longer remember, however.  I believe it worked on either side, but I felt more comfortable with it after the Exit().

Comment 5

[Hector Chu]

I'm not sure if I understand your problem.  Why would WM_QUIT not exit the Mozilla message loop?  On line 160 it tests if the message is WM_QUIT, and if it is it calls Exit() which should exit the loop.

Also, #2 mentions that it wouldn't be a problem if messages didn't get processed.  Unfortunately that would affect many applications that rely on WM_QUIT being the last processed message.

Comment 6

[Chad Austin]

I wish I could remember exactly but somehow we were pingponging infinitely between IMVU's event loop and nsAppShell's event loop.

If I get a chance, after our other embedding woes, I will try to dig deeper.  It's our fault for not submitting this patch sooner.

Comment 7

[Hector Chu]

Just a thought, but perhaps the priority of the WM_QUIT message is an issue because messages are being continuously generated from somewhere?

Comment 8

[Ere Maijala (slow)]

Comment on attachment 364204 [details] [diff] [review]
Patch

I'm a bit hesitant to give this r+. While it might fix the symptom, I doubt it's actually fixing the issue. The question is why we have a situation where the message processing never manages to empty the queue. This could mean e.g. that we have a case where handling a message causes the same message to be regenerated time after time. Which is obviously bad.

Would it be possible to log the messages handled to maybe get a hint on what is going on?

Calling Exit() before posting the message actually makes sense, because Exit() (which seems a bit poorly named to me) just sets an exiting flag.

Comment 9

[Chad Austin]

It's certainly possible to log messages or otherwise dig into the problem further.  I don't have time at the moment, but I'll let you know if I find more details.

Comment 10

[Ere Maijala (slow)]

Comment on attachment 364204 [details] [diff] [review]
Patch

Minusing for now.