Bug 181996 – [GTK] Page crash after swipe gesture running GNOME3 under wayland

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan-Michael Brummer]

Leaving epiphany with a gesture (swipe from left to open shell overview, swipe from bottom to open OSK) and re-entering causes a crash of the current page.

Page is terminate due to a SIGKILL caused by: Received an invalid message "WebPageProxy.DidReceiveEvent" from the web process.
Due to SIGKILL no backtrace can be generated.

Always reproducible.

Notes:
 - Tablet running Fedora with GNOME 3 under Wayland.
 - Does not occure running GNOME 3 under X.

Versions:
 - Fedora 27
 - GNOME 3.26
 - WebkitGTK 2.18.5

[Jan-Michael Brummer]

Additional information: Window must be maximized.

[Jan-Michael Brummer]

Created attachment 332075 [details]
GDB log of minibrowser

Compiled webkit on my tablet including minibrowser. Using this app i can reproduce the issue and could create a gdb log. See attachment.

(gdb) bt full
#0  0x00007fffecbeda71 in WTFCrash () at /home/jbrummer/Downloads/webkitgtk-2.18.5/lib/libjavascriptcoregtk-4.0.so.18
#1  0x00007ffff141202a in WebKit::WebEventFactory::createWebTouchEvent(_GdkEvent const*, WTF::Vector<WebKit::WebPlatformTouchPoint, 0ul, WTF::CrashOnOverflow, 16ul>&&) ()
    at /home/jbrummer/Downloads/webkitgtk-2.18.5/lib/libwebkit2gtk-4.0.so.37
#2  0x00007ffff1410fa1 in WebKit::NativeWebTouchEvent::NativeWebTouchEvent(_GdkEvent*, WTF::Vector<WebKit::WebPlatformTouchPoint, 0ul, WTF::CrashOnOverflow, 16ul>&&) ()
    at /home/jbrummer/Downloads/webkitgtk-2.18.5/lib/libwebkit2gtk-4.0.so.37
#3  0x00007ffff14daa1a in webkitWebViewBaseTouchEvent(_GtkWidget*, _GdkEventTouch*) () at /home/jbrummer/Downloads/webkitgtk-2.18.5/lib/libwebkit2gtk-4.0.so.37
#4  0x00007ffff0173b77 in _gtk_marshal_BOOLEAN__BOXEDv (closure=0x48b450, return_value=0x7fffffffd920, instance=<optimized out>, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>,

[Jan-Michael Brummer]

Update: createWebTouchEvent () receives GDK_TOUCH_CANCEL which is currently not handled.

[Jan-Michael Brummer]

I've successfully added support for GDK_TOUCH_CANCEL which fixes the crash. As i've created it on the release version i will checkout the git version of webkitgtk and provide a proper patch.

[Michael Catanzaro]

Oh wow, thanks! You might want to review https://webkit.org/contributing-code/ before attaching your patch. Key steps are (a) run the prepare-ChangeLog script, and (b) set the r? and cq? flags on the Bugzilla attachment.

I notice that in webkitWebViewBaseTouchEvent() (in WebKitWebViewBase.cpp) also needs fixed. There a NativeWebTouchEvent is created from the received GdkEventTouch* regardless of its GdkEventType. But that is illegal unless the GdkEventType is GDK_TOUCH_BEGIN, GDK_TOUCH_UPDATE, or GDK_TOUCH_END, according to the switch case you already found in createWebTouchEvent(). So webkitWebViewBaseTouchEvent() needs to be updated as well, to not create a new NativeWebTouchEvent in this case. So in the default case, webkitWebViewBaseTouchEvent() ought to return FALSE instead of continuing on. That way, if some new GdkEventType for GdkEventTouch* were to be added in the future (not likely for GTK+ 3, but it could happen in future major versions) WebKit will at least not crash like this.

Now, according to the GTK+ documentation, the only other valid GdkEventType for a GdkEventTouch* is GDK_TOUCH_CANCEL, so fortunately you're right that that's the only additional GdkEventType we need to support. I'm going to blindly guess that should be handled exactly the same as GDK_TOUCH_END in webkitWebViewBaseTouchEvent...? So that switch case in webkitWebViewBaseTouchEvent() should *probably* just fall through:

    case GDK_TOUCH_END:
    case GDK_TOUCH_CANCEL:
        ASSERT(priv->touchEvents.contains(sequence));
        priv->touchEvents.remove(sequence);
        break;

