Bug 44935 – [Qt] Web process crash when pressing modifiers in input field

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

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.

[Kimmo Kinnunen]

Created attachment 66029 [details]
Patch fixing the crash

[Ariya Hidayat]

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?

[Kimmo Kinnunen]

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

[Ariya Hidayat]

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

[Kimmo Kinnunen]

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

[Ariya Hidayat]

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

[Kimmo Kinnunen]

Created attachment 69773 [details]
Patch try #2

Patch addressing the comments.

[WebKit Commit Bot]

Comment on attachment 69773 [details]
Patch try #2

Clearing flags on attachment: 69773

Committed r69105: <http://trac.webkit.org/changeset/69105>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Kimmo Kinnunen]

reopening due to a regression.

[Kimmo Kinnunen]

Created attachment 69785 [details]
Fix for the test regression

[Balazs Kelemen]

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?

[Csaba Osztrogonác]

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

[Balazs Kelemen]

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

[WebKit Commit Bot]

Comment on attachment 69785 [details]
Fix for the test regression

Clearing flags on attachment: 69785

Committed r69134: <http://trac.webkit.org/changeset/69134>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kimmo Kinnunen]

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

[Kimmo Kinnunen]

Created attachment 69891 [details]
keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler

[Kimmo Kinnunen]

Comment on attachment 69891 [details]
keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler

oops, I noticed we were talking about different function.

[Kimmo Kinnunen]

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

[Balazs Kelemen]

Seems to be optimized enough. Thanks for dealing with that.