Bug 247089 - [GTK] UI process crash when opening video settings
Summary: [GTK] UI process crash when opening video settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Carlos Garcia Campos
URL:
Keywords:
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2022-10-26 14:39 PDT by Michael Catanzaro
Modified: 2022-11-25 06:30 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-10-26 14:39:46 PDT
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.
Comment 1 Michael Catanzaro 2022-10-26 15:10:04 PDT
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.
Comment 2 Carlos Garcia Campos 2022-10-27 05:22:18 PDT
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.
Comment 3 Michael Catanzaro 2022-10-27 06:12:37 PDT
(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.
Comment 4 Alice Mikhaylenko 2022-10-27 06:14:24 PDT
Right, it needs modifiers for WebExtensions and it's from this cycle. Previously it didn't look at the event at all.
Comment 5 Michael Catanzaro 2022-10-27 10:17:13 PDT
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.
Comment 6 Michael Catanzaro 2022-10-27 11:11:05 PDT
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.
Comment 7 Michael Catanzaro 2022-10-27 11:14:01 PDT
(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.
Comment 8 Carlos Garcia Campos 2022-11-25 01:59:11 PST
Pull request: https://github.com/WebKit/WebKit/pull/6809
Comment 9 EWS 2022-11-25 06:30:22 PST
Committed 257023@main (f4b30ff111b4): <https://commits.webkit.org/257023@main>

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