Bug 44935

Summary: [Qt] Web process crash when pressing modifiers in input field
Product: WebKit Reporter: Kimmo Kinnunen <kimmo.t.kinnunen>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, ariya.hidayat, commit-queue, kbalazs, ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
patch describing the point of the crash
none
Patch fixing the crash
none
Patch try #2
none
Fix for the test regression
none
keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler
none
disambiguateKeyDownEvent compiled none

Description Kimmo Kinnunen 2010-08-31 01:39:08 PDT
Created attachment 66021 [details]
patch describing the point of the crash

Web process crashes when pressing modifiers in input field

Null pointer reference in PlatformKeyboardEvent.
Comment 1 Kimmo Kinnunen 2010-08-31 04:14:03 PDT
Created attachment 66029 [details]
Patch fixing the crash
Comment 2 Ariya Hidayat 2010-08-31 05:29:50 PDT
Comment on attachment 66029 [details]
Patch fixing the crash 

> +#if defined(XP_UNIX)
> +uint32_t PlatformKeyboardEvent::x11NativeModifiers() const
> +{
> +    ASSERT(m_qtEvent);
> +    return m_qtEvent->nativeModifiers();
> +}
> +
> +uint32_t PlatformKeyboardEvent::x11NativeScanCode() const
> +{
> +    ASSERT(m_qtEvent);
> +    return m_qtEvent->nativeScanCode();
> +}
> +#endif

This is confusing. Can you explain what is the reason behind introducing these two Unix-only functions?
Comment 3 Kimmo Kinnunen 2010-08-31 06:17:41 PDT
(In reply to comment #2)
> (From update of attachment 66029 [details])
> > +#if defined(XP_UNIX)
> > +uint32_t PlatformKeyboardEvent::x11NativeModifiers() const
> > +{
> > +    ASSERT(m_qtEvent);
> > +    return m_qtEvent->nativeModifiers();
> > +}
> > +
> > +uint32_t PlatformKeyboardEvent::x11NativeScanCode() const
> > +{
> > +    ASSERT(m_qtEvent);
> > +    return m_qtEvent->nativeScanCode();
> > +}
> > +#endif
> 
> This is confusing. Can you explain what is the reason behind introducing these two Unix-only functions?

The idea is that X11 plugins can be supported even though PlatformKeyboardEvent may not be instantiated from QKeyEvent

In other words: the implementation tries to remove the notion that PlatformKeyboardEvent is QKeyEvent.

Otherwise we must synthetize QKeyEvent from WebKeyboardEvent. This is a bit redundant. On top of redundancy, this is not even possible, because there's no such a constructor for QKeyEvent (X11 Qt uses QKeyEventEx internally)
Comment 4 Ariya Hidayat 2010-09-02 23:35:52 PDT
 
> In other words: the implementation tries to remove the notion that PlatformKeyboardEvent is QKeyEvent.

I did not ask what the patch is doing, which is quite clear, but rather why it is implemented that way.

Here is what I had in mind when thinking about this problem. There will be nativeModifiers() and nativeScanCode() for PlatformKeyboardEvent under #if PLATFORM(QT), i.e. no need to specialize for XP_UNIX. Can this work?
Comment 5 Kimmo Kinnunen 2010-09-02 23:42:27 PDT
(In reply to comment #4)
> > In other words: the implementation tries to remove the notion that PlatformKeyboardEvent is QKeyEvent.
> 
> I did not ask what the patch is doing, which is quite clear, but rather why it is implemented that way.
> 
> Here is what I had in mind when thinking about this problem. There will be nativeModifiers() and nativeScanCode() for PlatformKeyboardEvent under #if PLATFORM(QT), i.e. no need to specialize for XP_UNIX. Can this work?

You mean that remove the #if XP_UNIX and remove the x11 prefix? Should work, though I don't see any point where the other qt platforms use the methods. It's ok for me, if it's ok for you.
Comment 6 Ariya Hidayat 2010-09-03 00:49:34 PDT
> You mean that remove the #if XP_UNIX and remove the x11 prefix? Should work, though I don't see any point where the other qt platforms use the methods. It's ok for me, if it's ok for you.


IMHO it makes the code more readable and less confusing.
Comment 7 Kimmo Kinnunen 2010-10-05 05:46:33 PDT
Created attachment 69773 [details]
Patch try #2

Patch addressing the comments.
Comment 8 WebKit Commit Bot 2010-10-05 06:28:53 PDT
Comment on attachment 69773 [details]
Patch try #2

Clearing flags on attachment: 69773

Committed r69105: <http://trac.webkit.org/changeset/69105>
Comment 9 WebKit Commit Bot 2010-10-05 06:28:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 2010-10-05 07:42:57 PDT
(In reply to comment #8)
> (From update of attachment 69773 [details])
> Clearing flags on attachment: 69773
> 
> Committed r69105: <http://trac.webkit.org/changeset/69105>

It broke plugins/keyboard-events.html on Qt bot:
http://build.webkit.org/results/Qt%20Linux%20Release/r69105%20%2821509%29/plugins/keyboard-events-pretty-diff.html
Comment 11 Kimmo Kinnunen 2010-10-05 08:01:00 PDT
reopening due to a regression.
Comment 12 Kimmo Kinnunen 2010-10-05 08:16:41 PDT
Created attachment 69785 [details]
Fix for the test regression
Comment 13 Balazs Kelemen 2010-10-05 08:59:12 PDT
Comment on attachment 69773 [details]
Patch try #2

View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review

> WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579
> +}

We should reorganize the members in the enum to do this test it in O(1). Do you agree?
Comment 14 Csaba Osztrogonác 2010-10-05 09:05:58 PDT
(In reply to comment #13)
> (From update of attachment 69773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review
> 
> > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579
> > +}
> 
> We should reorganize the members in the enum to do this test it in O(1). Do you agree?

It isn't an enum, but defines. We don't need to reorganize it,
because GCC (and other compilers) generates a jump table
for this case construction to execute the binary in O(1). ;)
Comment 15 Balazs Kelemen 2010-10-05 09:51:13 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 69773 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review
> > 
> > > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579
> > > +}
> > 
> > We should reorganize the members in the enum to do this test it in O(1). Do you agree?
> 
> It isn't an enum, but defines. 

Doesn't matter.

> We don't need to reorganize it,
> because GCC (and other compilers) generates a jump table
> for this case construction to execute the binary in O(1). ;)

