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
Additional information: Window must be maximized.
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>,
Update: createWebTouchEvent () receives GDK_TOUCH_CANCEL which is currently not handled.
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.
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....
(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.
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 :)
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.
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 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.
Created attachment 332167 [details] Add GDK_TOUCH_CANCEL support - V2 Updated patch
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 > +2018-01-24 Jan-Michael Brummer <jan.brummer@tabos.org> > + [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.
Created attachment 332168 [details] Add GDK_TOUCH_CANCEL support - V3 Removed remaining nitpicks.
Comment on attachment 332168 [details] Add GDK_TOUCH_CANCEL support - V3 OK, let's try!
Comment on attachment 332168 [details] Add GDK_TOUCH_CANCEL support - V3 Clearing flags on attachment: 332168 Committed r227544: <https://trac.webkit.org/changeset/227544>
All reviewed patches have been landed. Closing bug.
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 > +2018-01-24 Jan-Michael Brummer <jan.brummer@tabos.org> > + [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 <jan.brummer@tabos.org> [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
Ah, I also failed to notice: that belongs in Source/WebKit/ChangeLog, not the toplevel ChangeLog. I'll push a fixup.
(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.
http://trac.webkit.org/changeset/227549/webkit