Build: RC2, RC1, and previous on Win2k/NT Bug occurrs when Mozilla is slow to respond (which happens quite frequently after switching between tasks) Steps to Reproduce: 1. Open Mozilla 2. Switch to another task. 3. Switch back to Mozilla. This *should* cause a delay on page loading response. 4. CTRL+Click on a link to spawn in a new tab. 5. Let go of CTRL directly after clicking. Results: Page loads in current tab. Expected: Page loads in new tab. Reproduceablity: Only if you're fast enough to let go of CTRL before Mozilla responds (not hard in some cases with delays of 5 seconds or more) Workaround: Hold CTRL down until the page begins to load.

-> event handling

I think it may help to reproduce this bug by opening several tabs with different pages at once. The rendering time goes way up; this should create an excellent lag. :)

.

Was about to report this myself (after a year of seeing this under all Firebird versions and Windows 98SE and 2000) but I see someone beat me to it ;-) FWIW under Windows the WM_?BUTTONDOWN and WM_?BUTTONDBLCLK messages both pass key flags in wParam for whether Ctrl or Shift are pressed *AT THE TIME OF THE CLICK*. This is the correct way to distinguish Ctrl-Clicks. It is not appropriate to call GetKeyState(VK_CONTROL) or variations of at the time the message is dispatched, because on a loaded system (such as one running some version of Mozilla :-/) it can be some time before an application gets round to checking its message queue, by which time the user has probably released any modifier keys. The workaround works, but it is quite annoying to have to hold down a key for 30-odd seconds whilst waiting for the browser to respond...

Ok, I think I know why this is occuring now. Is anyone looking out for this bug at the moment - it looks like the Assigned To contact is defunct? In mozilla\widget\src\windows\nsWindow.cpp, DispatchMouseEvent() uses (via IS_VK_DOWN) GetKeyState() to test the state of the Shift, Control and Alt keys. GetKeyState() uses a saved snapshot of the keyboard state, not the current hardware state, so shouldn't change during the processing of a single message even if that takes a long time. I've generated a test harness and it seems reliable with the caveats below. WM_LBUTTONDOWN/UP do however send the Shift and Control key states as part of wParam - you're supposed to test for the presence of the MK_CONTROL and MK_SHIFT bits rather than calling GetKeyState(). This doesn't work for the Alt key however - the mouse messages never set MK_ALT - so you have to use GetKeyState(VK_MENU) for that. (To make myself clearer: during a mouse message, GetKeyState() for ctrl and shift ought to work but is unnecessary because Windows has already told you what they are. Alt must still be checked via GetKeyState() however, and I can see the benefit of checking them all in a consistent manner.) The precise situation which is failing here is: User presses the Control key, presses the mouse button over a link, releases the mouse button, then releases the control key, and then some time later due to a high system load the browser is actually able to process that sequence. Most Win32 apps use GetMessage() to run their main message loop. This returns new messages in first-in-first-out order. In these cases, the above sequence wouldn't be a problem - it would behave exactly as expected. Moz however tries to prioritise certain types of message (see mozilla\widget\src\windows\nsAppShell.cpp, nsAppShell::Run()). First keyboard/IME then mouse, then all other messages. Putting input messages ahead of, say slow paint messages, is good for responsiveness, but keyboard and mouse messages should never be reordered in this fashion. In the above scenario, when we eventually get round to processing the queue, the two keyboard messages (Ctrl down, Ctrl up) will be processed first. Then the two mouse messages (lbutton down, lbutton up). GetKeyState(VK_CONTROL) will return up because as far as the message queue is concerned the key has already been released, even though it was pressed at the time the mouse messages were generated. The implementation of PeekKeyAndIMEMessage() itself suggests a solution, assuming we want to keep using PeekMessage() and not switch to GetMessage() in order to maintain some prioritisation. PeekKeyAndIMEMessage() peeks on both normal key messages, and IME messages separately. (You have to specify a single contiguous range of message values for each call. Key messages are a single contiguous range. IME messages are a separate range, mouse messages a third separate, but internally contiguous range.) It then tests the message time (msg.time) to find which happened earlier and return that one. Instead of handing key and IME messages to PeekKeyAndIMEMessage() and handling mouse messages in a separate PeekMessage call, mouse messages should be bundled into PeekKeyAndIME(AndMouse)Message(), removing and processing only the first one of the three by time. This will ensure that they are not seen out of order, hence that by the time the mouse click is processed the Alt key has not already been marked as released.

