| Summary: | [GTK] Back/forward gesture snapshot always times out | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alice Mikhaylenko <alicem> | ||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | berto, bugs-noreply, cgarcia, commit-queue, ews-watchlist, gustavo, mcatanzaro | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Linux | ||||||||
| Bug Depends on: | 197363 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Alice Mikhaylenko
2019-04-24 08:18:55 PDT
Created attachment 368125 [details]
Patch
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. 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 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? > 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. > 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 Note: this patch is NOT intended to be backported to 2.24. Comment on attachment 368125 [details] Patch Clearing flags on attachment: 368125 Committed r244648: <https://trac.webkit.org/changeset/244648> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 197363 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. Oh, right. Then I wonder how it even works and why didn't tests crash here?.. Created attachment 368465 [details]
Patch
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 on attachment 368465 [details]
Patch
I think it's fine. I see you added null checks everywhere needed. Sorry for missing this earlier.
If it breaks tests again (unlikely) we'll just revert it again, not the end of the world. Comment on attachment 368465 [details]
Patch
Ok, marking as commit-queue? then.
Comment on attachment 368465 [details] Patch Clearing flags on attachment: 368465 Committed r244744: <https://trac.webkit.org/changeset/244744> All reviewed patches have been landed. Closing bug. |