301029 - Hangs if key pressed continuosly to cycle through list box items which linked to other list boxes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ilkka Pirskanen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5 SUSE/1.0.5-2.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5 SUSE/1.0.5-2.1

When cycling through list box items by continuosly pressing a key to cycle
through items starting with that key, Firefox hangs. This happens at least on
http://www.openoffice.org/issues/query.cgi page where each "Component" list box
item causes "Subcomponent" list box to update its content.

Reproducible: Always

Steps to Reproduce:
1. Open http://www.openoffice.org/issues/query.cgi
2. Click on first item on "Component" list box (currently *Testproduct) to move
focus to that list box
3. Press and HOLD key S for 5 seconds.
Actual Results:  
Firefox hangs (at least on my computer).


Expected Results:  
Firefox should have cycled through all the "Component" list box items starting
with S (scipting, sdk, sg, sk, sl and so on). Each item choice affects
"Subcomponent" list box contents. MS IE 6.0 on Windows XP SP2 works correctly
albeit slowly as it takes time to update the "Subcomponent" (and other) list
boxes based on "Component" list box item.

SUSE 9.3 Professional + KDE 4.3 + Firefox 1.0.5

Comment 1

[Xbehave]

confirmed on kubuntu 7.10 (kde 3) 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007113004 Minefield/3.0b2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.10) Gecko/20071126 Ubuntu/7.10 (gutsy) Firefox/2.0.0.10

Not a problem on windows (only tested)
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9 

Comment 2

[Ryan Hickman]

conf - Kubuntu 7.10

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011704 Minefield/3.0b3pre

Comment 3

[Xbehave]

still happens on 3.1b2 Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre

Comment 4

[Mats Palmgren (inactive)]

