Bug 198995 - [GTK] Enable navigation swipe layout tests
Summary: [GTK] Enable navigation swipe layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-19 05:28 PDT by Alexander Mikhaylenko
Modified: 2019-06-21 07:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.47 KB, patch)
2019-06-19 05:47 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (17.58 KB, patch)
2019-06-20 01:40 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (17.95 KB, patch)
2019-06-20 08:31 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-06-19 05:28:27 PDT
See the patch
Comment 1 Alexander Mikhaylenko 2019-06-19 05:47:58 PDT
Created attachment 372455 [details]
Patch
Comment 2 Alexander Mikhaylenko 2019-06-19 05:49:16 PDT
Unfortunately, this doesn't catch the current PSON-related regression, since none of the tests trigger PSON. Some help with that would be appreciated.
Comment 3 Carlos Garcia Campos 2019-06-19 05:53:09 PDT
(In reply to Alexander Mikhaylenko from comment #2)
> Unfortunately, this doesn't catch the current PSON-related regression, since
> none of the tests trigger PSON. Some help with that would be appreciated.

what regression?
Comment 4 Alexander Mikhaylenko 2019-06-19 05:57:02 PDT
(In reply to Carlos Garcia Campos from comment #3)
> (In reply to Alexander Mikhaylenko from comment #2)
> > Unfortunately, this doesn't catch the current PSON-related regression, since
> > none of the tests trigger PSON. Some help with that would be appreciated.
> 
> what regression?

https://bugs.webkit.org/show_bug.cgi?id=198996
Comment 5 Carlos Garcia Campos 2019-06-19 05:59:01 PDT
Comment on attachment 372455 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:443
> +    GdkEvent* event = createScrollEvent(m_webPageProxy.viewWidget(), delta, 0);

This event is leaked, I think. Better use GUniquePtr to ensure we don't leak it.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:451
> +    GdkEvent* event = createScrollEvent(m_webPageProxy.viewWidget(), 0, 0);

Ditto.
Comment 6 Alexander Mikhaylenko 2019-06-19 07:33:48 PDT
Hm, agree. However, when you pass a row pointer to gtk_widget_event(), GUniquePtr destroys it before it can be used.

Which is to be expected, but I don't understand why it works in WebKitWebViewBase::TouchEventController::startDrag() and other functions there. Isn't it exactly same situation?
Comment 7 Michael Catanzaro 2019-06-19 09:23:57 PDT
(In reply to Alexander Mikhaylenko from comment #6)
> Hm, agree. However, when you pass a row pointer to gtk_widget_event(),
> GUniquePtr destroys it before it can be used.

Er, what did you mean here? Did you mean to say "raw pointer"?

You currently have:

GdkEvent* event = createScrollEvent(m_webPageProxy.viewWidget(), delta, 0);
gtk_widget_event(m_webPageProxy.viewWidget(), event);
return true;

The suggested fix is:

GUniquePtr<GdkEvent> event(createScrollEvent(m_webPageProxy.viewWidget(), delta, 0);
gtk_widget_event(m_webPageProxy.viewWidget(), event.get()); // event still valid
return true; // event destroyed here
Comment 8 Alexander Mikhaylenko 2019-06-19 10:17:06 PDT
(In reply to Michael Catanzaro from comment #7)
> Er, what did you mean here? Did you mean to say "raw pointer"?

Oops. Yes, that's what I meant. :)

> The suggested fix is:
> 
> GUniquePtr<GdkEvent> event(createScrollEvent(m_webPageProxy.viewWidget(),
> delta, 0);
> gtk_widget_event(m_webPageProxy.viewWidget(), event.get()); // event still
> valid
> return true; // event destroyed here

That's exactly what I tried. It crashes. Just as

```
GdkEvent* event = createScrollEvent(m_webPageProxy.viewWidget(), delta, 0);
gtk_widget_event(m_webPageProxy.viewWidget(), event);
g_free(event); // gdk_event_free(event) as well
return true;

```
crashes too.
Comment 9 Michael Catanzaro 2019-06-19 12:30:12 PDT
Well where is the crash?

You write: "GUniquePtr destroys it before it can be used." But GUniquePtr is alive until after the call to gtk_widget_event() completes. Is the crash inside gtk_widget_event() itself?

The event parameter of gtk_widget_event() is not documented to be transfer full, so either we have to free it (in which case GUniquePtr is appropriate), or there is an annotation bug. If we don't have to free it, you can still use GUniquePtr and then call .release() in order to document ownership and avoid ever assigning ownership to a raw pointer.
Comment 10 Alexander Mikhaylenko 2019-06-20 01:40:10 PDT
Ok, I completely misinterpreted the crash. Sorry :/

It looks like when the event is freed, event->scroll.window gets deref'd, so I needed to ref it.
Comment 11 Alexander Mikhaylenko 2019-06-20 01:40:35 PDT
Created attachment 372547 [details]
Patch
Comment 12 Michael Catanzaro 2019-06-20 07:44:01 PDT
Comment on attachment 372547 [details]
Patch

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

LGTM

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:424
> +    g_object_ref(window);
> +    event->scroll.window = window;

Better/clearer:

event->scroll.window = g_object_ref(window);

> LayoutTests/platform/gtk/TestExpectations:1228
> +# Flaky, the order of lines in the output may change.
> +webkit.org/b/170484 swipe/main-frame-pinning-requirement.html [ Pass Failure ]

This should be lower in the expectations file under the flaky section, since it's a failure that should be fixed, rather than a failure that we're OK with having forever like the other test you've added here.
Comment 13 Alexander Mikhaylenko 2019-06-20 07:49:27 PDT
(In reply to Michael Catanzaro from comment #12)
> Better/clearer:
> 
> event->scroll.window = g_object_ref(window);

Needs an additional cast. :)

```
../../Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp: In function ‘WTF::GUniquePtr<_GdkEvent> WebKit::createScrollEvent(GtkWidget*, double, double)’:
../../Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:423:40: error: invalid conversion from ‘gpointer’ {aka ‘void*’} to ‘GdkWindow*’ {aka ‘_GdkWindow*’} [-fpermissive]
     event->scroll.window = g_object_ref(window);
                            ~~~~~~~~~~~~^~~~~~~~
```

If
```
    event->scroll.window = reinterpret_cast<GdkWindow*>(g_object_ref(window));
```
is still fine (honestly, looks less clean to me), then sure. :)

> This should be lower in the expectations file under the flaky section, since
> it's a failure that should be fixed, rather than a failure that we're OK
> with having forever like the other test you've added here.

Ok.
Comment 14 Michael Catanzaro 2019-06-20 08:25:20 PDT
(In reply to Alexander Mikhaylenko from comment #13)
> If
> ```
>     event->scroll.window =
> reinterpret_cast<GdkWindow*>(g_object_ref(window));
> ```
> is still fine (honestly, looks less clean to me), then sure. :)

Meh, how about GDK_WINDOW(g_object_ref(window))?
Comment 15 Alexander Mikhaylenko 2019-06-20 08:31:18 PDT
Created attachment 372559 [details]
Patch
Comment 16 Alexander Mikhaylenko 2019-06-20 08:32:47 PDT
(In reply to Michael Catanzaro from comment #14)
> Meh, how about GDK_WINDOW(g_object_ref(window))?

Oh, I assumed that was against the code style. Ok then :)
Comment 17 Michael Catanzaro 2019-06-20 09:35:42 PDT
(In reply to Alexander Mikhaylenko from comment #16)
> Oh, I assumed that was against the code style. Ok then :)

Nope, GObject casts are fine. Even better when the code is not performance-sensitive, since they type-check for us.
Comment 18 WebKit Commit Bot 2019-06-20 10:05:25 PDT
Comment on attachment 372559 [details]
Patch

Clearing flags on attachment: 372559

Committed r246639: <https://trac.webkit.org/changeset/246639>
Comment 19 WebKit Commit Bot 2019-06-20 10:05:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Michael Catanzaro 2019-06-20 17:14:22 PDT
By chance, I noticed this pattern from WebKitWebViewBase.cpp:

        scrollEvent->scroll.window = event->window ? GDK_WINDOW(g_object_ref(event->window)) : nullptr;

We probably need to null-check it here too before passing it to g_object_ref()?
Comment 21 Alexander Mikhaylenko 2019-06-21 00:22:53 PDT
Hmm, when can window be empty? Note that here we're using GdkWindow of the WebView. I have a suspicion that if it's null, it would fail much earlier than here. :)
Comment 22 Michael Catanzaro 2019-06-21 07:05:25 PDT
OK, thanks for checking!