Bug 162147 – [MediaStream] Resolve constraints and enumerate devices in the UI process

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

On OSX and iOS, the WebCore sandbox profile does not allow it to access media capture devices. The UI process will grant sandbox extensions giving access to capture devices after the user has consented to the gUM request, but this is a problem in two cases:

- evaluating the initial constraints passed to gUM to see if the user should be prompted (eg. there is no reason to ask the user for permission if the constraints can't be satisfied at all).
- when MediaDevices.enumerateDevices is called before gUM.

To fix this, both of these tasks should be performed in the UI process.

[Eric Carlson]

Created attachment 289234 [details]
Patch for the bots.

[Eric Carlson]

Created attachment 289261 [details]
Another patch for the bots

[Eric Carlson]

Created attachment 289263 [details]
Updated patch.

[WebKit Commit Bot]

Attachment 289263 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/FeatureDefines.h:582:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 289263 [details]
Updated patch.

Attachment 289263 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2108115

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289276 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Eric Carlson]

Created attachment 290765 [details]
Proposed patch.

[Eric Carlson]

Created attachment 290768 [details]
Proposed patch.

[Eric Carlson]

Created attachment 290769 [details]
Proposed patch.

[Build Bot]

Comment on attachment 290769 [details]
Proposed patch.

Attachment 290769 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2228804

New failing tests:
fast/mediastream/MediaDevices-getUserMedia.html
fast/mediastream/getusermedia.html
fast/mediastream/mock-media-source.html

[Build Bot]

Created attachment 290779 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Eric Carlson]

Created attachment 291014 [details]
Updated patch.

[Eric Carlson]

Created attachment 291015 [details]
Updated patch.

[Build Bot]

Comment on attachment 291015 [details]
Updated patch.

Attachment 291015 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2244995

New failing tests:
fast/mediastream/MediaDevices-getUserMedia.html
fast/mediastream/getusermedia.html

[Build Bot]

Created attachment 291018 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Eric Carlson]

Created attachment 291549 [details]
Updated patch.

[Darin Adler]

Comment on attachment 291549 [details]
Updated patch.

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

We should really make a class derived from ContextDestructionObserver called DocumentDestructionObserver so we don’t need all these casts from scriptExecutionContext to Document. That base class would have a document() function.

Really not fond of the MedaiaConstraintsImpl terminology.

Did not finish reviewing. But here are my first batch of comments.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.cpp:49
> +Ref<MediaConstraintsImpl> MediaConstraintsImpl:: create(const MediaConstraintsData& data)

Stray space here before the word "create".

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:50
> +public:
> +    MediaConstraintsData(MediaTrackConstraintSetMap&& mandatory, Vector<MediaTrackConstraintSetMap>&& advanced, bool valid)
> +    {
> +        mandatoryConstraints = WTFMove(mandatory);
> +        advancedConstraints = WTFMove(advanced);
> +        isValid = valid;
> +    }
> +    explicit MediaConstraintsData() { }

None of this code should be needed. This should all work with { } syntax without defining any constructors. Also, "explicit" has no meaning on a constructor unless it has exactly one non-optional argument, so there’s no value in saying it on an empty constructor like this.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:68
> +    MediaConstraintsData data() const { return m_data; }

This needs to return const MediaConstraintsData&, otherwise we will copy the entire vector every time this is called!

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:74
> +    MediaConstraintsImpl(const MediaConstraintsData& data)
> +        : m_data(data)
>      {
>      }

This should take an rvalue reference and should use WTFMove.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:62
> +    if (m_scriptExecutionContext)
> +        return m_scriptExecutionContext->securityOrigin();
> +
> +    return nullptr;

WebKit coding style uses early return, so this code would be written in the reverse order.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:70
> +    if (Frame* frame = downcast<Document>(*scriptExecutionContext()).frame()) {

Not clear why this call site calls scriptExecutionContext() but the rest of the same function uses m_scriptExecutionContext. I suggest using the same thing consistently.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:89
> +    UserMediaController* controller = UserMediaController::from(document.page());

Would be better to use auto* here.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:125
> +        Ref<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, WTFMove(privateStream));

Should use auto.

> Source/WebCore/platform/mediastream/CaptureDevice.h:46
> +    explicit CaptureDevice() { }

The use of explicit here is incorrect. Please just omit it. Could also write:

    CaptureDevice() = default;

Most people on the project prefer that style, I think.

> Source/WebCore/platform/mediastream/MediaConstraints.h:90
> +    MediaConstraint() { }
> +    ~MediaConstraint() { }

Could write:

    MediaConstraint() = default;
    ~MediaConstraint() = default;

> Source/WebCore/platform/mediastream/MediaConstraints.h:268
> +    NumericConstraint()

Can we just write:

    NumericConstraint() = default;

> Source/WebCore/platform/mediastream/MediaConstraints.h:269
> +        : MediaConstraint()

Why is this line needed?