At least, that *seems* like it would be the right thing to do....

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> So in the default case,
> webkitWebViewBaseTouchEvent() ought to return FALSE instead of continuing
> on.

Actually, I've just filed bug #182031 to convert these functions to return GDK_EVENT_STOP and GDK_EVENT_PROPAGATE, so it would be better to go ahead and use GDK_EVENT_PROPAGATE instead.

[Jan-Michael Brummer]

Yes, i had to adjust three different places to get it working. And yes, i implemented it as an alias for GDK_TOUCH_END.

Patch will take a couple of hours. It is still compiling the libraries on my tablet...

Hopefully it will be integrated soon as i do not want to compile it on my smartphone :)

[Jan-Michael Brummer]

Created attachment 332155 [details]
Add GDK_TOUCH_CANCEL support

Added support for GDK_TOUCH_CANCEL:
 - to fix the gesture bug
 - updated other code paths to respect also GDK_TOUCH_CANCEL where only GDK_TOUCH_END has been checked.

[EWS Watchlist]

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 332155 [details]
Add GDK_TOUCH_CANCEL support

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

This looks right, thanks! I'm just going to request just minor changes:

> Source/WebKit/ChangeLog:3
> +        Add missing GDK_TOUCH_CANCEL support which fixes page crashes due to cancelled gestures.

We prefer to use the title of the bug for the first line here:

"[GTK] Page crash after swipe gesture running GNOME3 under wayland"

Then you can move this description down after the Reviewed by line.

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:376
> +            // Fall-through

That reminds me, we actually have a macro for this that uses attribute [[fallthrough]] or __attribute__ ((fallthrough)) or one of the compiler-specific variants. We should use it here! #include <wtf/Compiler.h> and then:

  case GDK_TOUCH_CANCEL:
      FALLTHROUGH;
  case GDK_TOUCH_END:
      // ...

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:944
> +        // Fall-through

Use FALLTHROUGH here, too.

[Jan-Michael Brummer]

Created attachment 332167 [details]
Add GDK_TOUCH_CANCEL support - V2

Updated patch

[Michael Catanzaro]

Comment on attachment 332167 [details]
Add GDK_TOUCH_CANCEL support - V2

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

Remaining nits:

> Source/WebKit/ChangeLog:2
> [phone]  Jan-Michael Brummer  <[e-mail]>
> +        [GTK] Page crash after swipe gesture running GNOME3 under wayland

There should be a blank line here.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:71
>  #include <wtf/text/CString.h>
> +#include <wtf/Compiler.h>

I'm surprised 'webkit-patch upload' didn't spit warnings at you about this... uppercase letters sort before lowercase letters, so if you used that command you would have gotten an alphabetization warning here. Normally Bugzilla complains too, but maybe it only looks at the patch if you set the r? flag.

[Jan-Michael Brummer]

Created attachment 332168 [details]
Add GDK_TOUCH_CANCEL support - V3

Removed remaining nitpicks.

[Michael Catanzaro]

Comment on attachment 332168 [details]
Add GDK_TOUCH_CANCEL support - V3

OK, let's try!

[WebKit Commit Bot]

Comment on attachment 332168 [details]
Add GDK_TOUCH_CANCEL support - V3

Clearing flags on attachment: 332168

Committed r227544: <https://trac.webkit.org/changeset/227544>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Comment on attachment 332168 [details]
Add GDK_TOUCH_CANCEL support - V3

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

> Source/WebKit/ChangeLog:4
> [phone]  Jan-Michael Brummer  <[e-mail]>
> +        [GTK] Page crash after swipe gesture running GNOME3 under wayland
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=181996

I didn't notice in time, but the format I had been hoping for was:

2018-01-24  Jan-Michael Brummer  <[e-mail]>

        [GTK] Page crash after swipe gesture running GNOME3 under wayland
        https://bugs.webkit.org/show_bug.cgi?id=181996

Like all the other entries in the ChangeLog. Ah well. :P

[Michael Catanzaro]

Ah, I also failed to notice: that belongs in Source/WebKit/ChangeLog, not the toplevel ChangeLog. I'll push a fixup.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #18)
> Ah, I also failed to notice: that belongs in Source/WebKit/ChangeLog, not
> the toplevel ChangeLog. I'll push a fixup.

Wait, no, you got it right, I just confused myself by looking in the wrong file! I can do this.... I'll just change the line spacing, then.

[Michael Catanzaro]

http://trac.webkit.org/changeset/227549/webkit