145313 - Ctrl/Shift/etc state should be registered at click time, not later when loading/processing

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Dwayne]

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.

Comment 1

[jag (Peter Annema)]

-> event handling

Comment 2

[Dwayne]

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. :)

Comment 3

[Pham]

.

Comment 4

[John Sullivan]

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...

Comment 5

[John Sullivan]

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.

Comment 6

[John Sullivan]

Attachment: fix for out-of-order input message processing under Windows

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

Comment 7

[John Sullivan]

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.)

Comment 8

[Dave Garrett]

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.

Comment 9

[mr]

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.

Comment 12

[Mike Schroepfer]

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...

Comment 13

[Daniel B.]

(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

Comment 14

[Dave Garrett]

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.

Comment 15

[John Sullivan]

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.)

Comment 16

[John Sullivan]

Attachment: Simplify Windows message dispatch

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

Comment 17

[Serge Gautherie (:sgautherie)]

(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...

Comment 18

[John Sullivan]

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.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is too scary to take on branch, and seems like something that has been around forever.

Lets shoot for next release instead.

Comment 20

[John Sullivan]

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.

Comment 21

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

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 22

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

Comment on attachment 320385 [details] [diff] [review]
Simplify Windows message dispatch

John, are you interested in pursuing this as I suggested?

Comment 23

[mr]

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).

Comment 24

[mr]

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.

Comment 25

[mr]

Bug 489177 is also a duplicate of this bug.

Comment 28

[Dave Garrett]

Thanks, duped.

Comment 29

[David Balažic]

Seeing this with 3.6.8 on Windows 7.

(see http://forums.mozillazine.org/viewtopic.php?f=38&t=1968419 )

Comment 30

[Marat Tanalin]

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.

Comment 31

[mr]

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.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this should be fixed by the patch for bug 593372.

Would you test this on latest trunk?

Comment 33

[Cameron McCormack (:heycam)]

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?

Comment 34

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

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.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, see also:

http://blogs.msdn.com/b/oldnewthing/archive/2004/11/30/272262.aspx

Comment 36

[(no longer active)]

Yep, seems fine to me too.

Comment 37

[mr]

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.