Bug 222888

Summary:

[GStreamer] CaptureDevice monitor used from UIProcess

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

WebRTC

Assignee:

Philippe Normand <pnormand>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, cgarcia, eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

192697, 222889

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Philippe Normand

Reported 2021-03-07 09:18:08 PST

If the webapp starts listening on the devicechange event on the MediaDevice (hello jitsi), a RELEASE_ASSERT is hit, because the UserMediaPermissionRequestManager (WebProcess) sends a BeginMonitoringCaptureDevices message to the UIProcess... (gdb) bt #0 0x00007f1c1adf394e in WTFCrash() () at ../../Source/WTF/wtf/Assertions.cpp:295 #1 0x00007f1c277f1b6b in WTFCrashWithInfo(int, char const*, char const*, int) () at DerivedSources/ForwardingHeaders/wtf/Assertions.h:671 #2 0x00007f1c2945df7b in WebCore::ensureGStreamerInitialized() () at ../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:242 #3 0x00007f1c2d50d119 in WebCore::GStreamerCaptureDeviceManager::captureDevices() (this=0x7f1c34089080 <WebCore::GStreamerAudioCaptureDeviceManager::singleton()::manager>) at ../../Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp:90 #4 0x00007f1c2c22ea24 in WebCore::CaptureDeviceManager::getCaptureDevices(WTF::CompletionHandler<void (WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)>&&) (this=0x7f1c34089080 <WebCore::GStreamerAudioCaptureDeviceManager::singleton()::manager>, callback=...) at ../../Source/WebCore/platform/mediastream/CaptureDeviceManager.h:38 #5 0x00007f1c2c241461 in WebCore::RealtimeMediaSourceCenter::getMediaStreamDevices(WTF::CompletionHandler<void (WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)>&&) (this=0x7f1c3407e4f0 <WebCore::RealtimeMediaSourceCenter::singleton()::center>, completion=...) at ../../Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:153 #6 0x00007f1c2875593d in WebKit::UserMediaProcessManager::updateCaptureDevices(WebKit::UserMediaProcessManager::ShouldNotify) (this=0x7f1c34060400 <WebKit::UserMediaProcessManager::singleton()::manager>, shouldNotify=WebKit::UserMediaProcessManager::ShouldNotify::No) at ../../Source/WebKit/UIProcess/UserMediaProcessManager.cpp:222 #7 0x00007f1c287698a4 in WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27::operator()() const (this=0x7ffd8ca5a5b0) at ../../Source/WebKit/UIProcess/UserMediaProcessManager.cpp:261 #8 0x00007f1c2876986d in std::__invoke_impl<void, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::__invoke_other, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&) (__f=...) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/invoke.h:60 #9 0x00007f1c2876982d in std::__invoke<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&) (__fn=...) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/invoke.h:95 #10 0x00007f1c28769800 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)::{lambda()#1}::operator()() const (this=0x7ffd8ca5a588) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:717 #11 0x00007f1c287697d7 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)::{lambda()#2}::operator()() const (this=0x3) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:722 #12 0x00007f1c287697a9 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)::{lambda()#2}::__invoke() () at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:722 #13 0x00007f1c14d34aaf in __pthread_once_slow (once_control=0x7f1c34060450 <WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::onceFlag>, init_routine=0x7f1c136bd810 <std::__once_proxy()>) at pthread_once.c:116 #14 0x00007f1c2876977b in __gthread_once(int*, void (*)()) (__once=0x7f1c34060450 <WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::onceFlag>, __func=0x7f1c136bd810 <std::__once_proxy()>) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/x86_64-unknown-linux-gnu/bits/gthr-default.h:700 #15 0x00007f1c28755a68 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&) (__once=..., __f=...) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:729 #16 0x00007f1c287559e4 in WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices() (this=0x7f1c34060400 <WebKit::UserMediaProcessManager::singleton()::manager>) at ../../Source/WebKit/UIProcess/UserMediaProcessManager.cpp:260 #17 0x00007f1c287dbaba in WebKit::WebPageProxy::beginMonitoringCaptureDevices() (this=0x7f07bc0f9000) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:8228 #18 0x00007f1c27be7b84 in IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(), std::tuple<>>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(), std::tuple<>&&, std::integer_sequence<unsigned long>) (object=0x7f07bc0f9000, function=(void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const)) 0x7f1c287dba90 <WebKit::WebPageProxy::beginMonitoringCaptureDevices()>, args=...) at ../../Source/WebKit/Platform/IPC/HandleMessage.h:43 #19 0x00007f1c27be7af0 in IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(), std::tuple<>, std::integer_sequence<unsigned long> >(std::tuple<>&&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)()) (args=..., object=0x7f07bc0f9000, function=(void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const)) 0x7f1c287dba90 <WebKit::WebPageProxy::beginMonitoringCaptureDevices()>) at ../../Source/WebKit/Platform/IPC/HandleMessage.h:49 #20 0x00007f1c27bd9126 in IPC::handleMessage<Messages::WebPageProxy::BeginMonitoringCaptureDevices, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)()) (decoder=..., object=0x7f07bc0f9000, function=(void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const)) 0x7f1c287dba90 <WebKit::WebPageProxy::beginMonitoringCaptureDevices()>) at ../../Source/WebKit/Platform/IPC/HandleMessage.h:121 #21 0x00007f1c27bd04bc in WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc0f9000, connection=..., decoder=...) at DerivedSources/WebKit/WebPageProxyMessageReceiver.cpp:1493 #22 0x00007f1c2859f481 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc2fc4b8, connection=..., decoder=...) at ../../Source/WebKit/Platform/IPC/MessageReceiverMap.cpp:123 #23 0x00007f1c286ea52e in WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc2fc480, connection=..., decoder=...) at ../../Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:221 #24 0x00007f1c288b4eef in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc2fc480, connection=..., decoder=...) at ../../Source/WebKit/UIProcess/WebProcessProxy.cpp:819 #25 0x00007f1c2857976c in IPC::Connection::dispatchMessage(IPC::Decoder&) (this=0x7f1c104a8198, decoder=...) at ../../Source/WebKit/Platform/IPC/Connection.cpp:1010 #26 0x00007f1c28579c2c in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (this=0x7f1c104a8198, message=std::unique_ptr<IPC::Decoder> = {...}) at ../../Source/WebKit/Platform/IPC/Connection.cpp:1055 #27 0x00007f1c28578c11 in IPC::Connection::dispatchIncomingMessages() (this=0x7f1c104a8198) at ../../Source/WebKit/Platform/IPC/Connection.cpp:1159 #28 0x00007f1c2857e352 in IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)::$_8::operator()() (this=0x7f1c104a18a8) at ../../Source/WebKit/Platform/IPC/Connection.cpp:977 --Type <RET> for more, q to quit, c to continue without paging-- #29 0x00007f1c2857e2fe in WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)::$_8, void>::call() (this=0x7f1c104a18a0) at DerivedSources/ForwardingHeaders/wtf/Function.h:52 #30 0x00007f1c277f1332 in WTF::Function<void ()>::operator()() const (this=0x7ffd8ca5b5b8) at DerivedSources/ForwardingHeaders/wtf/Function.h:83 #31 0x00007f1c1ae2c395 in WTF::RunLoop::performWork() (this=0x7f1c104f9000) at ../../Source/WTF/wtf/RunLoop.cpp:128 #32 0x00007f1c1aec571c in WTF::RunLoop::RunLoop()::$_1::operator()(void*) const (this=0x7f1c104f9000, userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:80 #33 0x00007f1c1aec56f5 in WTF::RunLoop::RunLoop()::$_1::__invoke(void*) (userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:79 #34 0x00007f1c1aec56a9 in WTF::RunLoop::$_0::operator()(_GSource*, int (*)(void*), void*) const (this=0xb13450, source=0xb13450, callback=0x7f1c1aec56e0 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:53 #35 0x00007f1c1aec4725 in WTF::RunLoop::$_0::__invoke(_GSource*, int (*)(void*), void*) (source=0xb13450, callback=0x7f1c1aec56e0 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45 #36 0x00007f1c3418adbf in g_main_dispatch (context=0xb31460) at ../glib/gmain.c:3337 #37 g_main_context_dispatch (context=0xb31460) at ../glib/gmain.c:4055 #38 0x00007f1c3418b168 in g_main_context_iterate (context=context@entry=0xb31460, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4131 #39 0x00007f1c3418b233 in g_main_context_iteration (context=context@entry=0xb31460, may_block=may_block@entry=1) at ../glib/gmain.c:4196 #40 0x00007f1c13be962d in g_application_run (application=0xb97210 [GtkApplication], argc=-1935296492, argv=<optimized out>) at ../gio/gapplication.c:2560 #41 0x000000000041c23d in main (argc=1, argv=0x7ffd8ca5ba18) at ../../Tools/MiniBrowser/gtk/main.c:833