> Source/WebCore/platform/mediastream/MediaConstraints.h:313
> +    IntConstraint()
> +        : NumericConstraint()

Same comments as above.

> Source/WebCore/platform/mediastream/MediaConstraints.h:332
> +    DoubleConstraint()
> +        : NumericConstraint()

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:351
> +    BooleanConstraint()
> +        : MediaConstraint()

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:453
> +    StringConstraint()
> +        : MediaConstraint()

Ditto.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:57
> +    using ValidConstaintsHandler = std::function<void(const Vector<RefPtr<RealtimeMediaSource>>&& audioTracks, const Vector<RefPtr<RealtimeMediaSource>>&& videoTracks)>;

Spelling error: Should be constraints, not "constaints".

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:58
> +    using InvalidConstaintsHandler = std::function<void(const String& invalidConstraint)>;

Ditto.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2257
> +    for (size_t i = 0; i < advancedCount; ++i) {
> +        MediaTrackConstraintSetMap map;
> +        if (!decoder.decode(map))
> +            return false;
> +
> +        advancedConstraints.append(WTFMove(map));
> +    }

Should use reserveInitialCapacity and uncheckedAppend here.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2275
> +    encoder << static_cast<uint64_t>(device.kind());

Why use a 64-bit integer to encode two bits of data? Is that a common pattern in ArgumentEncoder?

[Eric Carlson]

Comment on attachment 291549 [details]
Updated patch.

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

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.cpp:49
>> +Ref<MediaConstraintsImpl> MediaConstraintsImpl:: create(const MediaConstraintsData& data)
> 
> Stray space here before the word "create".

Fixed.

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:50
>> +    explicit MediaConstraintsData() { }
> 
> None of this code should be needed. This should all work with { } syntax without defining any constructors. Also, "explicit" has no meaning on a constructor unless it has exactly one non-optional argument, so there’s no value in saying it on an empty constructor like this.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:68
>> +    MediaConstraintsData data() const { return m_data; }
> 
> This needs to return const MediaConstraintsData&, otherwise we will copy the entire vector every time this is called!

Fixed!

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:74
>>      }
> 
> This should take an rvalue reference and should use WTFMove.

We need a version that takes a const MediaConstraintsData and WTFMove doesn't like const qualified references, so I left this but added a version that takes rvalue references to the MediaConstraintsData fields.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:62
>> +    return nullptr;
> 
> WebKit coding style uses early return, so this code would be written in the reverse order.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:70
>> +    if (Frame* frame = downcast<Document>(*scriptExecutionContext()).frame()) {
> 
> Not clear why this call site calls scriptExecutionContext() but the rest of the same function uses m_scriptExecutionContext. I suggest using the same thing consistently.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:89
>> +    UserMediaController* controller = UserMediaController::from(document.page());
> 
> Would be better to use auto* here.

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:125
>> +        Ref<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, WTFMove(privateStream));
> 
> Should use auto.

Fixed.

>> Source/WebCore/platform/mediastream/CaptureDevice.h:46
>> +    explicit CaptureDevice() { }
> 
> The use of explicit here is incorrect. Please just omit it. Could also write:
> 
>     CaptureDevice() = default;
> 
> Most people on the project prefer that style, I think.

Changed to use "= default" throughout.

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:57
>> +    using ValidConstaintsHandler = std::function<void(const Vector<RefPtr<RealtimeMediaSource>>&& audioTracks, const Vector<RefPtr<RealtimeMediaSource>>&& videoTracks)>;
> 
> Spelling error: Should be constraints, not "constaints".

Fixed throughout.

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2257
>> +    }
> 
> Should use reserveInitialCapacity and uncheckedAppend here.

Fixed.

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2275
>> +    encoder << static_cast<uint64_t>(device.kind());
> 
> Why use a 64-bit integer to encode two bits of data? Is that a common pattern in ArgumentEncoder?

It is a common pattern, but I changed to use encodeEnum/decodeEnum for type safety (which uses a 64-bit integer).

[Eric Carlson]

Created attachment 291792 [details]
Updated patch.

[Eric Carlson]

Created attachment 291826 [details]
Updated patch.

[Darin Adler]

Comment on attachment 291826 [details]
Updated patch.

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

Looks good overall.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:71
> +    MediaConstraintsData m_data { };

I’m surprised these braces are needed. I would expect this to work the same without the { }.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.h:34
> +#include <wtf/RefCounted.h>
> +#include <wtf/text/WTFString.h>

