Bug 164760 – [MediaStream] Don't request user permission for a device if it has already been granted in the current browsing context

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Do not prompt the user for permission to use a capture device if permission has already granted access for the current browsing context.

[Radar WebKit Bug Importer]

<rdar://problem/29261266>

[Eric Carlson]

Created attachment 294834 [details]
Proposed patch.

[Build Bot]

Comment on attachment 294834 [details]
Proposed patch.

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

New failing tests:
fast/mediastream/MediaStream-MediaElement-srcObject.html

[Build Bot]

Created attachment 294837 [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 294838 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 294838 [details]
Proposed patch.

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

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:51
> +    if (deviceUID.isEmpty())

Can this case happen?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:72
> +    } while (0);

Is it left-over code, or were you planning to add something like ASSERT(!securityOriginsAreEqual(request))?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:81
> +    if (!m_userMediaDocumentSecurityOrigin || !m_userMediaDocumentSecurityOrigin->equal(request.userMediaDocumentSecurityOrigin()))

I am unclear whether SecurityOrigin::equal is the right routine to call, in particular the check for m_domainWasSetInDOM.
It seems the best choice AFAICS though.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:92
> +            state->reset(request);

I guess reset and securityOriginsAreEqual could be grouped into one function.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:121
> +    Ref<UserMediaPermissionRequestProxy> request = UserMediaPermissionRequestProxy::create(*this, userMediaID, frameID, userMediaDocumentOriginIdentifier, topLevelDocumentOriginIdentifier, audioDeviceUIDs, videoDeviceUIDs);

Use auto here.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:256
>          auto topLevelOrigin = API::SecurityOrigin::create(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin());

userMediaOrigin is a RefPtr I guess while it should be a Ref.
That would allow removing the *userMediaOrigin.get() below.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:262
> +        for (auto deviceUID : audioDeviceUIDs) {

Would "for (const auto&" be more efficient?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:266
> +            }

I wonder whether this pattern could be done at vector level, something like T* firstMatching(const MatchFunction&) and its const version.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:40
> +    FrameAuthorizationState(UserMediaPermissionRequestProxy&);

Should be explicit.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:41
> +    ~FrameAuthorizationState() { }

Do we need it?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:38
> +    , m_topLevelDocumentSecurityOrigin(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin())

Has this strategy of persistent granting the potential to be reused in other APIs?
If so, I would introduce a structure for frameID and origins.
That would also reduce the number of passed parameters which might improve readability.

> LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:48
> +                // successfully created a stream for the mock audio device.

Have we tests that exercice the case of no-persistency?
Like the case of equal returning false for obvious reasons (port change) but also non obvious (dom flag I talked above)

[Eric Carlson]

Comment on attachment 294838 [details]
Proposed patch.

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

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:51
>> +    if (deviceUID.isEmpty())
> 
> Can this case happen?

Yes, because userMediaAccessWasDenied() is sometimes called before a request is created (e.g. when the UIClient doesn't implement decidePolicyForUserMediaPermissionRequest).

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:72
>> +    } while (0);
> 
> Is it left-over code, or were you planning to add something like ASSERT(!securityOriginsAreEqual(request))?

I combined reset and securityOriginsAreEqual.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:92
>> +            state->reset(request);
> 
> I guess reset and securityOriginsAreEqual could be grouped into one function.

Good idea, fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:121
>> +    Ref<UserMediaPermissionRequestProxy> request = UserMediaPermissionRequestProxy::create(*this, userMediaID, frameID, userMediaDocumentOriginIdentifier, topLevelDocumentOriginIdentifier, audioDeviceUIDs, videoDeviceUIDs);
> 
> Use auto here.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:262
>> +        for (auto deviceUID : audioDeviceUIDs) {
> 
> Would "for (const auto&" be more efficient?

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:266
>> +            }
> 
> I wonder whether this pattern could be done at vector level, something like T* firstMatching(const MatchFunction&) and its const version.

I will leave that for another day.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:40
>> +    FrameAuthorizationState(UserMediaPermissionRequestProxy&);
> 
> Should be explicit.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:41
>> +    ~FrameAuthorizationState() { }
> 
> Do we need it?

No, removed.

>> LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:48
>> +                // successfully created a stream for the mock audio device.
> 
> Have we tests that exercice the case of no-persistency?
> Like the case of equal returning false for obvious reasons (port change) but also non obvious (dom flag I talked above)

I modified WTR to track the number of times a gUM call makes it to the UA, and added methods so it can be read and reset from script,  and added a test.

[Eric Carlson]

Created attachment 295500 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 295500 [details]
Patch for landing.

Rejecting attachment 295500 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 295500, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2584177

[Eric Carlson]

Created attachment 295509 [details]
Updated patch for landing.

[WebKit Commit Bot]

Comment on attachment 295509 [details]
Updated patch for landing.

Clearing flags on attachment: 295509

Committed r209008: <http://trac.webkit.org/changeset/209008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to comment #11)
> Comment on attachment 295509 [details]
> Updated patch for landing.
> 
> Clearing flags on attachment: 295509
> 
> Committed r209008: <http://trac.webkit.org/changeset/209008>

This change appears to have caused two fast/mediastream test issues:

Failure:
fast/mediastream/MediaDevices-getUserMedia.html

Timeout:
fast/mediastream/delayed-permission-allowed.html

https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r209012%20(11295)/results.html

[Ryan Haddad]

Reverted r209008 for reason:

This change appears to have caused two fast/mediastrem LayoutTests to fail.

Committed r209024: <http://trac.webkit.org/changeset/209024>

[Eric Carlson]

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

[Eric Carlson]

Created attachment 295606 [details]
Updated patch for landing.

[WebKit Commit Bot]

Comment on attachment 295606 [details]
Updated patch for landing.

Clearing flags on attachment: 295606

Committed r209082: <http://trac.webkit.org/changeset/209082>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.