Attachments
Patch (5.47 KB, patch) 2021-03-07 10:56 PST, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (6.43 KB, patch) 2021-03-23 07:58 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (6.71 KB, patch) 2021-03-23 09:47 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (6.87 KB, patch) 2021-03-28 06:34 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (15.92 KB, patch) 2021-03-29 10:55 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (19.42 KB, patch) 2021-04-07 09:10 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (19.41 KB, patch) 2021-04-08 06:15 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (19.33 KB, patch) 2021-04-12 09:03 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (19.08 KB, patch) 2021-04-12 09:40 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (18.86 KB, patch) 2021-04-12 09:42 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Philippe Normand

Comment 1 2021-03-07 10:56:04 PST

Created attachment 422532 [details] Patch

Carlos Garcia Campos

Comment 2 2021-03-08 00:46:16 PST

Comment on attachment 422532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422532&action=review > Source/WebKit/UIProcess/UserMediaProcessManager.h:61 > + DevicesChangedCallback m_devicesChangedCallback; Maybe we should make this gstreamer only. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:40 > +#if USE(GSTREAMER) > +#include "UserMediaProcessManager.h" > +#endif It's a bit weird to use UI process API from the web process directly. Maybe we should move it to Shared (not necessarily as part of this patch, though).

Philippe Normand

