Bug 222888 – [GStreamer] CaptureDevice monitor used from UIProcess

• Component: WebRTC
• Status: RESOLVED FIXED

[Philippe Normand]

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

[Philippe Normand]

Created attachment 422532 [details]
Patch

[Carlos Garcia Campos]

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 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]

So, does this patch needs an update?

[youenn fablet]

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]

Created attachment 424017 [details]
Patch

[Alex Christensen]

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]

Created attachment 424030 [details]
Patch

[Philippe Normand]

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]

Created attachment 424496 [details]
Patch

[youenn fablet]

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 on attachment 424496 [details]
Patch

I'll rework this then. Thanks Youenn :)

[Philippe Normand]

Created attachment 424547 [details]
Patch

[Philippe Normand]

Does it look better Youenn? :)

[youenn fablet]

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 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 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]

Created attachment 425406 [details]
Patch

[Philippe Normand]

Created attachment 425503 [details]
Patch

[youenn fablet]

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 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]

(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]

Created attachment 425753 [details]
Patch

[Philippe Normand]

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]

Created attachment 425759 [details]
Patch

[Philippe Normand]

Created attachment 425760 [details]
Patch

[EWS]

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]

<rdar://problem/76581284>