247089 2022-10-26 14:39:46 -0700 [GTK] UI process crash when opening video settings 2022-11-25 06:30:25 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build PC Linux RESOLVED FIXED 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 P2 Normal --- 210100 1 mcatanzaro cgarcia alicem bugs-noreply cgarcia mcatanzaro oldest_to_newest 1908542 0 mcatanzaro 2022-10-26 14:39:46 -0700 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. 1908549 1 mcatanzaro 2022-10-26 15:10:04 -0700 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. 1908681 2 cgarcia 2022-10-27 05:22:18 -0700 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. 1908690 3 mcatanzaro 2022-10-27 06:12:37 -0700 (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. 1908692 4 alicem 2022-10-27 06:14:24 -0700 Right, it needs modifiers for WebExtensions and it's from this cycle. Previously it didn't look at the event at all. 1908758 5 mcatanzaro 2022-10-27 10:17:13 -0700 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. 1908781 6 mcatanzaro 2022-10-27 11:11:05 -0700 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. 1908785 7 mcatanzaro 2022-10-27 11:14:01 -0700 (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. 1914651 8 cgarcia 2022-11-25 01:59:11 -0800 Pull request: https://github.com/WebKit/WebKit/pull/6809 1914686 9 ews-feeder 2022-11-25 06:30:22 -0800 Committed 257023@main (f4b30ff111b4): <https://commits.webkit.org/257023@main> Reviewed commits have been landed. Closing PR #6809 and removing active labels.