Bug 247089

Summary:	[GTK] UI process crash when opening video settings
Product:	WebKit	Reporter:	Michael Catanzaro <mcatanzaro>
Component:	WebKitGTK	Assignee:	Carlos Garcia Campos <cgarcia>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	alicem, bugs-noreply, cgarcia, mcatanzaro
Priority:	P2
Version:	WebKit Nightly Build
Hardware:	PC
OS:	Linux
See Also:	https://bugs.webkit.org/show_bug.cgi?id=247092 https://bugs.webkit.org/show_bug.cgi?id=246716 https://bugs.webkit.org/show_bug.cgi?id=247141
Bug Depends on:
Bug Blocks:	210100

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.