deviceId constraint doesn't work with getUserMedia
<rdar://problem/31899730>
Created attachment 309533 [details] Proposed patch.
Comment on attachment 309533 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=309533&action=review > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:319 > + MediaConstraintsData localAudioConstraintsData = MediaConstraintsData(audioConstraintsData); > + localAudioConstraintsData.deviceIDHashSalt = deviceIdentifierHashSalt; > + > + MediaConstraintsData localVideoConstraintsData = MediaConstraintsData(videoConstraintsData); > + localVideoConstraintsData.deviceIDHashSalt = deviceIdentifierHashSalt; > + > + auto audioConstraints = MediaConstraintsImpl::create(WTFMove(localAudioConstraintsData)); > + auto videoConstraints = MediaConstraintsImpl::create(WTFMove(localVideoConstraintsData)); Nit: this is really wordy. Could we maybe add some way of creating an Impl with just a MediaConstraintsData and a salt?
Comment on attachment 309533 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=309533&action=review >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:319 >> + auto videoConstraints = MediaConstraintsImpl::create(WTFMove(localVideoConstraintsData)); > > Nit: this is really wordy. Could we maybe add some way of creating an Impl with just a MediaConstraintsData and a salt? Good idea, fixed.
Created attachment 309540 [details] Patch for landing.
Looks good to me too. Main question is I am not sure why we need both a userMediaDocumentSecurityOrigin and a topLevelDocumentSecurityOrigin. Some nits below that could be addressed as follow-ups. View in context: https://bugs.webkit.org/attachment.cgi?id=309533&action=review > Source/WebCore/ChangeLog:31 > + (WebCore::RealtimeMediaSource::selectSettings): Special case DeviceId because it s/it// > Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.cpp:42 > + , m_topLevelDocumentSecurityOrigin(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin()) We have these two security origins. Why isn't it sufficient to have one? > Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:55 > + WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin() { return &m_topLevelDocumentSecurityOrigin.get(); } We should return references. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:68 > +void FrameAuthorizationState::ensureSecurityOriginsAreEqual(WebCore::SecurityOrigin* userMediaDocumentSecurityOrigin, WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin) Is it ok to pass null pointers for both of these? Probably should pass as const. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:87 > +FrameAuthorizationState& UserMediaPermissionRequestManagerProxy::stateForRequest(RequestType& request) Pass a const RequestType&? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:188 > + auto frameState = stateForRequest(*request); auto&? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:309 > + auto validateConstraintsHandler = [this, userMediaID, validHandler, invalidHandler, audioConstraintsData, videoConstraintsData](const String& deviceIdentifierHashSalt) { In theory, we should probably do something like validHandler = WTFMove(validHandler) and ditto for invalidHander. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:321 > + RealtimeMediaSourceCenter::singleton().validateRequestConstraints(validHandler, invalidHandler, audioConstraints, videoConstraints); Then we should move validHandler and validHandler. Might need mark validateConstraintsHandler as mutable. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:324 > + auto haveDeviceSaltHandler = [this, userMediaID, frameID, validateConstraintsHandler](uint64_t userMediaID, const String& deviceIdentifierHashSalt, bool originHasPersistentAccess) { Ditto for moving validateConstraintsHandler I think. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:357 > +void UserMediaPermissionRequestManagerProxy::getUserMediaPermissionInfo(uint64_t userMediaID, uint64_t frameID, UserMediaPermissionCheckProxy::CompletionHandler&& handler, const String&& userMediaDocumentOriginIdentifier, const String&& topLevelDocumentOriginIdentifier) Why const String&&? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:379 > +void UserMediaPermissionRequestManagerProxy::enumerateMediaDevicesForFrame(uint64_t userMediaID, uint64_t frameID, String userMediaDocumentOriginIdentifier, String topLevelDocumentOriginIdentifier) Should be String&& probably > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:49 > + void setDeviceIdentifierHashSalt(const String& hashSalt) { m_deviceIdentifierHashSalt = hashSalt; } Should be String&&. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:50 > + String deviceIdentifierHashSalt() const { return m_deviceIdentifierHashSalt; } Can we make it a const String&? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:94 > + void getUserMediaPermissionInfo(uint64_t userMediaID, uint64_t frameID, UserMediaPermissionCheckProxy::CompletionHandler&&, const String&&userMediaDocumentOriginIdentifier, const String&& topLevelDocumentOriginIdentifier); String&& and space
Comment on attachment 309533 [details] Proposed patch. Attachment 309533 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3707616 New failing tests: fast/mediastream/MediaDevices-enumerateDevices.html
Created attachment 309547 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 309540 [details] Patch for landing. Clearing flags on attachment: 309540 Committed r216545: <http://trac.webkit.org/changeset/216545>
Patch was rolled out in r216550.
Created attachment 309575 [details] Update patch.
Comment on attachment 309575 [details] Update patch. Clearing flags on attachment: 309575 Committed r216563: <http://trac.webkit.org/changeset/216563>
(In reply to WebKit Commit Bot from comment #12) > Comment on attachment 309575 [details] > Update patch. > > Clearing flags on attachment: 309575 > > Committed r216563: <http://trac.webkit.org/changeset/216563> This change caused 2 API test failures as well as an assertion failure: WebKit2.UserMediaBasic and MediaCaptureDisabledTest.EnableAndDisable https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/1389/steps/run-api-tests/logs/stdio fast/mediastream/apply-constraints-video.html https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r216581%20(1025)/results.html
Reverted r216563 for reason: Revision caused 2 api failures Committed r216596: <http://trac.webkit.org/changeset/216596>
Created attachment 309717 [details] Another patch.
The GTK breakage looks unrelated: gtk-wk2-ews/WebKit/Source/WebKit2/WebKit2Prefix.h -Wno-unused-parameter -MMD -MT Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp.o -MF Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp.o.d -o Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp.o -c ../../Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp g++: internal compiler error: Killed (program cc1plus) libbacktrace could not find executable to open Please submit a full bug report,
Comment on attachment 309717 [details] Another patch. Attachment 309717 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3718906 New failing tests: fast/mediastream/get-user-media-device-id.html fast/mediastream/MediaDevices-getUserMedia.html
Created attachment 309727 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 309744 [details] Updated patch.
Comment on attachment 309744 [details] Updated patch. Attachment 309744 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3719927 New failing tests: fast/mediastream/MediaDevices-getUserMedia.html
Created attachment 309766 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 309770 [details] Updated patch.
Some ideas for code improvements as a follow-up patch. There is one straightfroward issue that needs to be fixed though (moving a variable and reusing it afterwards). View in context: https://bugs.webkit.org/attachment.cgi?id=309744&action=review > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:49 > + MediaConstraintsData(const MediaConstraints& constraints) Shouldn't it be marked explicit? > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:56 > + MediaConstraintsData(const MediaConstraintsData& constraints, const String& hashSalt) hashSalt should be a String&&, and we could do something like String&& = { } to remove the above constructor. I am not sure where it is used by the way. Maybe we should just beef up MediaConstraintsData(MediaTrackConstraintSetMap&&, Vector<MediaTrackConstraintSetMap>&&, bool) to take a String&& instead? > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:85 > + String deviceIDHashSalt() const final { return m_data.deviceIDHashSalt; } Should be a const String& I think. > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:86 > + void setDeviceIDHashSalt(const String& salt) final { m_data.deviceIDHashSalt = salt; } Should be taking a String&& > Source/WebCore/Modules/mediastream/MediaDevices.h:85 > + std::optional<RealtimeMediaSourceCenter::DevicesChangedObserverToken> m_deviceChangedToken; I do not understand that change. Having 0 as a special 'unset' value seems fine with me. What is the issue with the previous code? > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:159 > m_allowedVideoDeviceUID = videoDeviceUID; audioDeviceUID and videoDeviceUID should be String&&. > Source/WebCore/platform/mediastream/MediaConstraints.h:814 > + virtual String deviceIDHashSalt() const = 0; Ditto on changes the signature here. > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:502 > +bool RealtimeMediaSource::selectSettings(const MediaConstraints& constraints, FlattenedConstraint& candidates, String& failedConstraint, SelectType type) I guess the alternative approach would be for selectSettings to remove deviceId constraints when not being useful. But this conflicts with const& signature. Seems fine. > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:552 > + double constraintDistance = downcast<StringConstraint>(constraint).fitnessDistance(hashedID); If we have several downcast<> like this, maybe we should encapsulate this in MediaConstraint, for instance constraint.asStringConstraint(). Or a templated fitnessDistance, but maybe that would cause more harm. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:183 > + static NeverDestroyed<HashMap<unsigned, std::function<void()>>> map; I am not sure this is healthy to add more static variables. We should at least move this code closer to RealtimeMediaSourceCenter::singleton() so that if want to attach RealtimeMediaSourceCenter to the page, or to the process later on, we do not forget about this one. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:202 > + auto callbacks = observerMap(); Do we want to make a separate copy of the map? Should it be auto&. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:203 > for (auto it : callbacks) Probably want auto& here? and use iterator instead of it. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:253 > + audioSourceIds.uncheckedAppend(info.id); Nit: since we do not reuse audioDeviceInfo, we could probably take a for(auto& info..) and do audioSourceIds.uncheckedAppend(WTFMove(info.id)) > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:261 > + videoSourceIds.uncheckedAppend(info.id); Ditto here. > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:169 > + invalidConstraint = emptyString(); Are we adding that for safety reasons? Since sourceUIDs is not empty, the value should be ignored, right? The new pattern would be to add a VectorOfStringOrErrorMessage. Btw, if we fail to create several sources on several devices, we will only pick the last invalid constraint. Is it ok? > Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.cpp:42 > + , m_topLevelDocumentSecurityOrigin(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin()) Should probably directly take Ref<SecurityOrigin>&& instead of strings here. Future refactoring probably. > Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:55 > + WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin() { return &m_topLevelDocumentSecurityOrigin.get(); } Should return references. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:44 > + , m_topLevelDocumentSecurityOrigin(topLevelDocumentSecurityOrigin) These might well be references and not pointers. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:68 > +void FrameAuthorizationState::ensureSecurityOriginsAreEqual(WebCore::SecurityOrigin* userMediaDocumentSecurityOrigin, WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin) Probably here as well. Should take const& as well. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:87 > +FrameAuthorizationState& UserMediaPermissionRequestManagerProxy::stateForRequest(RequestType& request) Can probably take a const RequestType& > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:275 > + auto userMediaOrigin = API::SecurityOrigin::create(SecurityOriginData::fromDatabaseIdentifier(userMediaDocumentOriginIdentifier).value_or(SecurityOriginData()).securityOrigin()); RequestUserMediaPermissionForFrame should take WebCore::SecurityOriginData as input parameter not strings. This would remove the need for conversions. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:315 > + RealtimeMediaSourceCenter::singleton().validateRequestConstraints(WTFMove(validHandler), WTFMove(invalidHandler), audioConstraints, videoConstraints); I don't know if this is always the case, but here we could move the constraints so that the underlying implementation would be fine modifying these constraints. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:325 > + return; Should we do this check before taking the request? If so, can request be null in some cases? Probably but asking the question anyway. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:350 > + UserMediaPermissionCheckProxy::CompletionHandler failureHandler = handler; Since you move handler in the request, failureHandler will be empty here. Probably not good. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:354 > + failureHandler(userMediaID, String(), false); Can you do something like request->reportFailure(...) to handle the case above? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:368 > + return; Same question/comment as above. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:374 > + auto deviceInfo = RealtimeMediaSourceCenter::singleton().getMediaStreamDevices(); We are creating a Vector for each call to getMediaStreamDevices. DidCompleteMediaDeviceEnumeration expecting a const&, this Vector allocation seems unnecessary. Might be worth changing the signature in the future. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:47 > + void ensureSecurityOriginsAreEqual(WebCore::SecurityOrigin* userMediaDocumentSecurityOrigin, WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin); Probably want references here and Ref<SecurityOrigin> below. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:49 > + void setDeviceIdentifierHashSalt(const String& hashSalt) { m_deviceIdentifierHashSalt = hashSalt; } Should be String&&
Created attachment 309942 [details] Patch for landing.
Created attachment 309954 [details] Patch for landing.
Created attachment 310056 [details] Patch for landing.
Comment on attachment 310056 [details] Patch for landing. Clearing flags on attachment: 310056 Committed r216836: <http://trac.webkit.org/changeset/216836>