This patch fixes PeekKeyAndIMEMessage() in ./mozilla/widget/src/windows/nsAppShell.cpp as described above.

Actually, even with this patch I can still occasionally see the problem - it looks like the MSG.time value does not always reliably increase between messages or there is some other weirdness going on. The patch appears to make some difference but is not a total solution. The only solution I can see is to switch to using plain old GetMessage() to extract messages, which would lose the prioritisation but would ensure correct in-order processing of input messages. (The problem I have is that I now have a *much* faster machine than when I was seeing this problem a lot, so it's much harder to reproduce. On a Pentium 166 I was seeing it fairly reliably because every new tab was taking several seconds to open.)

Was about to post a new report similar to this... might as well just revive this old one. I'm using 2.0.0.3 right now and I routinely have this problem. (laptop; don't always have a mouse plugged in, so I use ctrl+click a lot) If I open a bunch of tabs with ctrl+click when I already have a pile open I need to hold ctrl until they load or they'll load in the foreground. This seems to be quite an old bug, and it still needs fixing.

I believe this is the same problem as in Bug 334497 – "click processing doesn't use shift/modifier at click time, checks at processing time". I posted an extended description there. Please check it out.

Given this issue is in 2.0.x and this bug is 6 years old we can't hold the final 1.9/FF3 release for this...

