Bug 221187 – Permission request API for MediaKeySystem access support

• Component: WebKit2
• Status: RESOLVED FIXED

[Philippe Normand]

At least GTK/WPE would like to have API for asking permission to the user when EME's MediaKeyAccess is being requested by a page. On Apple platforms the permission is granted by default until a decision is made about supporting this in their APIUIClient.

[Philippe Normand]

Created attachment 418826 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

I expect apple's EWS to fail. I'll check the build issues.

[Philippe Normand]

Created attachment 418853 [details]
Patch

[Philippe Normand]

Created attachment 418854 [details]
Patch

[Philippe Normand]

Created attachment 418855 [details]
Patch

[Philippe Normand]

Created attachment 418857 [details]
Patch

[Philippe Normand]

Created attachment 418859 [details]
Patch

[Philippe Normand]

mac-wk1 layout tests failing, most likely because of missing UIDelegate support for the permission request. I'll check it.

Any hints regarding the watch/tv build failures would be welcome though!

[Philippe Normand]

Created attachment 418878 [details]
Patch

[Philippe Normand]

Created attachment 418979 [details]
Patch

[Philippe Normand]

Created attachment 418980 [details]
Patch

[Jer Noble]

Comment on attachment 418980 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemClient.h:29
> +#include <wtf/CompletionHandler.h>

This doesn't look necessary; could this be removed?

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemClient.h:48
> +WEBCORE_EXPORT void provideMediaKeySystemTo(Page*, MediaKeySystemClient*);

Both these should probably be references.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:29
> +#include "FeaturePolicy.h"

Could this be moved into the implementation file?

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:31
> +#include "Page.h"

Ditto here, but the `from()` method would need to be un-inlined, which is fine since supplementName() is already in the implementation file also.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:32
> +#include <wtf/CompletionHandler.h>

This doesn't appear to be used.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:41
> +    explicit MediaKeySystemController(MediaKeySystemClient*);

This should take a reference; and MediaKeySystemClient should probably be a CanMakeWeakPtr.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:44
> +    MediaKeySystemClient* client() const { return m_client; }

Is this really needed?

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:55
> +    MediaKeySystemClient* m_client;

This should probably be a WeakPtr<MediaKeySystemClient>.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:60
> +    m_client->requestMediaKeySystem(request);

This should have a NULL check.

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.h:65
> +    m_client->cancelMediaKeySystemRequest(request);

Ditto.

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequest.h:29
> +#include <WebCore/ClientOrigin.h>

This doesn't seem to be used. Can it be moved to the implementation file?

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequestManagerProxy.h:31
> +#include <wtf/CompletionHandler.h>
> +#include <wtf/Deque.h>

These doesn't appear to be used.

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequestManagerProxy.h:35
> +#include <wtf/RunLoop.h>
> +#include <wtf/Seconds.h>

Ditto.

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequestManagerProxy.h:50
> +#if !RELEASE_LOG_DISABLED
> +    , private LoggerHelper
> +#endif

So, ...

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequestManagerProxy.h:72
> +#if !RELEASE_LOG_DISABLED
> +    const Logger& logger() const final;
> +    const void* logIdentifier() const final { return m_logIdentifier; }
> +    const char* logClassName() const override { return "MediaKeySystemPermissionRequestManagerProxy"; }
> +    WTFLogChannel& logChannel() const final;
> +#endif

It turns out there's not a great reason to explicitly inherit from LoggerHelper (unless a subclass relies upon them); these methods just need to be defined in the scope in which LOG macro appears.  The methods need to exist, but it's also fine just to define them as local statics, if needed. Also, this means you don't need the LoggerHelper.h header (nor even Logger or WTFLogChannel, if they're forward declared).

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequestManagerProxy.h:80
> +    Ref<const Logger> m_logger;

This isn't needed, as you just get the Logger from the page.

> Source/WebKit/UIProcess/MediaKeySystemPermissionRequestProxy.h:68
> +    MediaKeySystemPermissionRequestManagerProxy* m_manager;

This should be a `WeakPtr<MediaKeySystemPermissionRequestManagerProxy>`.

> Source/WebKit/UIProcess/SpeechRecognitionRemoteRealtimeMediaSource.cpp:37
> +#include <WebCore/CARingBuffer.h>
> +#include <WebCore/WebAudioBufferList.h>

Is this perhaps an unrelated change?