(In reply to comment #0)
> Actual Results:  
> Firefox hangs (at least on my computer).

Maybe you did not wait long enough for the events to be processed?
I doubt that this is a true hang. Firefox should work normally after
it has processed all the events (which could take several minutes though).

For me, pressing 's' for 5 seconds takes Firefox 3.04 about 1min50s
to process.  Firefox trunk (20081123) takes about 1min.
(Kubuntu 8.04 x86-64 on a 2.66GHz Core2 CPU for reference)
FWIW, Opera 9.51 on Linux has the same problem, it takes 1min10s.

The problem does not occur on Windows XP, nor MacOSX 10.5.5.

I suspect that X11 generates events at a certain rate and puts them on
the input queue regardless of the application's ability to consume them
at that rate and that Windows/MacOSX automagically throttle the event
rate somehow. (I tested with a few printfs on Windows and it seems to
to confirm that.)

Processing the 'onchange' stuff takes about the same time on all
platforms, so the problem is not that we're slower on Linux just
that we're asked to handle a lot more events.

Comment 5

[Ilkka Pirskanen]

(In reply to comment #4)
> (In reply to comment #0)
> > Actual Results:  
> > Firefox hangs (at least on my computer).
> Maybe you did not wait long enough for the events to be processed?
> I doubt that this is a true hang.

I tried this on Firefox 3.0.3/openSUSE 11.0, and it seems that the problem is fixed at least on this platform.

When originally reporting this, it was sufficient to press a key a few times to induce hang and in this situation no other windows were responsive either, no matter how long you waited. Now other windows are responsive all the time, indicating Firefox is processing the keystrokes.

I am not sure if correcting the problem is related to changes in Firefox or  other system components.

If the recent reporters could re-test this whether this is just slow processing and no actual hang for them also?

Comment 6

[Mats Palmgren (inactive)]

Attachment: WIP

This seems to work for me.  It removes consecutive pending duplicate
KeyPress events.  I would appreciate it if you can test this in your
environment, there's a build with this patch here:
https://build.mozilla.org/tryserver-builds/2008-11-25_11:[e-mail]-1227641059/[e-mail]2
It prints "removing pending KeyPress" on the console when an event is discarded.

Comment 7

[Ilkka Pirskanen]

I tried this and it removes consecutive pending duplicate KeyPress events on openSUSE 11.0, as you said.

I also experimented with Windows Vista SP1 + IE 7. It seems that this platform performs some kind of "throttling" when a key is pressed and held down. If a key is pressed repeatedly (pressed and released in rapid succession), it seems that there is no throttling and it can take a while to process all the keystrokes produced this way.

As this is probably a rare situation, and might have negative effect on some browser games etc. which depend on the way browser recognizes keystrokes be careful if considering implementing this fix.

Comment 8

[Mats Palmgren (inactive)]

Thanks for testing.  Regarding games, I would think that the patch is
actually an improvement in that case too since you'd avoid stacking up
"commands" that continues to be executed after you release the key.
I'm not using browser-based games much though so if anyone have examples
where key-repeat throttling would be bad I'd love to hear about them.

As far as I can tell the patch makes GTK-based browsers behave more like
Windows/MacOSX, so the risk of regressing games/web sites seems low.

Comment 9

[Ilkka Pirskanen]

When thinkin of it, most likely browser based games would actually benefit from this change, as yoy say. Just wanted to highlight the risk of a regression, which probably is pretty low.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (same as WIP, without the printf)

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 350190 [details] [diff] [review]
Patch rev. 1 (same as WIP, without the printf)

This seems the right thing to do to me.

It took me a while to understand why XPeekEvent didn't always see KeyRelease
events: GDK calls XkbSetDetectableAutorepeat so that KeyRelease events are
generated only when the key is physically released.  Maybe it would be worth
adding a comment like that to explain to other people looking at the code.

>           next_event.xkey.state != event->state

This is comparing a GDK state with an XEvent state.  Fortunately the bits
mostly line up but _gdk_keymap_add_virtual_modifiers at least sometimes adds
some bits to the GDK state.

Implementing the filter through gdk_window_add_filter would give us access to
the current XEvent so that two XEvents could be compared, but that function
seems a bit much effort.

Looking at the docs for GdkModifierType, I suspect it is safe to mask out bits
15-31 from the GDK state.  (I don't know whether it matters if bits 13 and 14
are masked or not.)

>    GdkDisplay* gdkDisplay = gtk_widget_get_display(widget);

>       GdkWindow* nextGdkWindow =
>           gdk_window_lookup_for_display(gdkDisplay, next_event.xany.window);

next_event.xany.window != gdk_x11_drawable_get_xid(GDK_DRAWABLE(event.window))

would be a little more efficient.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #11)
> Maybe it would be worth adding a comment like that

I added a comment.

> Looking at the docs for GdkModifierType, I suspect it is safe to mask out bits
> 15-31 from the GDK state.  (I don't know whether it matters if bits 13 and 14
> are masked or not.)

I changed it to:
  next_event.xkey.state != (event->state & GDK_MODIFIER_MASK)
with the intent of masking out unrelated GDK bits but still include
super/hyper/meta and release bits -- it seems safer to not match events
with those bits.  (FWIW, I don't think GDK_RELEASE_MASK can ever be set
in key_press_event_cb())
http://library.gnome.org/devel/gdk/unstable/gdk-Windows.html#GdkModifierType

> >        gdk_window_lookup_for_display(gdkDisplay, next_event.xany.window);
> 
> next_event.xany.window != gdk_x11_drawable_get_xid(GDK_DRAWABLE(event.window))
> 
> would be a little more efficient.

I tried that but it didn't work. They differ by 1, I get:
next_event.xany.window = 0x32000c5
gdk_x11_drawable_get_xid(GDK_DRAWABLE(event.window) = 0x32000c4

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to comment #12)
> (In reply to comment #11)
> > >        gdk_window_lookup_for_display(gdkDisplay, next_event.xany.window);
> > 
> > next_event.xany.window != gdk_x11_drawable_get_xid(GDK_DRAWABLE(event.window))
> > 
> > would be a little more efficient.
> 
> I tried that but it didn't work. They differ by 1, I get:
> next_event.xany.window = 0x32000c5
> gdk_x11_drawable_get_xid(GDK_DRAWABLE(event.window) = 0x32000c4

Oh, sorry.  Looks like key events get a special window:

line 608 at
http://svn.gnome.org/viewvc/gtk%2B/trunk/gdk/x11/gdkwindow-x11.c?revision=21215&view=markup

and line 947 at
http://svn.gnome.org/viewvc/gtk%2B/trunk/gdk/x11/gdkevents-x11.c?revision=21833&view=markup

gdk_window_lookup_for_display (which you are using) is more consistent with what gdk_event_translate does.

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 350686 [details] [diff] [review]
Patch rev. 2

(In reply to comment #12)
> > Looking at the docs for GdkModifierType, I suspect it is safe to mask out
> > bits 15-31 from the GDK state.  (I don't know whether it matters if bits
> > 13 and 14 are masked or not.)
> 
> I changed it to:
>   next_event.xkey.state != (event->state & GDK_MODIFIER_MASK)
> with the intent of masking out unrelated GDK bits but still include
> super/hyper/meta and release bits -- it seems safer to not match events
> with those bits.  (FWIW, I don't think GDK_RELEASE_MASK can ever be set
> in key_press_event_cb())
> http://library.gnome.org/devel/gdk/unstable/gdk-Windows.html#GdkModifierType

I don't think super/hyper/meta should be tested as these are never on the
XEvent, but only added to GDK events.

For example, if the modifier is Super, then typically that is Mod4 and so
next_event.xkey.state is 0x00000040.

GDK adds GDK_SUPER_MASK in an effort to make it easier to detect the Super
modifier and so event->state is 0x04000040.  I think this should still be
considered to match a next_event.xkey.state of 0x00000040.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Ok, I changed GDK_MODIFIER_MASK to 0x1FFF.

Comment 16

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

+    Display* dpy = GDK_WINDOW_XDISPLAY(event->window);
+    GdkDisplay* gdkDisplay = gtk_widget_get_display(widget);

Wouldn't it make more sense to get gdkDisplay and then get 'dpy' from that?

Comment 17

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Nit fixed.

Comment 18

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/9148153082a8

No regression test since it's not testable through the current test
frameworks I think.  The code depends on the X11 event queue.

-> FIXED