Bug 198995 – [GTK] Enable navigation swipe layout tests

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

[Alice Mikhaylenko]

See the patch

[Alice Mikhaylenko]

Created attachment 372455 [details]
Patch

[Alice Mikhaylenko]

Unfortunately, this doesn't catch the current PSON-related regression, since none of the tests trigger PSON. Some help with that would be appreciated.

[Carlos Garcia Campos]

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

[Alice Mikhaylenko]

(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

[Carlos Garcia Campos]

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.

[Alice Mikhaylenko]

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?

[Michael Catanzaro]

(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

[Alice Mikhaylenko]

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

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Created attachment 372547 [details]
Patch

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

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

[Michael Catanzaro]

(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))?

[Alice Mikhaylenko]

Created attachment 372559 [details]
Patch

[Alice Mikhaylenko]

(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 :)

[Michael Catanzaro]

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

[WebKit Commit Bot]

Comment on attachment 372559 [details]
Patch

Clearing flags on attachment: 372559

Committed r246639: <https://trac.webkit.org/changeset/246639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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()?

[Alice Mikhaylenko]

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

[Michael Catanzaro]

OK, thanks for checking!