Bug 173527 – Merge MediaDevicesRequest and MediaDevicesEnumerationRequest to tighten up code and object lifetime

• Status: ASSIGNED
• Component: Media

[Darin Adler]

Merge MediaDevicesRequest and MediaDevicesEnumerationRequest to tighten up code and object lifetime

[Darin Adler]

Created attachment 313248 [details]
Patch

[Darin Adler]

Instead of isActive I could store the DOMPromise in a std::optional and set it to nullopt after we are done with it or when we cancel. I guess that would be a bit more elegant.

[Darin Adler]

Created attachment 313249 [details]
Patch

[Sam Weinig]

Comment on attachment 313249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313249&action=review

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:63
> +    auto* controller = UserMediaController::from(document.page());
> +    if (!controller) {
> +        // FIXME: Should we resolve or reject the promise here instead of leaving the website waiting?
> +        return;
> +    }

The fact that UserMediaController::from can fail seems so opaque. What it is really checking is if there is page.  It makes me think we should make UserMediaController::from (and therefore Supplement::from) take a Page& and return a Foo& make the caller check that page is not null instead of the foo.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:113
> +#if !PLATFORM(COCOA)
> +    UNUSED_PARAM(devices);
> +#else

This doesn't feel like a COCOA not COCOA divide, but rather a ENABLE(ATYPICAL_DEVICE_FILTERING) thing.  Actually, it would be better if that were a setting, all things considered, then it could be tested with different values.  Thoughts for the future.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:139
> +    Vector<RefPtr<MediaDeviceInfo>> devices;

I would say this should be a Vector<> of Ref<MediaDeviceInfo>, but I would only be impugning myself.  I really need to make Ref<> work with the bindings!

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:150
> +        if (id.isEmpty())
> +            continue;

Seems like id could be computed first and this check could be done directly after.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152
> +        devices.append(MediaDeviceInfo::create(&document, label, id, groupId, deviceType));

Quite silly that MediaDeviceInfo::create takes the document by pointer.

[Darin Adler]

Comment on attachment 313249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313249&action=review

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152
>> +        devices.append(MediaDeviceInfo::create(&document, label, id, groupId, deviceType));
> 
> Quite silly that MediaDeviceInfo::create takes the document by pointer.

Turns out it doesn’t even need a document pointer. Can this be a structure instead of an object? I don’t remember when we have that kind of choice in IDL.

[Darin Adler]

Committed r218530: <http://trac.webkit.org/changeset/218530>

[Darin Adler]

Comment on attachment 313249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313249&action=review

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:63
>> +    }
> 
> The fact that UserMediaController::from can fail seems so opaque. What it is really checking is if there is page.  It makes me think we should make UserMediaController::from (and therefore Supplement::from) take a Page& and return a Foo& make the caller check that page is not null instead of the foo.

Done.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:113
>> +#else
> 
> This doesn't feel like a COCOA not COCOA divide, but rather a ENABLE(ATYPICAL_DEVICE_FILTERING) thing.  Actually, it would be better if that were a setting, all things considered, then it could be tested with different values.  Thoughts for the future.

Used a #define in the file for clarity.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:150
>> +            continue;
> 
> Seems like id could be computed first and this check could be done directly after.

Done.

>>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:152
>>> +        devices.append(MediaDeviceInfo::create(&document, label, id, groupId, deviceType));
>> 
>> Quite silly that MediaDeviceInfo::create takes the document by pointer.
> 
> Turns out it doesn’t even need a document pointer. Can this be a structure instead of an object? I don’t remember when we have that kind of choice in IDL.

Removed the document argument.

[Matt Lewis]

After this patch several mediastream tests have started to crash on Debug builds with Assertion failures:

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r218588%20(1650)/results.html

stdio:

ASSERTION FAILED: !m_promise
/Volumes/Data/slave/sierra-debug/build/Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp(137) : void WebCore::MediaDevicesEnumerationRequest::setDeviceInfo(const Vector<WebCore::CaptureDevice> &, const WTF::String &, bool)
1   0x114c442fd WTFCrash
2   0x10a87ca40 WebCore::MediaDevicesEnumerationRequest::setDeviceInfo(WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool)
3   0x1059aaf57 WebKit::UserMediaPermissionRequestManager::didCompleteMediaDeviceEnumeration(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool)
4   0x105c2c426 WebKit::WebPage::didCompleteMediaDeviceEnumeration(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool)
5   0x105cacd9d void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool), std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>, 0ul, 1ul, 2ul, 3ul>(WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool), std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>)
6   0x105cabb28 void IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool), std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(std::__1::tuple<unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul>, WTF::String, bool>&&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool))
7   0x105c9ea80 void IPC::handleMessage<Messages::WebPage::DidCompleteMediaDeviceEnumeration, WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool)>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long, WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String&&, bool))
8   0x105c93b10 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&)
9   0x105c2da7e WebKit::WebPage::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
10  0x105c2dac4 non-virtual thunk to WebKit::WebPage::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
11  0x1055b6dc8 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
12  0x105e4eb4d WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
13  0x10548e463 IPC::Connection::dispatchMessage(IPC::Decoder&)
14  0x105483b98 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
15  0x10548ea60 IPC::Connection::dispatchOneMessage()
16  0x1054a6d9d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
17  0x1054a6cf9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
18  0x114c72fae WTF::Function<void ()>::operator()() const
19  0x114c92b53 WTF::RunLoop::performWork()
20  0x114c93324 WTF::RunLoop::performWork(void*)
21  0x7fffa88703e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
22  0x7fffa885165c __CFRunLoopDoSources0
23  0x7fffa8850b46 __CFRunLoopRun
24  0x7fffa8850544 CFRunLoopRunSpecific
25  0x7fffa7db0ebc RunCurrentEventLoopInMode
26  0x7fffa7db0cf1 ReceiveNextEventCommon
27  0x7fffa7db0b26 _BlockUntilNextEventMatchingListInModeWithFilter
28  0x7fffa6349a54 _DPSNextEvent
29  0x7fffa6ac57ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
30  0x7fffa633e3db -[NSApplication run]
31  0x7fffa6308e0e NSApplicationMain
LEAK: 14 WebPageProxy

[Darin Adler]

Feel free to roll out. Likely we just need to add an if statement to fix it.

[Matt Lewis]

Reverted r218530 for reason:

This revision caused multiple media stream test crashes on Debug builds.

Committed r218601: <http://trac.webkit.org/changeset/218601>

[Darin Adler]

Maybe I would have seen this on EWS if I had posted the final version of the patch.