Hm, are you sure? It could do that but I am afraid that the compiler is not so clever (or at least not all the compilers). Do you have an experience about that?
Comment 16 WebKit Commit Bot 2010-10-05 12:13:10 PDT
Comment on attachment 69785 [details]
Fix for the test regression

Clearing flags on attachment: 69785

Committed r69134: <http://trac.webkit.org/changeset/69134>
Comment 17 WebKit Commit Bot 2010-10-05 12:13:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Kimmo Kinnunen 2010-10-05 22:55:29 PDT
(In reply to comment #13)
> (From update of attachment 69773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review
> 
> > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579
> > +}
> 
> We should reorganize the members in the enum to do this test it in O(1). Do you agree?

We should, especially if it shows up in your profiles as a hotspot. Did it? It really doesn't show up for me, though, so it seems a bit of a wasted effort, effort which I would like to spend in some other place.

I don't know the optimization technique. I cannot modify the enum as it is implemented in Qt.  Do you mean that the order of the case statements should match the order of the enum values? 

I'll attach an example output from my compiler. I'm no expert in this, so I cannot say for sure, but something seems to be rearranged by the looks of it.

If the output doesn't match your expectation, feel free to rearrange switch.
Comment 19 Kimmo Kinnunen 2010-10-05 22:58:06 PDT
Created attachment 69891 [details]
keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler
Comment 20 Kimmo Kinnunen 2010-10-05 23:16:32 PDT
Comment on attachment 69891 [details]
keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler

oops, I noticed we were talking about different function.
Comment 21 Kimmo Kinnunen 2010-10-05 23:31:35 PDT
Created attachment 69894 [details]
disambiguateKeyDownEvent compiled

Again, I'm no expert, but i think the switch is compiled to following sequence:

	.loc 1 505 0
	cmp	r1, #111
	bgt	.L399
	cmp	r1, #96
	bge	.L398
	cmp	r1, #57
	bgt	.L400
	cmp	r1, #48
	bge	.L398
	cmp	r1, #32
	beq	.L398
Comment 22 Balazs Kelemen 2010-10-06 02:29:24 PDT
Seems to be optimized enough. Thanks for dealing with that.