Bug 152087 – [MediaStream] Expose media capture devices persistent permissions to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Make it possible for WebCore to check to see if persistent permission to access media capture devices has been granted to the page's origin.

[Eric Carlson]

Created attachment 267052 [details]
Preliminary patch for the bots.

Patch without useful ChangeLog entries for the bots to mull over.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 44 files


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

[Eric Carlson]

Created attachment 267056 [details]
Patch.

[Simon Fraser (smfr)]

Comment on attachment 267056 [details]
Patch.

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

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:86
> +    m_hasUserMediaPermission = access;

Is storing in m_hasUserMediaPermission thread-safe given the callOnMainThread() below?

> Source/WebCore/Modules/mediastream/UserMediaController.h:67
> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request)

I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in.

> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:43
> +RefPtr<UserMediaPermissionCheck> UserMediaPermissionCheck::create(Document& document, UserMediaPermissionCheckClient& client)

This should return a Ref<>

> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:81
> +    m_protector = this;

Weird. Why doesn't someone own this explicitly?

> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.h:69
> +    RefPtr<UserMediaPermissionCheck> m_protector;

Odd to have this here.

[Eric Carlson]

Comment on attachment 267056 [details]
Patch.

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

>> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:86
>> +    m_hasUserMediaPermission = access;
> 
> Is storing in m_hasUserMediaPermission thread-safe given the callOnMainThread() below?

Yes, this is the callback from the permission checker so it is called exactly once.

>> Source/WebCore/Modules/mediastream/UserMediaController.h:67
>> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request)
> 
> I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in.

The check function doesn't take ownership, it passes it along to the client (the WK1 or WK2 WebUserMediaClient ) which stores it in a map and calls the UI delegate (WK1) or page proxy (WK2).

>> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:43
>> +RefPtr<UserMediaPermissionCheck> UserMediaPermissionCheck::create(Document& document, UserMediaPermissionCheckClient& client)
> 
> This should return a Ref<>

Oops, will fix.

>> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:81
>> +    m_protector = this;
> 
> Weird. Why doesn't someone own this explicitly?

Actually, the checker is owned by a MediaDevicesRequest so this isn't necessary. Will remove.

[Darin Adler]

Comment on attachment 267056 [details]
Patch.

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

>>> Source/WebCore/Modules/mediastream/UserMediaController.h:67
>>> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request)
>> 
>> I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in.
> 
> The check function doesn't take ownership, it passes it along to the client (the WK1 or WK2 WebUserMediaClient ) which stores it in a map and calls the UI delegate (WK1) or page proxy (WK2).

So the argument type should just be UserMediaPermissionCheck& instead of Ref&&. Same for requestUserMediaAccess.

[Eric Carlson]

(In reply to comment #6)
> Comment on attachment 267056 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267056&action=review
> 
> >>> Source/WebCore/Modules/mediastream/UserMediaController.h:67
> >>> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request)
> >> 
> >> I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in.
> > 
> > The check function doesn't take ownership, it passes it along to the client (the WK1 or WK2 WebUserMediaClient ) which stores it in a map and calls the UI delegate (WK1) or page proxy (WK2).
> 
> So the argument type should just be UserMediaPermissionCheck& instead of
> Ref&&. Same for requestUserMediaAccess.

OK, fixed.

[Eric Carlson]

Created attachment 267102 [details]
Updated patch.

[Chris Dumez]

Comment on attachment 267102 [details]
Updated patch.

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

r=me with comments.

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:84
> +void MediaDevicesRequest::didCompleteCheck(bool access)

nit: I think we usually name booleans so that they start with something like is/can/should for clarity.

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:91
> +    RefPtr<MediaDevicesRequest> protectedThis(this);

Hmm, this looks unsafe. MediaDevicesRequest is not ThreadSafeRefCounted.

> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:75
> +    Document* document = downcast<Document>(scriptExecutionContext());

nit: I would prefer:
auto& document = downcast<Document>(*scriptExecutionContext());

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

Could be final ?

> Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:41
> +    static PassRefPtr<UserMediaPermissionCheckProxy> create(UserMediaPermissionRequestManagerProxy& manager, uint64_t userMediaID)

We don't use PassRefPtr anymore. This should return a Ref<>

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:63
> +PassRefPtr<UserMediaPermissionCheckProxy> UserMediaPermissionRequestManagerProxy::createUserMediaPermissionCheck(uint64_t userMediaID)

Should return a Ref<>.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:65
> +    RefPtr<UserMediaPermissionCheckProxy> request = UserMediaPermissionCheckProxy::create(*this, userMediaID);

Ref<>

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:66
> +    m_pendingDeviceRequests.add(userMediaID, request.get());

request.ptr()

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:40
> +    PassRefPtr<UserMediaPermissionCheckProxy> createUserMediaPermissionCheck(uint64_t userMediaID);

Ref<>

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:90
> +void UserMediaPermissionRequestManager::startUserMediaPermissionCheck(WebCore::UserMediaPermissionCheck& request)

You don't need the WebCore::

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:92
> +    Document* document = downcast<Document>(request.scriptExecutionContext());

auto*

> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:84
> +    DEPRECATED_DEFINE_STATIC_LOCAL(UserMediaCheckMap, requests, ());

static NeverDestroyed<> ?

> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:119
> +void WebUserMediaClient::requestUserMediaAccess(UserMediaRequest& prpRequest)

We should get rid of the prp prefix

> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:123
> +    UserMediaRequest* request = &prpRequest;

We don't really need this local variable anymore I believe.

> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:153
> +void WebUserMediaClient::checkUserMediaPermission(UserMediaPermissionCheck& prpRequest)

We should drop the prp prefix

> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:157
> +    UserMediaPermissionCheck* request = &prpRequest;

Not needed?

> Tools/WebKitTestRunner/TestController.h:289
> +    typedef Vector<std::pair<WKRetainPtr<WKSecurityOriginRef>, WKRetainPtr<WKUserMediaPermissionRequestRef>>> PermissionRequestMap;

Calling this "Map" is a tad confusing. Maybe List or Vector?

[Chris Dumez]

Comment on attachment 267102 [details]
Updated patch.

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

> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.h:34
> +#include <wtf/PassRefPtr.h>

Useless include.

> Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:31
> +#include <wtf/PassRefPtr.h>

Useless include.

[Eric Carlson]

Created attachment 267116 [details]
Patch for landing.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:25:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 45 files


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

[Eric Carlson]

Created attachment 267121 [details]
Another updated patch for landing.

[Eric Carlson]

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