> Source/WebKit/UIProcess/SpeechRecognitionRemoteRealtimeMediaSource.h:30
> +#include "SharedMemory.h"

Ditto?

> Source/WebKit/UIProcess/WebPageProxy.cpp:10465
> +    completionHandler(false);

This appears to deny all media key system access unless clients specifically add a UIClient to do the reverse, which is a breaking change. Could this just be "true" by default instead?

> Source/WebKitLegacy/mac/WebCoreSupport/WebMediaKeySystemClient.h:34
> +    WebView *webView() { return m_webView; }

This never appears to be called. Is it necessary, or indeed is the WebView* needed at all?

> Source/WebKitLegacy/mac/WebCoreSupport/WebMediaKeySystemClient.mm:43
> +void WebMediaKeySystemClient::pageDestroyed()
> +{
> +    delete this;
> +}

This seems really weird. (Same above, too.)

[Jer Noble]

(In reply to Philippe Normand from comment #9)
> mac-wk1 layout tests failing, most likely because of missing UIDelegate
> support for the permission request. I'll check it.
> 
> Any hints regarding the watch/tv build failures would be welcome though!

Looks like MediaKeySystemRequest is guarded by `#if ENABLE(MEDIA_STREAM)` not `#if ENABLE(ENCRYPTED_MEDIA)`. And the former isn't enabled on TV or watchOS.

[Philippe Normand]

Created attachment 419383 [details]
Patch

[Philippe Normand]

Comment on attachment 418980 [details]
Patch

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

Thanks Jer!

>> Source/WebKit/UIProcess/MediaKeySystemPermissionRequest.h:29
>> +#include <WebCore/ClientOrigin.h>
> 
> This doesn't seem to be used. Can it be moved to the implementation file?

The class is entirely implemented in the header.

>> Source/WebKit/UIProcess/SpeechRecognitionRemoteRealtimeMediaSource.cpp:37
>> +#include <WebCore/WebAudioBufferList.h>
> 
> Is this perhaps an unrelated change?

Actually it is relevant to this patch, which somehow changes the unified build "layout" :/

[Philippe Normand]

(In reply to Jer Noble from comment #14)
> (In reply to Philippe Normand from comment #9)
> > mac-wk1 layout tests failing, most likely because of missing UIDelegate
> > support for the permission request. I'll check it.
> > 
> > Any hints regarding the watch/tv build failures would be welcome though!
> 
> Looks like MediaKeySystemRequest is guarded by `#if ENABLE(MEDIA_STREAM)`
> not `#if ENABLE(ENCRYPTED_MEDIA)`. And the former isn't enabled on TV or
> watchOS.

Oh wow, nice catch! 😅

[Philippe Normand]

Created attachment 419384 [details]
Patch

[Jer Noble]

Comment on attachment 419384 [details]
Patch

r=me, with one comment:

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

> Source/WebCore/Modules/encryptedmedia/MediaKeySystemController.cpp:57
> +    m_client->pageDestroyed();

Null check?

[Radar WebKit Bug Importer]

<rdar://problem/74073825>

[Philippe Normand]

Committed r272480: <https://trac.webkit.org/changeset/272480>

[Ryan Haddad]

Follow-up fix for macCatalyst in https://trac.webkit.org/changeset/272500/webkit

[Philippe Normand]

Thanks Ryan, sorry for the trouble.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 221586

[Philippe Normand]

Fix:

diff --git a/Source/WebCore/Modules/encryptedmedia/MediaKeySystemRequest.cpp b/Source/WebCore/Modules/encryptedmedia/MediaKeySystemRequest.cpp
index e669715e5de7..2a2fec324b0d 100644
--- a/Source/WebCore/Modules/encryptedmedia/MediaKeySystemRequest.cpp
+++ b/Source/WebCore/Modules/encryptedmedia/MediaKeySystemRequest.cpp
@@ -48,6 +48,7 @@ Ref<MediaKeySystemRequest> MediaKeySystemRequest::create(Document& document, con
 
 MediaKeySystemRequest::MediaKeySystemRequest(Document& document, const String& keySystem, Ref<DeferredPromise>&& promise)
     : ActiveDOMObject(document)
+    , m_identifier(MediaKeySystemRequestIdentifier::generate())
     , m_keySystem(keySystem)
     , m_promise(WTFMove(promise))
 {

[Philippe Normand]

Committed r272573: <https://trac.webkit.org/changeset/272573>

[Peng Liu]

This patch breaks Netflix playback on macOS Big Sur. See bug 224384.

[Peng Liu]

(In reply to Peng Liu from comment #27)
> This patch breaks Netflix playback on macOS Big Sur. See bug 224384.

[Philippe Normand]

Comment on attachment 419384 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2129
> +            if (!m_client.decidePolicyForMediaKeySystemPermissionRequest) {
> +                completionHandler(false);
> +                return;
> +            }

Can you check if this hit by Safari? I suspect this is the bug, if your APIUIClient doesn´t have a decidePolicyForMediaKeySystemPermissionRequest handler, the permission request is denied.

[Eric Carlson]

Comment on attachment 419384 [details]
Patch

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

>> Source/WebKit/UIProcess/API/C/WKPage.cpp:2129
>> +            }
> 
> Can you check if this hit by Safari? I suspect this is the bug, if your APIUIClient doesn´t have a decidePolicyForMediaKeySystemPermissionRequest handler, the permission request is denied.

Your initial comment in this bug was "On Apple platforms the permission is granted by default until a decision is made about supporting this in their APIUIClient". Did this change?

[Philippe Normand]

(In reply to Eric Carlson from comment #30)
> Comment on attachment 419384 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419384&action=review
> 
> >> Source/WebKit/UIProcess/API/C/WKPage.cpp:2129
> >> +            }
> > 
> > Can you check if this hit by Safari? I suspect this is the bug, if your APIUIClient doesn´t have a decidePolicyForMediaKeySystemPermissionRequest handler, the permission request is denied.
> 
> Your initial comment in this bug was "On Apple platforms the permission is
> granted by default until a decision is made about supporting this in their
> APIUIClient". Did this change?

No. I was just commenting on this specific part of the patch because of a hunch. I don't have a Netflix account, so can't reproduce this issue.

Are you sure this is a WebKit issue? Can you provide a reduced test-case?

[Peng Liu]

(In reply to Philippe Normand from comment #29)
> Comment on attachment 419384 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419384&action=review
> 
> > Source/WebKit/UIProcess/API/C/WKPage.cpp:2129
> > +            if (!m_client.decidePolicyForMediaKeySystemPermissionRequest) {
> > +                completionHandler(false);
> > +                return;
> > +            }
> 
> Can you check if this hit by Safari? I suspect this is the bug, if your
> APIUIClient doesn´t have a decidePolicyForMediaKeySystemPermissionRequest
> handler, the permission request is denied.

I just tested. When the issue happens, function `WebPageProxy::requestMediaKeySystemPermissionForFrame()` is not called at all. The reason seems to be that the WebContent process is in the background and the IPC message is not delivered to the UI process.

[Peng Liu]

Comment on attachment 419384 [details]
Patch

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

> Source/WebKit/WebProcess/EncryptedMedia/MediaKeySystemPermissionRequestManager.cpp:61
> +        sendMediaKeySystemRequest(request);

A bug here. When we open a tab in the background, the request won't be sent, and we won't retry when the tab goes to foreground.

[Philippe Normand]

Comment on attachment 419384 [details]
Patch

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

>> Source/WebKit/WebProcess/EncryptedMedia/MediaKeySystemPermissionRequestManager.cpp:61
>> +        sendMediaKeySystemRequest(request);
> 
> A bug here. When we open a tab in the background, the request won't be sent, and we won't retry when the tab goes to foreground.

Ah! Would the MediaCanStartListener be triggered in such case?

[Peng Liu]

Comment on attachment 419384 [details]
Patch

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

>>> Source/WebKit/WebProcess/EncryptedMedia/MediaKeySystemPermissionRequestManager.cpp:61
>>> +        sendMediaKeySystemRequest(request);
>> 
>> A bug here. When we open a tab in the background, the request won't be sent, and we won't retry when the tab goes to foreground.
> 
> Ah! Would the MediaCanStartListener be triggered in such case?

Yes. Just had a discussion with Eric and Jer. Will upload a patch for review after test. :-)

[Philippe Normand]

Thanks Peng!

[Peng Liu]

(In reply to Philippe Normand from comment #36)
> Thanks Peng!

My pleasure. :-)

Let's close this bug. The issue we discussed will be fixed in bug 224384.