See the patch
Created attachment 372455 [details] Patch
Unfortunately, this doesn't catch the current PSON-related regression, since none of the tests trigger PSON. Some help with that would be appreciated.
(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?
(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 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.
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?
(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
(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.
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.
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.
Created attachment 372547 [details] Patch
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.
(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.
(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))?
Created attachment 372559 [details] Patch
(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 :)
(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 on attachment 372559 [details] Patch Clearing flags on attachment: 372559 Committed r246639: <https://trac.webkit.org/changeset/246639>
All reviewed patches have been landed. Closing bug.
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()?
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. :)
OK, thanks for checking!