Comment 3 2021-03-08 01:02:16 PST

Comment on attachment 422532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422532&action=review >> Source/WebKit/UIProcess/UserMediaProcessManager.h:61 >> + DevicesChangedCallback m_devicesChangedCallback; > > Maybe we should make this gstreamer only. Why? Then we'd also need to ifdef the beginMonitoringCaptureDevices changes? >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:40 >> +#endif > > It's a bit weird to use UI process API from the web process directly. Maybe we should move it to Shared (not necessarily as part of this patch, though). I agree

Philippe Normand

Comment 4 2021-03-15 02:32:13 PDT

So, does this patch needs an update?

youenn fablet

Comment 5 2021-03-23 07:18:53 PDT

Comment on attachment 422532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422532&action=review > Source/WebKit/UIProcess/UserMediaProcessManager.cpp:216 > + m_devicesChangedCallback(); What if m_devicesChangedCallback is not set? Might not happen in practice but still. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:163 > + UserMediaProcessManager::singleton().beginMonitoringCaptureDevices([weakThis = makeWeakPtr(*this)]() { Maybe you should create your own class here. UserMediaProcessManager is in UIProcess folder. And beginMonitoringCaptureDevices is not doing a lot of things anyway. Maybe you could use WebCore::RealtimeMediaSourceCenter::singleton() directly

Philippe Normand

Comment 6 2021-03-23 07:58:52 PDT

Created attachment 424017 [details] Patch

Alex Christensen

Comment 7 2021-03-23 09:10:16 PDT

Comment on attachment 424017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424017&action=review > Source/WebKit/ChangeLog:8 > + GStreamer ports aim to reduce usage of the GStreamer library in the UIProcess and thus Why are you doing this? Our view is that the web process is untrusted so we moved capturing things like this to the trusted UI process. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:163 > + WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, shouldNotify] (auto&& newDevices) mutable { This looks like it could use the this pointer after it is freed. if (!weakThis) return; > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:71 > + enum class ShouldNotify { Yes, No }; : bool

Philippe Normand

Comment 8 2021-03-23 09:47:15 PDT

Created attachment 424030 [details] Patch

Philippe Normand

Comment 9 2021-03-23 09:49:50 PDT

Comment on attachment 424017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424017&action=review >> Source/WebKit/ChangeLog:8 >> + GStreamer ports aim to reduce usage of the GStreamer library in the UIProcess and thus > > Why are you doing this? Our view is that the web process is untrusted so we moved capturing things like this to the trusted UI process. I expanded the explanation. We unfortunately can't move out of WebProcess yet, and we try to not initialize GStreamer from the UIProcess either, due to performance concerns on embedded platforms. >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:163 >> + WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, shouldNotify] (auto&& newDevices) mutable { > > This looks like it could use the this pointer after it is freed. > > if (!weakThis) > return; Right, weak ptr now captured in lambda. >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:71 >> + enum class ShouldNotify { Yes, No }; > > : bool I wonder why an enum was used in the UserMediaProcess now ;)

Philippe Normand

Comment 10 2021-03-28 06:34:11 PDT

Created attachment 424496 [details] Patch

youenn fablet

Comment 11 2021-03-28 12:16:49 PDT

Comment on attachment 424496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424496&action=review > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:173 > + if (m_captureDevices.size() == oldDevices.size()) { We could add to CaptureDevice.h a free function like: bool haveDevicesChanged(const Vector< CaptureDevice >& oldDevices, const Vector< CaptureDevice >& newDevices). And RealtimeMediaSourceCenter could do the debouceTimer before calling the observer. We would share more code with UserMediaProcessManager and rewrite this callback as: if (! haveDevicesChanged(m_captureDevices, newDevices)) return; m_captureDevices = WTFMove(newDevices); if (shouldNotify == ShouldNotify::Yes) captureDevicesChanged(); > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:210 > + WebCore::RealtimeMediaSourceCenter::singleton().setDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() { UserMediaPermissionRequestManager is per page, but RealtimeMediaSourceCenter is taking only one callback so far. This probably does not work for processes with more than one page.

Philippe Normand

Comment 12 2021-03-28 13:00:38 PDT

Comment on attachment 424496 [details] Patch I'll rework this then. Thanks Youenn :)

Philippe Normand

Comment 13 2021-03-29 10:55:55 PDT

Created attachment 424547 [details] Patch

Philippe Normand

Comment 14 2021-04-02 02:46:34 PDT

Does it look better Youenn? :)

youenn fablet

Comment 15 2021-04-02 02:58:26 PDT

Comment on attachment 424547 [details] Patch Looks good to me, a few suggestions below. View in context: https://bugs.webkit.org/attachment.cgi?id=424547&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:89 > + WEBCORE_EXPORT UserMediaClient::DeviceChangeObserverToken addDevicesChangedObserver(std::function<void()>&&); I do not remember why we use std::function. We probably want Function here. > Source/WebKit/UIProcess/UserMediaProcessManager.cpp:205 > + WebCore::RealtimeMediaSourceCenter::singleton().removeDevicesChangedObserver(m_deviceChangeObserverToken); I don't think we want this one, or we should add the observer back when enabled is set to true. Given it is a singleton, let's register once and for all. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:181 > + m_deviceChangeObserverToken = WebCore::RealtimeMediaSourceCenter::singleton().addDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() { We probably want to remove m_deviceChangeObserverToken in the destructor if it is set, otherwise it will grow over time. If we do so, we do not need to use weakThis here. Another possibility would be to use a WeakHashSet<Observer> which would remove the explicit use of weakThis and observer removal.

Philippe Normand

Comment 16 2021-04-07 08:45:07 PDT

Comment on attachment 424547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424547&action=review >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:181 >> + m_deviceChangeObserverToken = WebCore::RealtimeMediaSourceCenter::singleton().addDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() { > > We probably want to remove m_deviceChangeObserverToken in the destructor if it is set, otherwise it will grow over time. > If we do so, we do not need to use weakThis here. > > Another possibility would be to use a WeakHashSet<Observer> which would remove the explicit use of weakThis and observer removal. Trying to understand what you mean here, are you suggesting to wrap a new Observer class in the RealtimeSourceCenter and then make UserMediaPermissionRequestManager (in gst case) and UserMediaProcessManager inherit from it? I´ve tried that but that triggers build errors in UserMediaPermissionRequestManager: WTF/Headers/wtf/WeakPtr.h:249:19: error: member 'weakPtrFactory' found in multiple base classes of different types return object.weakPtrFactory().template createWeakPtr<T>(object); ^ ../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:158:87: note: in instantiation of function template specialization 'WTF::makeWeakPtr<WebKit::UserMediaPermissionRequestManager>' requested here WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([weakThis = makeWeakPtr(*this), this, shouldNotify](auto&& newDevices) mutable { ^ WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup const WeakPtrFactory<T, Counter>& weakPtrFactory() const { return m_weakPtrFactory; } ^ WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-26.cpp:2: ../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:159:14: error: use of undeclared identifier 'weakThis' if (!weakThis) ^ 2 errors generated.

Philippe Normand

Comment 17 2021-04-07 08:58:31 PDT

Comment on attachment 424547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424547&action=review >>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:181 >>> + m_deviceChangeObserverToken = WebCore::RealtimeMediaSourceCenter::singleton().addDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() { >> >> We probably want to remove m_deviceChangeObserverToken in the destructor if it is set, otherwise it will grow over time. >> If we do so, we do not need to use weakThis here. >> >> Another possibility would be to use a WeakHashSet<Observer> which would remove the explicit use of weakThis and observer removal. > > Trying to understand what you mean here, are you suggesting to wrap a new Observer class in the RealtimeSourceCenter and then make UserMediaPermissionRequestManager (in gst case) and UserMediaProcessManager inherit from it? > > I´ve tried that but that triggers build errors in UserMediaPermissionRequestManager: > > WTF/Headers/wtf/WeakPtr.h:249:19: error: member 'weakPtrFactory' found in multiple base classes of different types > return object.weakPtrFactory().template createWeakPtr<T>(object); > ^ > ../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:158:87: note: in instantiation of function template specialization 'WTF::makeWeakPtr<WebKit::UserMediaPermissionRequestManager>' requested here > WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([weakThis = makeWeakPtr(*this), this, shouldNotify](auto&& newDevices) mutable { > ^ > WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup > const WeakPtrFactory<T, Counter>& weakPtrFactory() const { return m_weakPtrFactory; } > ^ > WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup > In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-26.cpp:2: > ../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:159:14: error: use of undeclared identifier 'weakThis' > if (!weakThis) > ^ > 2 errors generated. Ok, trying to decouple the observer from the UserMediaPermissionRequestManager class might work.

Philippe Normand

Comment 18 2021-04-07 09:10:08 PDT

Created attachment 425406 [details] Patch

Philippe Normand

Comment 19 2021-04-08 06:15:05 PDT

Created attachment 425503 [details] Patch

youenn fablet

Comment 20 2021-04-12 07:47:16 PDT

Comment on attachment 425503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425503&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:71 > +} I would use = default for consistency with above destructor. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:64 > + virtual void devicesChanged() { } I would make it virtual pure since there is no point for an observe with devicesChanged being a no-op > Source/WebKit/UIProcess/UserMediaProcessManager.h:53 > + void devicesChanged() final; Can it be made private? > Source/WebKit/UIProcess/UserMediaProcessManager.h:63 > + WebCore::UserMediaClient::DeviceChangeObserverToken m_deviceChangeObserverToken; No longer needed? > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57 > + class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer { Can be made private probably. You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:59 > + DeviceObserver(UserMediaPermissionRequestManager& manager) explicit.

Philippe Normand

Comment 21 2021-04-12 08:58:40 PDT

Comment on attachment 425503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425503&action=review >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57 >> + class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer { > > Can be made private probably. > You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer. I tried that already, would incur duplicate inheritance on CanMakeWeakPtr.

youenn fablet

Comment 22 2021-04-12 09:01:48 PDT

(In reply to Philippe Normand from comment #21) > Comment on attachment 425503 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425503&action=review > > >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57 > >> + class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer { > > > > Can be made private probably. > > You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer. > > I tried that already, would incur duplicate inheritance on CanMakeWeakPtr. Right, you can probably do something like: using Class::weakPtrFactory; using WeakValueType = Class::WeakValueType; With Class be one of the two CanMakeWeakPtr.

Philippe Normand

Comment 23 2021-04-12 09:03:28 PDT

Created attachment 425753 [details] Patch

Philippe Normand

Comment 24 2021-04-12 09:28:55 PDT

Comment on attachment 425503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425503&action=review >>>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57 >>>> + class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer { >>> >>> Can be made private probably. >>> You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer. >> >> I tried that already, would incur duplicate inheritance on CanMakeWeakPtr. > > Right, you can probably do something like: > using Class::weakPtrFactory; > using WeakValueType = Class::WeakValueType; > > With Class be one of the two CanMakeWeakPtr. Ah, nice! I didn't know about this trick. I'll upload a new version of the patch. Thanks!

Philippe Normand

Comment 25 2021-04-12 09:40:02 PDT

Created attachment 425759 [details] Patch

Philippe Normand

Comment 26 2021-04-12 09:42:17 PDT

Created attachment 425760 [details] Patch

EWS

Comment 27 2021-04-13 01:38:21 PDT

Committed r275871 (236436@main): <https://commits.webkit.org/236436@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425760 [details].

Radar WebKit Bug Importer

Comment 28 2021-04-13 01:39:21 PDT

<rdar://problem/76581284>

Note You need to log in before you can comment on or make changes to this bug.