SSIA
Created attachment 433401 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 433401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433401&action=review API approval 1 of 2 > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:400 > + Remove this extra blank line > Tools/ChangeLog:9 > + new API tests was adding, checking support for getDisplayMedia(). As this requires user added > Tools/glib/api_test_runner.py:175 > + env = self._test_env.copy() > + if os.path.basename(test_program) in self.TESTS_REQUIRING_INTERNAL_INJECTED_BUNDLE: > + env["TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_FILENAME"] = "libTestWebKitAPIInternalInjectedBundle.so" Hmmmm, this is unfortunate as the test is really not going to work without the test runner, is it? Could this be set by the code itself?
Comment on attachment 433401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433401&action=review >> Tools/glib/api_test_runner.py:175 >> + env["TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_FILENAME"] = "libTestWebKitAPIInternalInjectedBundle.so" > > Hmmmm, this is unfortunate as the test is really not going to work without the test runner, is it? Could this be set by the code itself? Which code? The UIClient API test requires this because the getDisplayMedia() call cannot work without user interaction, which we fake thanks to the internals.withUserGesture() thing.
The changes I did in the MiniBrowser are not very good TBH. Using the URI entry was perhaps not a great idea. For the displayMedia indicator Chomium uses a custom overlay widget. Something similar would be nice but I don't know how to achieve that.
Comment on attachment 433401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433401&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1381 > + * @WEBKIT_MEDIA_CAPTURE_STATE_NONE or @WEBKIT_MEDIA_CAPTURE_STATE_MUTED. Something that is not clear to me reading the API documentation: What happens if a media request permission is accepted, then the capture-state is set to WEBKIT_MEDIA_CAPTURE_STATE_NONE, and then a web page uses getUserMedia() again? Will there be a new permission request, or does one need to use the setter with WEBKIT_MEDIA_CAPTURE_STATE_ACTIVE before capture works again? I feel that this deserves clarification. The same question applies to the three introduced properties, as I suppose internally WebKit handles them in the same way. > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:620 > +webkit_web_view_get_camera_capture_state (WebKitWebView* web_view); Indentation in public headers should follow the GLib/GTK convention of left-aligning the opening parenthesis of function parameters — I know, it can get annoying, but it's what we do ¯\_(ツ)_/¯ > Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:597 > +webkit_web_view_get_camera_capture_state (WebKitWebView *web_view); …and here it's perfectly indented :) >>> Tools/glib/api_test_runner.py:175 >>> + env["TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_FILENAME"] = "libTestWebKitAPIInternalInjectedBundle.so" >> >> Hmmmm, this is unfortunate as the test is really not going to work without the test runner, is it? Could this be set by the code itself? > > Which code? > The UIClient API test requires this because the getDisplayMedia() call cannot work without user interaction, which we fake thanks to the internals.withUserGesture() thing. As a matter of fact, I think there are tests which we are currently skipping which needs this :)
Comment on attachment 433401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433401&action=review >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1381 >> + * @WEBKIT_MEDIA_CAPTURE_STATE_NONE or @WEBKIT_MEDIA_CAPTURE_STATE_MUTED. > > Something that is not clear to me reading the API documentation: What happens > if a media request permission is accepted, then the capture-state is set to > WEBKIT_MEDIA_CAPTURE_STATE_NONE, and then a web page uses getUserMedia() again? > > Will there be a new permission request, or does one need to use the setter with > WEBKIT_MEDIA_CAPTURE_STATE_ACTIVE before capture works again? > > I feel that this deserves clarification. The same question applies to the three > introduced properties, as I suppose internally WebKit handles them in the same way. Permission requests persistency support is left to the app, iirc. But I'll check.
(In reply to Philippe Normand from comment #4) > Which code? > The UIClient API test requires this because the getDisplayMedia() call > cannot work without user interaction, which we fake thanks to the > internals.withUserGesture() thing. I think it could be set by the API test itself, rather than the test script?
(In reply to Michael Catanzaro from comment #8) > (In reply to Philippe Normand from comment #4) > > Which code? > > The UIClient API test requires this because the getDisplayMedia() call > > cannot work without user interaction, which we fake thanks to the > > internals.withUserGesture() thing. > > I think it could be set by the API test itself, rather than the test script? I don't think we expose public API to set the injected bundle path within the WebContext?
Created attachment 433581 [details] Patch
(In reply to Philippe Normand from comment #9) > I don't think we expose public API to set the injected bundle path within > the WebContext? No, I mean you should do the setenv() in the API test itself (at the very top of main to avoid threadsafety issues) rather than relying on the test runner to do it. That way, it will work when the test is run directly without the test runner. Otherwise, if the test ever breaks in the future (it will), or if we need to run it manually for development, we will have trouble debugging it.
(In reply to Michael Catanzaro from comment #11) > (In reply to Philippe Normand from comment #9) > > I don't think we expose public API to set the injected bundle path within > > the WebContext? > > No, I mean you should do the setenv() in the API test itself (at the very > top of main to avoid threadsafety issues) rather than relying on the test > runner to do it. That way, it will work when the test is run directly > without the test runner. Otherwise, if the test ever breaks in the future > (it will), or if we need to run it manually for development, we will have > trouble debugging it. Ah OK, sorry I misunderstood :) Yeah this might work. I'll check
Created attachment 433587 [details] Patch
Comment on attachment 433587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433587&action=review > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1368 > + g_setenv("TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_FILENAME", "libTestWebKitAPIInternalInjectedBundle.so", TRUE); I would do this at the very top of main(), where there's no doubt you've done so before threads are initialized. I don't know for sure, but I would expects threads to exist by this point. If so, it's not safe. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1412 > + g_unsetenv("TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_FILENAME"); This will randomly crash, because some other thread will inevitably try calling getenv() at the same time. It's not needed anyway.
Comment on attachment 433587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433587&action=review >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1368 >> + g_setenv("TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_FILENAME", "libTestWebKitAPIInternalInjectedBundle.so", TRUE); > > I would do this at the very top of main(), where there's no doubt you've done so before threads are initialized. I don't know for sure, but I would expects threads to exist by this point. If so, it's not safe. Which main() are you talking about?
I assumed it was in the test file itself, TestUIClient.cpp, but I was wrong... it's in TestMain.cpp. Hm, is it safe to use libTestWebKitAPIInternalInjectedBundle.so for all tests? Since main calls startDBusServer() before it calls beforeAll(), we can be certain that beforeAll() is too late to safely modify the environment. :/
(In reply to Michael Catanzaro from comment #16) > I assumed it was in the test file itself, TestUIClient.cpp, but I was > wrong... it's in TestMain.cpp. > > Hm, is it safe to use libTestWebKitAPIInternalInjectedBundle.so for all > tests? > No. That's why in the original patch I was using it only for TestUIClient. > Since main calls startDBusServer() before it calls beforeAll(), we can be > certain that beforeAll() is too late to safely modify the environment. :/ Well, I did test here and it works. Also works on the EWS.
(In reply to Philippe Normand from comment #18) > Well, I did test here and it works. Also works on the EWS. Yeah, but if you get sufficiently unlucky it will crash. This has happened in practice https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/ so we shouldn't do it. Well drat, it's not so easy to avoid setting the environment variable from the test runner after all, then. :/
Comment on attachment 433581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433581&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:896 > + case PROP_CAMERA_CAPTURE_STATE: > + webkit_web_view_set_camera_capture_state(webView, static_cast<WebKitMediaCaptureState>(g_value_get_enum(value))); > + break; > + case PROP_MICROPHONE_CAPTURE_STATE: > + webkit_web_view_set_microphone_capture_state(webView, static_cast<WebKitMediaCaptureState>(g_value_get_enum(value))); > + break; > + case PROP_DISPLAY_CAPTURE_STATE: > + webkit_web_view_set_display_capture_state(webView, static_cast<WebKitMediaCaptureState>(g_value_get_enum(value))); Why don't we expose a single media capture state? > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4882 > + case WebCore::MediaProducer::MediaCaptureKind::AudioVideo: > + return; Can this happen? > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1073 > + " window.internals.withUserGesture(() => {" I think in other tests we emulate user gesture by sending a key press or something like that. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1100 > + webkit_settings_set_enable_media_stream(settings, enabled); > + webkit_settings_set_enable_mock_capture_devices(settings, FALSE); > + webkitSettingsSetMediaCaptureRequiresSecureConnection(settings, TRUE); So, we are only testing the permission request API, but not the new properties added to the web view?
Comment on attachment 433581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433581&action=review >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:896 >> + webkit_web_view_set_display_capture_state(webView, static_cast<WebKitMediaCaptureState>(g_value_get_enum(value))); > > Why don't we expose a single media capture state? A single prop for all 3 device types? >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4882 >> + return; > > Can this happen? No, this value is used only in the page API for stopping capture of both audio and video, AFAICS. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1100 >> + webkitSettingsSetMediaCaptureRequiresSecureConnection(settings, TRUE); > > So, we are only testing the permission request API, but not the new properties added to the web view? I can add some.
Comment on attachment 433581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433581&action=review >>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:896 >>> + webkit_web_view_set_display_capture_state(webView, static_cast<WebKitMediaCaptureState>(g_value_get_enum(value))); >> >> Why don't we expose a single media capture state? > > A single prop for all 3 device types? Nevermind, I realized that didn't make sense after reading the entire patch, but forgot to remove this question. >>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4882 >>> + return; >> >> Can this happen? > > No, this value is used only in the page API for stopping capture of both audio and video, AFAICS. Use ASSERT_NOT_REACHED() then to make it clearer. >>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:1100 >>> + webkitSettingsSetMediaCaptureRequiresSecureConnection(settings, TRUE); >> >> So, we are only testing the permission request API, but not the new properties added to the web view? > > I can add some. Yes please, and check we really need the injected bundle, I think we managed to emulate user gesture without it. In any case, we already have a web extension for tests, you can use WebExtensionTest.cpp to expose any js you need.
Created attachment 433752 [details] Patch
Comment on attachment 433752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433752&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:9431 > + if (m_isClosed) > + return; Seems like this is breaking mac API tests, I'll have to check it...
Comment on attachment 433752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433752&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:9431 >> + return; > > Seems like this is breaking mac API tests, I'll have to check it... I made this change to prevent a UIProcess crash triggered by GObject properties notifications during disposal of the WebView: (gdb) bt #0 g_type_check_instance_cast (type_instance=0xda6490, iface_type=0xd9d000 [GtkEntry/GtkWidget/GInitiallyUnowned]) at ../gobject/gtype.c:4115 #1 0x000000000041c1ce in webViewMediaCaptureStateChanged (webView=0xfbe130 [WebKitWebView], paramSpec=0xfa1e30 [GParamEnum], window=0xdb8490 [BrowserWindow]) at ../../Tools/MiniBrowser/gtk/BrowserWindow.c:657 #5 0x00007ff7aebd0ad3 in <emit signal notify:camera-capture-state on instance 0xfbe130 [WebKitWebView]> (instance=instance@entry=0xfbe130, signal_id=<optimized out>, detail=<optimized out>) at ../gobject/gsignal.c:3553 #2 0x00007ff7aebb6fcf in g_closure_invoke (closure=0xf8a6f0, return_value=return_value@entry=0x0, n_param_values=2, param_values=param_values@entry=0x7ffedc3d6fb0, invocation_hint=invocation_hint@entry=0x7ffedc3d6f30) at ../gobject/gclosure.c:810 #3 0x00007ff7aebc9e5b in signal_emit_unlocked_R (node=node@entry=0x9ce960, detail=detail@entry=1535, instance=instance@entry=0xfbe130, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d6fb0) at ../gobject/gsignal.c:3741 #4 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d7150) at ../gobject/gsignal.c:3497 #6 0x00007ff7aebbbbe4 in g_object_dispatch_properties_changed (object=0xfbe130 [WebKitWebView], n_pspecs=<optimized out>, pspecs=<optimized out>) at ../gobject/gobject.c:1206 #7 0x00007ff7aebbdd9a in g_object_notify_by_spec_internal (pspec=<optimized out>, object=0xfbe130 [WebKitWebView]) at ../gobject/gobject.c:1299 #8 g_object_notify_by_pspec (object=0xfbe130 [WebKitWebView], pspec=<optimized out>) at ../gobject/gobject.c:1409 #9 0x00007ff7b7f16c15 in webkitWebViewMediaCaptureStateDidChange(_WebKitWebView*, WTF::OptionSet<WebCore::MediaProducer::MediaState>) (webView=0xfbe130 [WebKitWebView], mediaStateFlags=...) at ../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:362 #10 0x00007ff7b7eec205 in UIClient::mediaCaptureStateDidChange(WTF::OptionSet<WebCore::MediaProducer::MediaState>) (this=0x7ff7ac5f6558, mediaStateFlags=...) at ../../Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:357 #11 0x00007ff7b7c40690 in WebKit::WebPageProxy::updateReportedMediaCaptureState() (this=0x7ff74c0fcc00) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:9457 #12 0x00007ff7b7c47db8 in WebKit::WebPageProxy::updatePlayingMediaDidChange(WTF::OptionSet<WebCore::MediaProducer::MediaState>, WebKit::WebPageProxy::CanDelayNotification) (this=0x7ff74c0fcc00, newState=..., canDelayNotification=WebKit::WebPageProxy::CanDelayNotification::No) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:9407 #13 0x00007ff7b7c479ef in WebKit::WebPageProxy::resetState(WebKit::WebPageProxy::ResetStateReason) (this=0x7ff74c0fcc00, resetStateReason=WebKit::WebPageProxy::ResetStateReason::PageInvalidated) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:7823 #14 0x00007ff7b7c42623 in WebKit::WebPageProxy::close() (this=0x7ff74c0fcc00) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:1168 #15 0x00007ff7b7f7669e in webkitWebViewBaseDispose(_GObject*) (gobject=0xfbe130 [WebKitWebView]) at ../../Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:734 #16 0x00007ff7b7f2324f in webkitWebViewDispose(_GObject*) (object=0xfbe130 [WebKitWebView]) at ../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1021 #17 0x00007ff7aebbd9f1 in g_object_run_dispose (object=0xfbe130 [WebKitWebView]) at ../gobject/gobject.c:1226 #18 0x00007ff7b5dd6850 in gtk_overlay_forall (overlay=0xf1a9d0 [GtkOverlay], include_internals=<optimized out>, callback=0x7ff7b5f09050 <gtk_widget_destroy>, callback_data=0x0) at ../gtk/gtkoverlay.c:628 #19 0x00007ff7b5cb414b in gtk_container_destroy (widget=0xf1a9d0 [GtkOverlay]) at ../gtk/gtkcontainer.c:1701 #23 0x00007ff7aebd0ad3 in <emit signal ??? on instance 0xf1a9d0 [GtkOverlay]> (instance=instance@entry=0xf1a9d0, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553 #20 0x00007ff7aebb6f0c in g_closure_invoke (closure=closure@entry=0xa18840, return_value=return_value@entry=0x0, n_param_values=1, param_values=param_values@entry=0x7ffedc3d7b50, invocation_hint=invocation_hint@entry=0x7ffedc3d7ad0) at ../gobject/gclosure.c:810 #21 0x00007ff7aebc9df5 in signal_emit_unlocked_R (node=node@entry=0xa2ba50, detail=detail@entry=0, instance=instance@entry=0xf1a9d0, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d7b50) at ../gobject/gsignal.c:3859 #22 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d7cd0) at ../gobject/gsignal.c:3497 #24 0x00007ff7b5f128af in gtk_widget_dispose (object=0xf1a9d0 [GtkOverlay]) at ../gtk/gtkwidget.c:12162 #25 0x00007ff7aebbd9f1 in g_object_run_dispose (object=0xf1a9d0 [GtkOverlay]) at ../gobject/gobject.c:1226 #26 0x00007ff7b5c5f5a8 in gtk_box_forall (container=<optimized out>, include_internals=<optimized out>, callback=0x7ff7b5f09050 <gtk_widget_destroy>, callback_data=0x0) at ../gtk/gtkbox.c:2675 #27 0x00007ff7b5cb414b in gtk_container_destroy (widget=0xf6a170 [BrowserTab]) at ../gtk/gtkcontainer.c:1701 #31 0x00007ff7aebd0ad3 in <emit signal ??? on instance 0xf6a170 [BrowserTab]> (instance=instance@entry=0xf6a170, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553 #28 0x00007ff7aebb6f0c in g_closure_invoke (closure=closure@entry=0xa18840, return_value=return_value@entry=0x0, n_param_values=1, param_values=param_values@entry=0x7ffedc3d7fd0, invocation_hint=invocation_hint@entry=0x7ffedc3d7f50) at ../gobject/gclosure.c:810 #29 0x00007ff7aebc9df5 in signal_emit_unlocked_R (node=node@entry=0xa2ba50, detail=detail@entry=0, instance=instance@entry=0xf6a170, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d7fd0) at ../gobject/gsignal.c:3859 #30 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d8150) at ../gobject/gsignal.c:3497 #32 0x00007ff7b5f128af in gtk_widget_dispose (object=0xf6a170 [BrowserTab]) at ../gtk/gtkwidget.c:12162 #33 0x00007ff7aebbd9f1 in g_object_run_dispose (object=0xf6a170 [BrowserTab]) at ../gobject/gobject.c:1226 #34 0x00007ff7b5dce456 in gtk_notebook_forall (container=<optimized out>, include_internals=0, callback=0x7ff7b5f09050 <gtk_widget_destroy>, callback_data=0x0) at ../gtk/gtknotebook.c:4607 #35 0x00007ff7b5cb414b in gtk_container_destroy (widget=0xe22500 [GtkNotebook]) at ../gtk/gtkcontainer.c:1701 #39 0x00007ff7aebd0ad3 in <emit signal ??? on instance 0xe22500 [GtkNotebook]> (instance=instance@entry=0xe22500, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553 --Type <RET> for more, q to quit, c to continue without paging-- #36 0x00007ff7aebb6f0c in g_closure_invoke (closure=closure@entry=0xa18840, return_value=return_value@entry=0x0, n_param_values=1, param_values=param_values@entry=0x7ffedc3d8460, invocation_hint=invocation_hint@entry=0x7ffedc3d83e0) at ../gobject/gclosure.c:810 #37 0x00007ff7aebc9df5 in signal_emit_unlocked_R (node=node@entry=0xa2ba50, detail=detail@entry=0, instance=instance@entry=0xe22500, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d8460) at ../gobject/gsignal.c:3859 #38 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d85e0) at ../gobject/gsignal.c:3497 #40 0x00007ff7b5f128af in gtk_widget_dispose (object=0xe22500 [GtkNotebook]) at ../gtk/gtkwidget.c:12162 #41 0x00007ff7aebbd9f1 in g_object_run_dispose (object=0xe22500 [GtkNotebook]) at ../gobject/gobject.c:1226 #42 0x00007ff7b5c5f5a8 in gtk_box_forall (container=<optimized out>, include_internals=<optimized out>, callback=0x7ff7b5f09050 <gtk_widget_destroy>, callback_data=0x0) at ../gtk/gtkbox.c:2675 #43 0x00007ff7b5cb414b in gtk_container_destroy (widget=0xe30c70 [GtkBox]) at ../gtk/gtkcontainer.c:1701 #47 0x00007ff7aebd0ad3 in <emit signal ??? on instance 0xe30c70 [GtkBox]> (instance=instance@entry=0xe30c70, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553 #44 0x00007ff7aebb6f0c in g_closure_invoke (closure=closure@entry=0xa18840, return_value=return_value@entry=0x0, n_param_values=1, param_values=param_values@entry=0x7ffedc3d88e0, invocation_hint=invocation_hint@entry=0x7ffedc3d8860) at ../gobject/gclosure.c:810 #45 0x00007ff7aebc9df5 in signal_emit_unlocked_R (node=node@entry=0xa2ba50, detail=detail@entry=0, instance=instance@entry=0xe30c70, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d88e0) at ../gobject/gsignal.c:3859 #46 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d8a60) at ../gobject/gsignal.c:3497 #48 0x00007ff7b5f128af in gtk_widget_dispose (object=0xe30c70 [GtkBox]) at ../gtk/gtkwidget.c:12162 #49 0x00007ff7aebbd9f1 in g_object_run_dispose (object=0xe30c70 [GtkBox]) at ../gobject/gobject.c:1226 #50 0x00007ff7b5f249b9 in gtk_window_forall (container=0xdb8490 [BrowserWindow], include_internals=0, callback=0x7ff7b5f09050 <gtk_widget_destroy>, callback_data=0x0) at ../gtk/gtkwindow.c:8596 #51 0x00007ff7b5cb414b in gtk_container_destroy (widget=0xdb8490 [BrowserWindow]) at ../gtk/gtkcontainer.c:1701 #55 0x00007ff7aebd0ad3 in <emit signal ??? on instance 0xdb8490 [BrowserWindow]> (instance=instance@entry=0xdb8490, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553 #52 0x00007ff7aebb6fcf in g_closure_invoke (closure=closure@entry=0xa18840, return_value=return_value@entry=0x0, n_param_values=1, param_values=param_values@entry=0x7ffedc3d8d70, invocation_hint=invocation_hint@entry=0x7ffedc3d8cf0) at ../gobject/gclosure.c:810 #53 0x00007ff7aebc9df5 in signal_emit_unlocked_R (node=node@entry=0xa2ba50, detail=detail@entry=0, instance=instance@entry=0xdb8490, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d8d70) at ../gobject/gsignal.c:3859 #54 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d8ef0) at ../gobject/gsignal.c:3497 #56 0x00007ff7b5f128af in gtk_widget_dispose (object=0xdb8490 [BrowserWindow]) at ../gtk/gtkwidget.c:12162 #57 0x00007ff7b5f27e20 in gtk_window_dispose (object=0xdb8490 [BrowserWindow]) at ../gtk/gtkwindow.c:3166 #58 0x00007ff7b5c50456 in gtk_application_window_dispose (object=0xdb8490 [BrowserWindow]) at ../gtk/gtkapplicationwindow.c:804 #59 0x0000000000418ee6 in browserWindowDispose (gObject=0xdb8490 [BrowserWindow]) at ../../Tools/MiniBrowser/gtk/BrowserWindow.c:993 #60 0x00007ff7aebbd9f1 in g_object_run_dispose (object=0xdb8490 [BrowserWindow]) at ../gobject/gobject.c:1226 #61 0x0000000000417c06 in webViewClose (webView=0xfbe130 [WebKitWebView], window=0xdb8490 [BrowserWindow]) at ../../Tools/MiniBrowser/gtk/BrowserWindow.c:404 #65 0x00007ff7aebd0ad3 in <emit signal ??? on instance 0xfbe130 [WebKitWebView]> (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>) at ../gobject/gsignal.c:3553 #62 0x00007ff7aebb6fcf in g_closure_invoke (closure=0xf67c60, return_value=return_value@entry=0x0, n_param_values=1, param_values=param_values@entry=0x7ffedc3d9270, invocation_hint=invocation_hint@entry=0x7ffedc3d91f0) at ../gobject/gclosure.c:810 #63 0x00007ff7aebc9e5b in signal_emit_unlocked_R (node=node@entry=0xf99060, detail=detail@entry=0, instance=instance@entry=0xfbe130, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffedc3d9270) at ../gobject/gsignal.c:3741 #64 0x00007ff7aebd0971 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d93f0) at ../gobject/gsignal.c:3497 #66 0x00007ff7b7f17a48 in webkitWebViewClosePage(_WebKitWebView*) (webView=0xfbe130 [WebKitWebView]) at ../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2506 #67 0x00007ff7b7f19984 in webkit_web_view_try_close(WebKitWebView*) (webView=0xfbe130 [WebKitWebView]) at ../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3060 #68 0x00000000004191c3 in browserWindowTryClose (action=0x0, parameter=0x0, userData=0xdb8490) at ../../Tools/MiniBrowser/gtk/BrowserWindow.c:389 #69 0x00000000004190ab in browserWindowDeleteEvent (widget=0xdb8490 [BrowserWindow], event=0xe978a0) at ../../Tools/MiniBrowser/gtk/BrowserWindow.c:1443 #70 0x00007ff7b5c20b18 in _gtk_marshal_BOOLEAN__BOXEDv (closure=0xa3ef20, return_value=0x7ffedc3d9740, instance=<optimized out>, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, param_types=0xa3d530) at gtk/gtkmarshalers.c:130 #71 0x00007ff7aebb7209 in _g_closure_invoke_va (closure=closure@entry=0xa3ef20, return_value=return_value@entry=0x7ffedc3d9740, instance=instance@entry=0xdb8490, args=args@entry=0x7ffedc3d9810, n_params=1, param_types=0xa3d530) at ../gobject/gclosure.c:873 --Type <RET> for more, q to quit, c to continue without paging-- #72 0x00007ff7aebcfc34 in g_signal_emit_valist (instance=0xdb8490, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffedc3d9810) at ../gobject/gsignal.c:3406 #73 0x00007ff7aebd0ad3 in g_signal_emit (instance=instance@entry=0xdb8490, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553 #74 0x00007ff7b5f07ab4 in gtk_widget_event_internal (widget=0xdb8490 [BrowserWindow], event=0xe978a0) at ../gtk/gtkwidget.c:7808 #75 0x00007ff7b5f0a892 in gtk_widget_event_internal (event=<optimized out>, widget=<optimized out>) at ../gtk/gtkwidget.c:7687 #76 gtk_widget_event (widget=<optimized out>, event=<optimized out>) at ../gtk/gtkwidget.c:7378 #77 0x0000000000000001 in () #78 0x0000000000e978a0 in () #79 0x0000000000eaf790 in () #80 0x0000000000000000 in ()
Comment on attachment 433752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433752&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1383 > + * true})`) this property will be set to @WEBKIT_MEDIA_CAPTURE_STATE_ACTIVE. @WEBKIT_MEDIA_CAPTURE_STATE_ACTIVE -> %WEBKIT_MEDIA_CAPTURE_STATE_ACTIVE > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1387 > + * @WEBKIT_MEDIA_CAPTURE_STATE_NONE or @WEBKIT_MEDIA_CAPTURE_STATE_MUTED. % here too > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1389 > + * If the capture state of the device is set to @WEBKIT_MEDIA_CAPTURE_STATE_NONE the web-page And here. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1408 > + * true})`) this property will be set to @WEBKIT_MEDIA_CAPTURE_STATE_ACTIVE. Ok, everywhere :-) > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4881 > + g_object_notify_by_pspec(G_OBJECT(webView), sObjProperties[PROP_MICROPHONE_CAPTURE_STATE]); This doesn't cause a mediaCaptureStateDidChange? > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4884 > + g_object_notify_by_pspec(G_OBJECT(webView), sObjProperties[PROP_CAMERA_CAPTURE_STATE]); Ditto > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4935 > + * Returns: The #WebKitMediaCaptureState of the camera device. If WebKitSettings:enable-mediastream #WebKitSettings:enable-mediastream > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4936 > + * is FALSE, this method will return %WEBKIT_MEDIA_CAPTURE_STATE_NONE. %FALSE > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4957 > + * If WebKitSettings:enable-mediastream is FALSE, this method will have no visible effect. Once the #WebKitSettings:enable-mediastream %FALSE > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4978 > + * Returns: The #WebKitMediaCaptureState of the microphone device. If WebKitSettings:enable-mediastream > + * is FALSE, this method will return %WEBKIT_MEDIA_CAPTURE_STATE_NONE. Same here and I guess the other ones. > Tools/MiniBrowser/gtk/BrowserWindow.c:1251 > + g_signal_connect(webView, "notify::camera-capture-state", G_CALLBACK(webViewMediaCaptureStateChanged), window); > + g_signal_connect(webView, "notify::microphone-capture-state", G_CALLBACK(webViewMediaCaptureStateChanged), window); > + g_signal_connect(webView, "notify::display-capture-state", G_CALLBACK(webViewMediaCaptureStateChanged), window); Try to use g_signal_connect_object or disconnect these signals in Dispose to see if that fixes the crash.
Comment on attachment 433752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433752&action=review >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4881 >> + g_object_notify_by_pspec(G_OBJECT(webView), sObjProperties[PROP_MICROPHONE_CAPTURE_STATE]); > > This doesn't cause a mediaCaptureStateDidChange? No. >> Tools/MiniBrowser/gtk/BrowserWindow.c:1251 >> + g_signal_connect(webView, "notify::display-capture-state", G_CALLBACK(webViewMediaCaptureStateChanged), window); > > Try to use g_signal_connect_object or disconnect these signals in Dispose to see if that fixes the crash. The former is not thread-safe though? The latter doesn't work, in Dispose I can't get the active webView, seems like it's gone.
Comment on attachment 433752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433752&action=review >>> Tools/MiniBrowser/gtk/BrowserWindow.c:1251 >>> + g_signal_connect(webView, "notify::display-capture-state", G_CALLBACK(webViewMediaCaptureStateChanged), window); >> >> Try to use g_signal_connect_object or disconnect these signals in Dispose to see if that fixes the crash. > > The former is not thread-safe though? The latter doesn't work, in Dispose I can't get the active webView, seems like it's gone. It is thread-safe nowadays, but even if not, it wouldn't matter here, the web view isn't used from multiple threads
Created attachment 433867 [details] Patch
Created attachment 433871 [details] Patch
(In reply to Carlos Garcia Campos from comment #28) > It is thread-safe nowadays The documentation of g_signal_connect_object() says it is not threadsafe, so I hope you don't rely on that assumption anywhere. > but even if not, it wouldn't matter here, the > web view isn't used from multiple threads GTK is itself not threadsafe anyway, so there's no way to make a GtkWidget threadsafe even if we wanted to do so for some reason.
(In reply to Michael Catanzaro from comment #31) > (In reply to Carlos Garcia Campos from comment #28) > > It is thread-safe nowadays > > The documentation of g_signal_connect_object() says it is not threadsafe, so > I hope you don't rely on that assumption anywhere. You are right, I got confused with the old bug that was documented and already fixed (https://bugzilla.gnome.org/show_bug.cgi?id=118536) > > but even if not, it wouldn't matter here, the > > web view isn't used from multiple threads > > GTK is itself not threadsafe anyway, so there's no way to make a GtkWidget > threadsafe even if we wanted to do so for some reason.
Comment on attachment 433871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433871&action=review > Source/WebKit/UIProcess/API/glib/WebKitUserMediaPermissionRequest.cpp:149 > + g_return_val_if_fail(request->priv->request, FALSE); I don't understand this. I know we are doing the same in the other methods, but I didn't know it. The request is set after object is constructed and we take a ref, so it can't be nullptr, but even in that case, it wouldn't be a programmer error, so g_return macros are not a good way to check that. I think this should just check the request passed to the function with g_return_val_if_fail(WEBKIT_IS_USER_MEDIA_PERMISSION_REQUEST(request), FALSE); And the same for the other functions. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4957 > + * If WebKitSettings:enable-mediastream is %FALSE, this method will have no visible effect. Once the #WebKitSettings:enable-mediastream > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4999 > + * If WebKitSettings:enable-mediastream is %FALSE, this method will have no visible effect. Once the #WebKitSettings:enable-mediastream > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:5041 > + * If WebKitSettings:enable-mediastream is %FALSE, this method will have no visible effect. Once the #WebKitSettings:enable-mediastream > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:67 > + void waitUntilCameraCaptureStateChangedTo(WebKitMediaCaptureState); > + void waitUntilDisplayCaptureStateChangedTo(WebKitMediaCaptureState); > + void waitUntilMicrophoneCaptureStateChangedTo(WebKitMediaCaptureState); Since this is only used by UIClient test, I think we could move it there instead.
Committed r280172 (239867@main): <https://commits.webkit.org/239867@main>