Bug 293786 – [WPE] Crash when running the mdn-bcd-collector test suite after 292932@main

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Carlos Alberto Lopez Perez]

Created attachment 475428 [details]
debug backtrace ("thread apply all bt full") generated on 295560@main

WPE is currently crashing when trying to run the test-suite at https://mdn-bcd-collector.gooborg.com/tests/

The crash seems to have been introduced by commit 292932@main

I'm attaching a full debug backtrace generated on 295560@main

As you see on the backtrace there are 370 threads ! that is not normal.

I tried to debug this and I found that It seems that the function DesktopPortalCamera::accessCamera() is calling itself recursively at line 166:

   129	bool DesktopPortalCamera::accessCamera()
   130	{
   131	    auto token = makeString("WebKit"_s, weakRandomNumber<uint32_t>());
   132	    GVariantBuilder options;
   133	    g_variant_builder_init(&options, G_VARIANT_TYPE_VARDICT);
   134	    g_variant_builder_add(&options, "{sv}", "handle_token", g_variant_new_string(token.utf8().data()));
   135	
   136	    auto connection = g_dbus_proxy_get_connection(m_proxy.get());
   137	    auto connectionString = StringView::fromLatin1(g_dbus_connection_get_unique_name(connection));
   138	    auto sender = makeStringByReplacingAll(connectionString.substring(1), '.', '_');
   139	    auto objectPath = makeString("/org/freedesktop/portal/desktop/request/"_s, sender, '/', token);
   140	    m_cameraAccessResults.add(objectPath, std::nullopt);
   141	
   142	    m_currentResponseCallback = [&](auto* variant) mutable {
   143	        if (!variant) {
   144	            m_cameraAccessResults.get(objectPath) = false;
   145	            return;
   146	        }
   147	
   148	        uint32_t response;
   149	        g_variant_get(variant, "(u@a{sv})", &response, nullptr);
   150	        // 0 response value means the user allowed device access.
   151	        m_cameraAccessResults.set(objectPath, !response);
   152	    };
   153	
   154	    auto signalId = g_dbus_connection_signal_subscribe(connection, "org.freedesktop.portal.Desktop", "org.freedesktop.portal.Request",
   155	        "Response", objectPath.utf8().data(), nullptr, G_DBUS_SIGNAL_FLAGS_NO_MATCH_RULE, reinterpret_cast<GDBusSignalCallback>(+[](GDBusConnection*, const char* /* senderName */, const char* /* objectPath */, const char* /* interfaceName */, const char* /* signalName */, GVariant* parameters, gpointer userData) {
   156	            auto& self = *reinterpret_cast<DesktopPortal*>(userData);
   157	            self.notifyResponse(parameters);
   158	        }),
   159	        this, nullptr);
   160	
   161	    g_dbus_proxy_call(m_proxy.get(), "AccessCamera", g_variant_new("(a{sv})", &options), G_DBUS_CALL_FLAGS_NONE, -1, nullptr, reinterpret_cast<GAsyncReadyCallback>(+[](GDBusProxy* proxy, GAsyncResult* result, gpointer) {
   162	        auto finalResult = adoptGRef(g_dbus_proxy_call_finish(proxy, result, nullptr));
   163	    }), nullptr);
   164	
   165	    while (m_currentResponseCallback)
   166	        g_main_context_iteration(nullptr, false);
   167	
   168	    g_dbus_connection_signal_unsubscribe(connection, signalId);
   169	    auto result = m_cameraAccessResults.take(objectPath);
   170	    return *result;
   171	}

[Carlos Alberto Lopez Perez]

note: to reproduce/test this I'm using wpe with the flatpak-sdk

[Philippe Normand]

It seems specific to those tests, which call multiple times in parallel navigator.mediaDevices.enumerateDevices() without awaiting the result properly...

[Philippe Normand]

Which browser? Cog? MiniBrowser?

Can you try this workaround?

diff --git a/Source/WebCore/platform/mediastream/gstreamer/DesktopPortal.cpp b/Source/WebCore/platform/mediastream/gstreamer/DesktopPortal.cpp
index 505b6df7ce00..6e2ec3c27536 100644
--- a/Source/WebCore/platform/mediastream/gstreamer/DesktopPortal.cpp
+++ b/Source/WebCore/platform/mediastream/gstreamer/DesktopPortal.cpp
@@ -128,6 +128,10 @@ bool DesktopPortalCamera::isCameraPresent()
 
 bool DesktopPortalCamera::accessCamera()
 {
+    if (m_currentResponseCallback) {
+        gst_printerrln("accessCamera call already in progress");
+        return false;
+    }
     auto token = makeString("WebKit"_s, weakRandomNumber<uint32_t>());
     GVariantBuilder options;
     g_variant_builder_init(&options, G_VARIANT_TYPE_VARDICT);

I think that we should make the AccessCamera dbus call blocking, but then I'm not sure how to handle Request Response thing...

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #3)
> Which browser? Cog? MiniBrowser?

I'm using MiniBrowser by default. But I also tried with Cog. The behavior is the same
 
> Can you try this workaround?
> 

Yes. Tried. It fixes the crash :)

r=me

Even if this is a workaround, it is totally worth landing this meanwhile a better fix is implemented.
Fixing a crash is always good :)

> I think that we should make the AccessCamera dbus call blocking, but then I'm not sure how to handle Request Response thing...

It is a complex problem. Maybe is worth checking how other browsers do this.
Here is an idea, that may be totally wrong or even not implementable at all, just thinking out loud: Perhaps instead of sending each `navigator.mediaDevices.enumerateDevices()` request to dbus, put those request in a queue and only allow one at a time to be sent to dbus. In other words: wait until the response of the last one that was sent to dbus comes back (maybe with some timeout seconds) until allowing the next one in the queue to be sent to dbus.

[Philippe Normand]

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

[EWS]

Committed 295675@main (d4ab7b6d6f91): <https://commits.webkit.org/295675@main>

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

[Philippe Normand]

.

[Philippe Normand]

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

[EWS]

Committed 296261@main (fc82cafe4cd6): <https://commits.webkit.org/296261@main>

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