Bug 221187

Created attachment 418826 [details]
Patch

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

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

Created attachment 418853 [details]
Patch

Created attachment 418854 [details]
Patch

Created attachment 418855 [details]
Patch

Created attachment 418857 [details]
Patch

Created attachment 418859 [details]
Patch

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!

Created attachment 418878 [details]
Patch

Created attachment 418979 [details]
Patch

Created attachment 418980 [details]
Patch

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.)

(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.

Created attachment 419383 [details]
Patch

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" :/

(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! 😅

Created attachment 419384 [details]
Patch

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?

<rdar://problem/74073825>

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

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

Thanks Ryan, sorry for the trouble.

Re-opened since this is blocked by bug 221586

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))
 {

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

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

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

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.

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?

(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?

(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.

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.

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?

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. :-)

Thanks Peng!

(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.