Don’t need to include RefCounted.h if we are also including WTFString.h.

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.h:46
> +class MediaDevicesRequest : public RefCounted<MediaDevicesRequest>, public ContextDestructionObserver {

Might want to derive privately from ContextDestructionObserver instead of publicly.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
> +        deny(MediaAccessDenialReason::OtherFailure, "");

Should use emptyString() instead of "".

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:121
> +            deny(MediaAccessDenialReason::HardwareError, "");

Should use emptyString() instead of "".

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:127
> +            deny(MediaAccessDenialReason::HardwareError, "");

Should use emptyString() instead of "".

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:163
> +        m_promise.reject(OverconstrainedError::create(invalidConstraint, "Invalid constraint").get());

Should use ASCIILiteral.

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:56
> +class UserMediaRequest : public RefCounted<UserMediaRequest>, public ContextDestructionObserver {

Consider deriving privately from ContextDestructionObserver.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:87
> +    RefPtr<UserMediaRequest> request = m_idToUserMediaRequestMap.take(requestID);

I suggest using auto here instead of writing out RefPtr<UserMediaRequest>.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:137
> +    RefPtr<MediaDevicesEnumerationRequest> request = m_idToMediaDevicesEnumerationRequestMap.take(requestID);

Ditto.

[Eric Carlson]

Comment on attachment 291826 [details]
Updated patch.

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

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:71
>> +    MediaConstraintsData m_data { };
> 
> I’m surprised these braces are needed. I would expect this to work the same without the { }.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.h:34
>> +#include <wtf/text/WTFString.h>
> 
> Don’t need to include RefCounted.h if we are also including WTFString.h.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesRequest.h:46
>> +class MediaDevicesRequest : public RefCounted<MediaDevicesRequest>, public ContextDestructionObserver {
> 
> Might want to derive privately from ContextDestructionObserver instead of publicly.

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
>> +        deny(MediaAccessDenialReason::OtherFailure, "");
> 
> Should use emptyString() instead of "".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:121
>> +            deny(MediaAccessDenialReason::HardwareError, "");
> 
> Should use emptyString() instead of "".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:127
>> +            deny(MediaAccessDenialReason::HardwareError, "");
> 
> Should use emptyString() instead of "".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:163
>> +        m_promise.reject(OverconstrainedError::create(invalidConstraint, "Invalid constraint").get());
> 
> Should use ASCIILiteral.

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.h:56
>> +class UserMediaRequest : public RefCounted<UserMediaRequest>, public ContextDestructionObserver {
> 
> Consider deriving privately from ContextDestructionObserver.

Fixed.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:87
>> +    RefPtr<UserMediaRequest> request = m_idToUserMediaRequestMap.take(requestID);
> 
> I suggest using auto here instead of writing out RefPtr<UserMediaRequest>.

Fixed.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:137
>> +    RefPtr<MediaDevicesEnumerationRequest> request = m_idToMediaDevicesEnumerationRequestMap.take(requestID);
> 
> Ditto.

Fixed.

[Eric Carlson]

Created attachment 291832 [details]
Patch for landing.

[Eric Carlson]

Committed r207463: https://trac.webkit.org/r207463

[Csaba Osztrogonác]

(In reply to comment #24)
> Committed r207463: https://trac.webkit.org/r207463

It broke the GTK build, see build.webkit.org for details.

Additionally it would be great to have exactly same configuration on 
the GTK-EWS as buildbots on build.webkit.org to avoid similar breakages.

[Csaba Osztrogonác]

I think it is a similar compiler related problem as in bug163255

[Eric Carlson]

(In reply to comment #25)
> (In reply to comment #24)
> > Committed r207463: https://trac.webkit.org/r207463
> 
> It broke the GTK build, see build.webkit.org for details.
> 

Sorry about that!

> Additionally it would be great to have exactly same configuration on 
> the GTK-EWS as buildbots on build.webkit.org to avoid similar breakages.

Yes, having a different configurations makes it very difficult to avoid breakage for those of us not set up to build every port.

[Carlos Garcia Campos]

(In reply to comment #26)
> I think it is a similar compiler related problem as in bug163255

Yes, it's similar issue, I've just committed a build fix, because the build has been broken for long time. If the fix is not good, please file a new bug report to discuss how to properly fix it. Now I have more build issues to fix that were introduced after this one. I've already asked the GTK+ EWS bot maintainers to downgrade the GCC version there, sorry about this.

[Csaba Osztrogonác]

(In reply to comment #24)
> Committed r207463: https://trac.webkit.org/r207463

Additionally it broke the Apple Mac cmake build too:

/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:24:10: fatal error: 'WebCore/UserMediaRequest.h' file not found
#include <WebCore/UserMediaRequest.h>
         ^
1 error generated.

I have no idea how Apple Mac cmake port 
generates forwarding headers, cc-ing Alex.

[Philippe Normand]

This broke the GTK port. I can now see two different RealtimeMediaSourceCenter instances, one in the webprocess and one in the UI process, I suppose.

This is an issue because in one instance we check the constraints (validateRequestConstraints) and in the other we try to create sources without validating constraints, and that fails because the m_sourceMap hashmap of the the instance running in the webprocess is empty.

[Philippe Normand]

I think that (part of?) the solution would be to implement a CaptureDeviceManager for the OpenWebRTC backend.

[Philippe Normand]

Filed https://bugs.webkit.org/show_bug.cgi?id=164010 about this.