(In reply to comment #8) > This seems to be quite an old bug, and it still needs fixing. It also seems to show a fundamental misunderstanding of how to receive and/or queue user-input events. Daniel

Just did some more recent testing. Can't reproduce this in Linux at all on FF2 or FF3. (one of the dupes says "all", so that seems to be wrong) This bug still has effect in Windows with the latest FF3 nightly. I see there's a wanted1.9.0.x request pending. Is a wanted1.9 completely out of the question? This affects a lot of users still.

That's because this particular bug *can't* appear on Linux - it's a side effect of the way FF uses the Windows PeekMessage function to extract user input messages from the message queue out-of-order with respect to other messages. As a result it ends up reordering mouse and keyboard messages with respect to each other too. If someone has actually seen something going on under Linux, it's a different bug. However the "all" is probably just a default guess. This bug is well characterised, and so is the solution: rip out the over-clever code and replace it with a simpler message loop which just dispatches messages in the order received. The question then is why is the code as it is now? Someone must at some point have thought the current approach was necessary? (Even though it is pretty much doomed to not work.)

If there is consensus about taking this approach, this is the patch that should do it.

(In reply to comment #15) > The question then is why is the code as it is now? Someone must at some point > have thought the current approach was necessary? (Even though it is pretty much > doomed to not work.) Blame seems to go to bug 57332...

It seems to me that bug 57332 is about a different side-effect caused by the same message reordering approach, which was already present at that time - it was "fixed" there by adding IME messages to the list of messages pulled to the front of the queue. I wouldn't be suprised if there weren't several other rare but annoying bugs related to this. The common underlying cause is that message reordering just can't work.

This is too scary to take on branch, and seems like something that has been around forever. Lets shoot for next release instead.

Although it strikes me looking at the train wreck that is: http://bonsai.mozilla.org/cvsgraph.cgi?file=mozilla/widget/src/windows/nsAppShell.cpp There was once a reasonable reason for doing this: elevate *all* system messages above *purely* application internal messages. This then got corrupted into elevating specific system messages (keyboard/mouse) above others. This is the bit that definitely causes problems, since there is no way (given the millisecond precision message timestamp with no guarantee of strict monotonicity, and the non-contiguous ranges occupied by keyboard, mouse and IME messages) to preserve the relative ordering of these messages. The original attempt can work, with extreme caution: as long as no app message attempts to retrieve the system input state outside of the scope of the individual message (so anything relevant must be bundled into the app message, which isn't quite true of the original system messages themselves), and no app-level logic is ever directly performed within the context of an input message, it ought to be possible. In practice no application in my experience is ever structured cleanly enough to guarantee this.

Thanks for the great analysis, John. Messing with the message priorities has, in the past, had pretty major effects on performance and interactive response, so I'm reluctant to just take your patch, appealing as it is. Interactivity is especially hard to test :-(. How about changing PeekKeyAndIMEMessage to include mouse events, calling it PeekInputMessage say. It seems like we could then use three PeekMessage calls: one with PM_QS_INPUT to get regular keyboard *and* mouse events --- in the right order --- and two others to get IME events (WM_IME_STARTCOMPOSITION to WM_IME_KEYLAST, and WM_IME_SETCONTEXT to WM_IME_KEYUP), choosing the message with minimum timestamp. It's too bad Windows doesn't give us more detailed information about message order, or a way to PeekMessage for several ranges at once.

Comment on attachment 320385 [details] [diff] [review] Simplify Windows message dispatch John, are you interested in pursuing this as I suggested?

My impression is that this bug makes a really nasty combination together with Bug 484964 ("Flash players stutter during video or game playback every ten seconds or so - lasts for 1 to 2 seconds, then continues"). [Note that it's the whole browser drawing & message input handling that is lagging - not only Flash player.] Problem: When the browser lags behind, it does not take care of the click messages. When I ctrl-click or shift-click a link and release the modifier key shortly after the click, it happens quite often that both the click and the modifier release was made during lagging time. Then the click is taken care of at a later time, without modifier. This is very easy for me to reproduce: 1) load a page with animated GIFs, e g http://www.emoticones.com/emoticonos_bananas.php 2) when a typical 2-second drawing lag starts, quickly ctrl-click a link and release the ctrl key before drawing starts again Result: a new webpage opens (ordinary click). Expected result: a new tab opens (ctrl-click).

I just mentioned Bug 484964 above. Bug 490122 is probably the same, but better described. They are causing the delays that makes this bug serious. Moreover, Bug 497902 is a duplicate of this bug.

Bug 489177 is also a duplicate of this bug.

Thanks, duped.

Seeing this with 3.6.8 on Windows 7. (see http://forums.mozillazine.org/viewtopic.php?f=38&t=1968419 )

When this serious usability bug are planned to be fixed? This really makes Firefox _far_less_usable_: links are fundamental elements of the internet itself, and they are hugely broken in Firefox due to this bug. It makes sense to fix this as soon as possible (hopefully, in Firefox 4.0) to make Firefox far better. Thanks.

8½ years old bug that breaks keystroke order, click & scroll functionality - and status2.0 "wontfix" now got added? It also has Severity = "minor". I can tell you this: As long as Bug 490122 is in the system, this bug is definitely *not* a minor problem. After running Firefox for hours (a normal usecase), Bug 490122 periodically delays keypress processing several seconds(!), making all Ctrl/Shift keypresses coincide with totally wrong keypresses or scroll events. The results should not be accepted in a serious program: - When I try to open new tabs or new windows via Ctrl-click or Shift-click, Firefox ends up loading a new page in the current tab instead, leaving the page I obviously wanted to stay at. - When I try to scroll a page, Firefox instead goes back in browser history or resizes the display to huge letters, all because Shift or Ctrl was pressed some second before. Normal = "regular issue, some loss of functionality under specific circumstances" Minor = "minor loss of function, or other problem where easy workaround is present" This bug should be raised at least to Normal. And get fixed ASAP. Or please show us that easy workaround.

I think that this should be fixed by the patch for bug 593372. Would you test this on latest trunk?

Right, keyboard and mouse events should all be processed in the right order due to the changes in bug 593372. I am concerned though about the use of IS_VK_DOWN (which calls GetKeyState) in say nsWindow::DispatchMouseEvent. If when doing a Ctrl+Click on a link the window message queue backs up, and by the time the WM_MOUSEUP event is processed the Ctrl key has been released, what will IS_VK_DOWN return? Does GetKeyState return values based on messages that have been processed in the message queue, or does it keep a track internally of the the state of all keys?

http://msdn.microsoft.com/en-us/library/ms646301%28v=vs.85%29.aspx says GetKeyState reflects the state as per the state of the application message queue. (GetAsyncKeyState returns the true "current" key state.) So I think we're OK.

Yes, see also: http://blogs.msdn.com/b/oldnewthing/archive/2004/11/30/272262.aspx

Yep, seems fine to me too.

Seems like it works well in FF 4.0. Well done! I tried to provoke it by quickly opening (ctrl-clicking with fast keypress/release) 50 extra tabs... I reached 1,600 MB process memory and didn't notice a single glitch in the click/key handling. Everything seems to be handled in the correct order, even though the graphic updating is halting a lot.