Bug 197233 - [GTK] Back/forward gesture snapshot always times out
Summary: [GTK] Back/forward gesture snapshot always times out
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 197363
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-24 08:18 PDT by Alexander Mikhaylenko
Modified: 2019-04-29 12:08 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.93 KB, patch)
2019-04-24 08:20 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (13.44 KB, patch)
2019-04-29 10:21 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Mikhaylenko 2019-04-24 08:18:55 PDT
See the attached patch.
Comment 1 Alexander Mikhaylenko 2019-04-24 08:20:41 PDT
Created attachment 368125 [details]
Patch
Comment 2 Alexander Mikhaylenko 2019-04-24 08:21:43 PDT
I think it's a regression after https://trac.webkit.org/changeset/243094/webkit. But I'm not 100% sure, because that crash also introduced a crash, so it can also be any change until that crash was fixed.
Comment 3 EWS Watchlist 2019-04-24 08:23:20 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 4 Michael Catanzaro 2019-04-24 14:35:23 PDT
Comment on attachment 368125 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:207
> +    bool isBackForwardNavigationGestureEnabled { false };

It's a priv struct: let glib zero the memory for us.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1631
> +    WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;

Moved by mistake?
Comment 5 Alexander Mikhaylenko 2019-04-24 14:38:31 PDT
> It's a priv struct: let glib zero the memory for us.

Ok

> Moved by mistake?

No, it was inside an #if PLATFORM(X11) && USE(TEXTURE_MAPPER_GL) && !USE(REDIRECTED_XCOMPOSITE_WINDOW) block.
Comment 6 Alexander Mikhaylenko 2019-04-24 14:58:36 PDT
> It's a priv struct: let glib zero the memory for us.

After looking at the code again: other values in the same struct are initialized[1], so this was done for consistency.

1: https://github.com/webkit/webkit/blob/master/Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp#L164-L193
Comment 7 Michael Catanzaro 2019-04-25 09:32:02 PDT
Note: this patch is NOT intended to be backported to 2.24.
Comment 8 WebKit Commit Bot 2019-04-25 09:58:14 PDT
Comment on attachment 368125 [details]
Patch

Clearing flags on attachment: 368125

Committed r244648: <https://trac.webkit.org/changeset/244648>
Comment 9 WebKit Commit Bot 2019-04-25 09:58:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Commit Bot 2019-04-29 05:21:10 PDT
Re-opened since this is blocked by bug 197363
Comment 11 Carlos Garcia Campos 2019-04-29 05:23:24 PDT
Comment on attachment 368125 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1650
> +    priv->viewGestureController = std::make_unique<WebKit::ViewGestureController>(*priv->pageProxy);
> +    priv->viewGestureController->setSwipeGestureEnabled(priv->isBackForwardNavigationGestureEnabled);

This requires to null-check viewGestureController everywhere before using it. And change webkitWebViewBaseViewGestureController() to return a pointer.
Comment 12 Alexander Mikhaylenko 2019-04-29 06:28:35 PDT
Oh, right. Then I wonder how it even works and why didn't tests crash here?..
Comment 13 Alexander Mikhaylenko 2019-04-29 10:21:31 PDT
Created attachment 368465 [details]
Patch
Comment 14 Alexander Mikhaylenko 2019-04-29 10:22:27 PDT
Can anybody give me the list of tests that started to fail? I found one (animations/crash-on-removing-animation.html), but there might be others. Running all tests 2 times (with and without the patch) would take forever here...
Comment 15 Michael Catanzaro 2019-04-29 10:31:56 PDT
Comment on attachment 368465 [details]
Patch

I think it's fine. I see you added null checks everywhere needed. Sorry for missing this earlier.
Comment 16 Michael Catanzaro 2019-04-29 10:32:30 PDT
If it breaks tests again (unlikely) we'll just revert it again, not the end of the world.
Comment 17 Alexander Mikhaylenko 2019-04-29 11:05:19 PDT
Comment on attachment 368465 [details]
Patch

Ok, marking as commit-queue? then.
Comment 18 WebKit Commit Bot 2019-04-29 12:08:23 PDT
Comment on attachment 368465 [details]
Patch

Clearing flags on attachment: 368465

Committed r244744: <https://trac.webkit.org/changeset/244744>
Comment 19 WebKit Commit Bot 2019-04-29 12:08:25 PDT
All reviewed patches have been landed.  Closing bug.