Bug 163011

Summary: [GTK] UIProcess crashes when using Japanese IM
Product: WebKit Reporter: Tomas Popela <tpopela>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, cgarcia, commit-queue, gns, mcatanzaro, mrobinson
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugzilla.redhat.com/show_bug.cgi?id=1381603
Bug Depends on: 163116    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
Fix review comments
none
Fix crash none

Description Tomas Popela 2016-10-06 06:49:11 PDT
When writing any content with Japanese IM activated (ie. in the inspector's console, or in the contenteditable element), the UIProcess crashes.
Comment 1 Tomas Popela 2016-10-06 06:57:49 PDT
Created attachment 290822 [details]
Proposed patch
Comment 2 WebKit Commit Bot 2016-10-06 06:59:21 PDT
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
Comment 3 Michael Catanzaro 2016-10-06 07:29:39 PDT
Comment on attachment 290822 [details]
Proposed patch

(Leaving cq+ for Carlos)
Comment 4 Carlos Garcia Campos 2016-10-06 08:24:22 PDT
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.
Comment 5 Tomas Popela 2016-10-06 08:30:42 PDT
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..
Comment 6 Tomas Popela 2016-10-07 03:57:24 PDT
Created attachment 290920 [details]
Fix review comments
Comment 7 WebKit Commit Bot 2016-10-07 05:06:52 PDT
Comment on attachment 290920 [details]
Fix review comments

Clearing flags on attachment: 290920

Committed r206909: <http://trac.webkit.org/changeset/206909>
Comment 8 WebKit Commit Bot 2016-10-07 05:06:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2016-10-07 09:39:09 PDT
Re-opened since this is blocked by bug 163116
Comment 10 Carlos Garcia Campos 2016-10-07 09:43:31 PDT
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.
Comment 11 Tomas Popela 2016-10-09 22:40:35 PDT
Created attachment 291063 [details]
Fix crash

Fix the crash that was found by Carlos (thank you).
Comment 12 WebKit Commit Bot 2016-10-10 00:15:10 PDT
Comment on attachment 291063 [details]
Fix crash

Clearing flags on attachment: 291063

Committed r206985: <http://trac.webkit.org/changeset/206985>
Comment 13 WebKit Commit Bot 2016-10-10 00:15:15 PDT
All reviewed patches have been landed.  Closing bug.