Bug 247089 – [GTK] UI process crash when opening video settings

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

[Michael Catanzaro]

Moving this from https://gitlab.gnome.org/GNOME/epiphany/-/issues/1891. When clicking the settings cog at the bottom-right corner of an HTML video element, the Epiphany UI process crashes because the WebKitWebView::context-menu signal is emitted with a NULL event parameter, which is illegal as that parameter is not documented to be nullable. Our documentation says:

"The event is expected to be one of the following types: a GdkEventButton of type GDK_BUTTON_PRESS when the context menu was triggered with mouse. a GdkEventKey of type GDK_KEY_PRESS if the keyboard was used to show the menu. a generic GdkEvent of type GDK_NOTHING when the GtkWidget::popup-menu signal was used to show the context menu."

WebKitWebViewBase is not prepared to handle the case where the context menu is opened by a Messages::WebPageProxy::ShowContextMenu message sent by the web process.

[Michael Catanzaro]

This is easy enough to fix for GTK 3, but I fear we might need an API change for GTK 4. I don't see how to generate a dummy GdkEvent with GTK 4.

[Carlos Garcia Campos]

I think we are still in time to try different approach in the new API. I would just stop using GdkEvent in context and popup menu signals. We can add a custom boxed type that can be shared with wpe and provide the info we want there.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #2)
> I think we are still in time to try different approach in the new API.

Yes.

> I would just stop using GdkEvent in context and popup menu signals. We can add
> a custom boxed type that can be shared with wpe and provide the info we want
> there.

Hmm, that could work, and would resolve a separate problem with the WPE API as well... although we'd need to agree on what info this type would need to convey. Epiphany only looks at gdk_event_get_type() to check for GDK_BUTTON_PRESS, then in that case, it uses gdk_event_get_modifier_state() and passes the GdkModifierType along to WebExtensions to decide what to do with it. That code is probably new, which would explain why we didn't notice before now.

[Alice Mikhaylenko]

Right, it needs modifiers for WebExtensions and it's from this cycle. Previously it didn't look at the event at all.

[Michael Catanzaro]

Well adding a new type is more complicated than I was expected. I think I'll do a quick fix for GTK 3 in a separate bug, to ensure a dummy GdkEvent always gets created, and leave this bug for deciding how to fix GTK 4 where dummy GdkEvents are no longer possible.

[Michael Catanzaro]

Also we need to consider what to do with the show-option-menu signal as well. That's the other place where we pass a GdkEvent via a signal handler. See bug #246716.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Well adding a new type is more complicated than I was expected. I think I'll
> do a quick fix for GTK 3 in a separate bug, to ensure a dummy GdkEvent
> always gets created,

Bug #247141

> and leave this bug for deciding how to fix GTK 4 where
> dummy GdkEvents are no longer possible.

Any proposals for how this API should look? For GTK, it might be reasonable to just make the GdkEvent nullable, although that won't do anything to help WPE.

[Carlos Garcia Campos]

Pull request: https://github.com/WebKit/WebKit/pull/6809

[EWS]

Committed 257023@main (f4b30ff111b4): <https://commits.webkit.org/257023@main>

Reviewed commits have been landed. Closing PR #6809 and removing active labels.