472077 - win32 nsAppShell eats messages

Status: RESOLVED INVALID

(Core :: Widget: Win32, defect)

Description

[Dmitry Dartz]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 2.0.50727; .NET CLR 1.1.4322)
Build Identifier: 1.9.0.5

Duplication of 445030, this bug is closed but not fixed and I can't reopen it. That's why I post here the details.


Reproducible: Sometimes

Steps to Reproduce:
1. send WM_QUIT to the application using ::PostQuitMessage(0)

Actual Results:  
in 40%-50% cases, the application won't quit

Expected Results:  
-

Embedding application normally runs its very own message loop and does not expect any controls (including embedded mozilla) to steal such important application-wide messages like WM_QUIT.
It appears that 
nsAppShell::ProcessNextNativeEvent grabs events not intended to the mozilla's window.

See call stack below:

>	xul.dll!nsBaseAppShell::Exit()  Line 179
 	xul.dll!nsAppShell::ProcessNextNativeEvent(int mayWait=0)  Line 146
 	xul.dll!nsBaseAppShell::DoProcessNextNativeEvent(int mayWait=0)  Line 151 + 0x11 bytes
 	xul.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr=0x0087d3e0, int mayWait=0, unsigned int recursionDepth=0)  Line 296 + 0xf bytes
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=0, int * result=0x0012fdf4)  Line 500
 	xul.dll!NS_ProcessPendingEvents_P(nsIThread * thread=0x0087d3e0, unsigned int timeout=20)  Line 180 + 0x14 bytes
 	xul.dll!nsBaseAppShell::NativeEventCallback()  Line 122
 	xul.dll!nsAppShell::EventWindowProc(HWND__ * hwnd=0x00041196, unsigned int uMsg=49849, unsigned int wParam=0, long lParam=58814464)  Line 76

You see that hwnd is here in nsAppShell::EventWindowProc()! and this handle could/should be used in PeekMessage that is invoked in nsAppShell::ProcessNextNativeEvent() to process messages sent to the window.

Comment 1

[Dmitry Dartz]

Possible solution:
-make sure that ::PeekMessage is called with appropriate non-null HWND that the message is expected for.
-if you need application-wide messages, don't call ::PeekMessage with null HWND, use hook(s) installed with SetWindowsHookEx instead.

Comment 2

[Dave Garrett]

(In reply to comment #0)
> Duplication of 445030, this bug is closed but not fixed and I can't reopen it.

Don't intentionally post duplicates. Comment in the old bug and ask for it to be reopened if needed.

Also, in bug 445030 comment 14 you say that this is not fixed in 1.9.0.5. However it might be fixed in trunk or 1.9.1 branch. Please test the current development builds rather than only the current release version.

Comment 3

[Dmitry Dartz]

>Don't intentionally post duplicates

Sorry. 
But I want the problem to be reviewed once again. It's not only WM_QUIT created the problem (see below).

>*** This bug has been marked as a duplicate of bug 445030 ***

it's not a duplicate as I thought in the beginning...

> Comment in the old bug and ask for it to be reopened

I'll follow this rule next time.

>However it might be fixed in trunk or 1.9.1 branch

Yep. WM_QUIT will work in 1.9.1. But it's just a "workaround", not a good solution at all. It works for particular problem with WM_QUIT only. There are many other application-wide messages that should never be processed in Mozilla code. Applications often pre-process the messages, for example to get such things as application-wide shortcuts to work.
A typical message loop in Delphi and BCB applications look like below (you may find very similar code in MFC):

function TApplication.ProcessMessage(): Boolean;
var
  Handled: Boolean;
  Msg: TMsg;
begin
  Result := False;
  if PeekMessage(Msg, 0, 0, 0, PM_REMOVE) then
  begin
    Result := True;
    if Msg.Message <> WM_QUIT then
    begin
      Handled := False;
      if Assigned(FOnMessage) then FOnMessage(Msg, Handled);
      if not IsHintMsg(Msg) and not Handled and not IsMDIMsg(Msg) and
        not IsKeyMsg(Msg) and not IsDlgMsg(Msg) then
      begin
        TranslateMessage(Msg);
        DispatchMessage(Msg);
      end;
    end
    else
      FTerminate := True;
  end;
end;

...
  while (ProcessMessage()) do;
...

as you see, the messages are pre-preocessed and can be handled in several methods like IsHintMsg, IsMDIMsg, IsKeyMsg, and IsDlgMsg without calling TranslateMessage(Msg)/DispatchMessage(Msg) pair. 

For example, if there is an application-wide shorcut Alt-C, msg=WM_KEYDOWN wparam=VK_C will be handled in IsKeyMsg(). It should happen regardless if message's HWND is mozilla window or not, just because it is an application-wide shorcut. IsHintMsg, IsMDIMsg, and IsDlgMsg handle all the other application-wide events. All messages not handled there, are passed through TranslateMessage/DispatchMessage and therefore will reach mozilla window handler if they are passed there (their HWND=mozilla's window handle).
Mozilla can get all the messages there, in the window message procedure. 
Instead, Embedded Mozilla calls PeekMessage with HWND=0 and therefore gets _all_ messages, and passes all non-handled messages through  TranslateMessage/DispatchMessage. As a result, application-wide shorctus won't trigger and other applicatoin-wide behaviour will be broken. It's not acceptable.

Comment 4

[Dave Garrett]

(In reply to comment #3)
> >*** This bug has been marked as a duplicate of bug 445030 ***
> 
> it's not a duplicate as I thought in the beginning...

Fair enough. CCing some people from bug 445030.

Comment 5

[Dorian]

Same as Bug 452949

I think there should be, at least, a way for an embedder to disable the optimisation leading to this bug. I have to remove the code in OnProcessNextEvent from nsBaseAppShell for my own gecko build.

Comment 6

[Dmitry Dartz]

Dorian,
I'm not sure what optimizations you're talking about... 
I believe that a particular gecko window needs only messages sent to this window and only, so peeking messages with PeekMessage() is just wrong way, not a kind of optimization. All the processing should moved into the window message handler.

Any ideas on why PeekMessage is there at all? What's its role? In other words, what will be broken if we just remove PeekMessage? Is it possible to find what bug was fixed by placing PeekMessage there?

If I remember, it already saw this code in 1.8.0.1, so it's there for some years now.

Comment 8

[Dorian]

I know it's the wrong way but that's how they did it. Nothing is broken if you remove it, you only lose performance.

Everything start from Bug 326273

Comment 9

[Dmitry Dartz]

>Everything start from Bug 326273

No, I think it started much earlier.
At least the same code can be found in nsAppShell.cpp with date/time stamp 2004-04-19 (grab 1.8.0.3 for example to see it), while the issue was submitted on 2006-02-07