Bug 197233 – [GTK] Back/forward gesture snapshot always times out

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

[Alice Mikhaylenko]

See the attached patch.

[Alice Mikhaylenko]

Created attachment 368125 [details]
Patch

[Alice Mikhaylenko]

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.

[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 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?

[Alice Mikhaylenko]

> 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.

[Alice Mikhaylenko]

> 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

[Michael Catanzaro]

Note: this patch is NOT intended to be backported to 2.24.

[WebKit Commit Bot]

Comment on attachment 368125 [details]
Patch

Clearing flags on attachment: 368125

Committed r244648: <https://trac.webkit.org/changeset/244648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 197363

[Carlos Garcia Campos]

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.

[Alice Mikhaylenko]

Oh, right. Then I wonder how it even works and why didn't tests crash here?..

[Alice Mikhaylenko]

Created attachment 368465 [details]
Patch

[Alice Mikhaylenko]

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...

[Michael Catanzaro]

Comment on attachment 368465 [details]
Patch

I think it's fine. I see you added null checks everywhere needed. Sorry for missing this earlier.

[Michael Catanzaro]

If it breaks tests again (unlikely) we'll just revert it again, not the end of the world.

[Alice Mikhaylenko]

Comment on attachment 368465 [details]
Patch

Ok, marking as commit-queue? then.

[WebKit Commit Bot]

Comment on attachment 368465 [details]
Patch

Clearing flags on attachment: 368465

Committed r244744: <https://trac.webkit.org/changeset/244744>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.