Bug 163011 – [GTK] UIProcess crashes when using Japanese IM

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Tomas Popela]

When writing any content with Japanese IM activated (ie. in the inspector's console, or in the contenteditable element), the UIProcess crashes.

[Tomas Popela]

Created attachment 290822 [details]
Proposed patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 290822 [details]
Proposed patch

(Leaving cq+ for Carlos)

[Carlos Garcia Campos]

Comment on attachment 290822 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:709
> +    // We need to copy the event as otherwise it could be destroyed before we
> +    // reach the lambda body.

This could be a single line.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:710
> +    GdkEvent* event = gdk_event_copy(reinterpret_cast<GdkEvent*>(keyEvent));

GUniquePtr<GdkEvent> event(gdk_event_copy(reinterpret_cast<GdkEvent*>(keyEvent)));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:711
> +    priv->inputMethodFilter.filterKeyEvent(&event->key, [priv, event](const WebCore::CompositionResults& compositionResults, InputMethodFilter::EventFakedForComposition faked) {

[priv, event = WTFMove(event)]

With this, the lambda takes the ownership and you don't need to call gdk_event_free(). Maybe it's better to call this keyEvent and then you don't have to rename all other cases.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:733
> +    // We need to copy the event as otherwise it could be destroyed before we
> +    // reach the lambda body.
> +    GdkEvent* event = gdk_event_copy(reinterpret_cast<GdkEvent*>(keyEvent));
> +    priv->inputMethodFilter.filterKeyEvent(&event->key, [priv, event](const WebCore::CompositionResults& compositionResults, InputMethodFilter::EventFakedForComposition faked) {

Ditto.

[Tomas Popela]

Thank you Carlos for the feedback!

(In reply to comment #4)
> Maybe it's better to call this keyEvent and then you don't
> have to rename all other cases.

I would rename it as we don't want to have a GdkEventKey event and GdkEvent keyEvent..

[Tomas Popela]

Created attachment 290920 [details]
Fix review comments

[WebKit Commit Bot]

Comment on attachment 290920 [details]
Fix review comments

Clearing flags on attachment: 290920

Committed r206909: <http://trac.webkit.org/changeset/206909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 163116

[Carlos Garcia Campos]

Comment on attachment 290920 [details]
Fix review comments

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:710
> +    priv->inputMethodFilter.filterKeyEvent(&event->key, [priv, event = WTFMove(event)](const WebCore::CompositionResults& compositionResults, InputMethodFilter::EventFakedForComposition faked) {

You can use event->key here, because even has already been moved to the lambda, use keyEvent instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:730
> +    priv->inputMethodFilter.filterKeyEvent(&event->key, [priv, event = WTFMove(event)](const WebCore::CompositionResults& compositionResults, InputMethodFilter::EventFakedForComposition faked) {

Ditto.

[Tomas Popela]

Created attachment 291063 [details]
Fix crash

Fix the crash that was found by Carlos (thank you).

[WebKit Commit Bot]

Comment on attachment 291063 [details]
Fix crash

Clearing flags on attachment: 291063

Committed r206985: <http://trac.webkit.org/changeset/206985>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.