Bug 190051 – Add a quirk to disable Modern EME for sites which are broken with it enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Add a quirk to disable Modern EME for sites which are broken with it enabled

[Jer Noble]

<rdar://44816624>

[Jer Noble]

Created attachment 350995 [details]
Patch

[Jer Noble]

Created attachment 350996 [details]
Patch

[Jer Noble]

Created attachment 351009 [details]
Patch

[Jer Noble]

Created attachment 351118 [details]
Patch

[Jer Noble]

Created attachment 351135 [details]
Patch

[EWS Watchlist]

Comment on attachment 351135 [details]
Patch

Attachment 351135 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9388893

New failing tests:
media/range-extract-contents-crash.html

[EWS Watchlist]

Created attachment 351157 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Jer Noble]

Ironically, this layout test failure was fixed by the "eliminate raw-pointer usage in fullscreen code" fix which landed 3 minutes after this test was run: <https://bugs.webkit.org/show_bug.cgi?id=188747>.

[Daniel Bates]

Comment on attachment 351135 [details]
Patch

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

> Source/WebCore/bindings/scripts/preprocess-idls.pl:261
> +        || $attributeName eq "PublicIdentifier" || $attributeName eq "DisabledByQuirk" );

Nit: There is a trailing space character before the ')'.

> Source/WebCore/html/HTMLMediaElement.cpp:2632
> +        && (!RuntimeEnabledFeatures::sharedFeatures().encryptedMediaAPIEnabled()
> +            || document().quirks().disableEncryptedMediaAPIQuirk())

Minor: OK as-is. I do not see the need to split this line across two lines. I would write this on one line.

> Source/WebCore/html/HTMLMediaElement.cpp:2769
> +    if (!RuntimeEnabledFeatures::sharedFeatures().encryptedMediaAPIEnabled()
> +        || document().quirks().disableEncryptedMediaAPIQuirk())

Ditto.

> Source/WebCore/html/HTMLMediaElement.idl:107
> +    [Conditional=ENCRYPTED_MEDIA, EnabledAtRuntime=EncryptedMediaAPI, DisabledByQuirk=disableEncryptedMediaAPI] readonly attribute MediaKeys mediaKeys;

Maybe a better name for this extended attribute would be DisabledOnSites? or negate its meaning and call it EnabledOnSite?

> Source/WebCore/page/Quirks.cpp:34
> +Quirks::Quirks(Document& document)

The word "quirks" seems ambiguous. We are talking about site-specific quirks. Unless you envision this being a general purpose mechanism for all kinds of quirky behavior (site-specific, app-specific, et cetera) I suggest we name this class SiteSpecificQuirks. Then we would rename the accessor on document from quirks() to siteSpecificQuirks().

> Source/WebCore/page/Quirks.cpp:41
> +bool Quirks::disableEncryptedMediaAPIQuirk() const

The name of this function reads like it will disable the Encrypted Media API quirk because it starts with a verb, "disable". But purpose of this function is to determine whether to enable or disable the Encrypted Media API quirk and its the callers responsibility to actually take or not take the code path that represents the Encrypted Media API quirk. Moreover, the name of this function does not describe what the "quirk" is. We seem to be using the word "quirk" to describe a general incompatibility that some sites have for an entire Web API. Maybe a better name for this function would be isIncompatibleWithEncryptedMediaAPI? or misbehavesWithEncryptedMediaAPI?

> Source/WebCore/page/Quirks.cpp:58
> +    auto topDomain = m_document->topOrigin().domain().convertToASCIILowercase();
> +
> +    m_disableEncryptedMediaAPIQuirk = topDomain == "amazon.com"
> +        || topDomain.endsWith(".amazon.com")
> +        || topDomain == "primevideo.com"
> +        || topDomain.endsWith(".primevideo.com")
> +        || topDomain == "starz.com"
> +        || topDomain.endsWith(".starz.com")
> +        || topDomain == "hulu.com"
> +        || topDomain.endsWith("hulu.com");

Do Encrypted Media APIs work in sub frames with documents served from these domains? Better question, how did you come to the decision to look at the origin of the top-most document as opposed to m_document?

[Jer Noble]

Comment on attachment 351135 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:2632
>> +            || document().quirks().disableEncryptedMediaAPIQuirk())
> 
> Minor: OK as-is. I do not see the need to split this line across two lines. I would write this on one line.

Sure.

>> Source/WebCore/html/HTMLMediaElement.idl:107
>> +    [Conditional=ENCRYPTED_MEDIA, EnabledAtRuntime=EncryptedMediaAPI, DisabledByQuirk=disableEncryptedMediaAPI] readonly attribute MediaKeys mediaKeys;
> 
> Maybe a better name for this extended attribute would be DisabledOnSites? or negate its meaning and call it EnabledOnSite?

The one Quirk so far is site-specific, but that won't necessarily be true for future quirks.  For example, we have a quirk in the media code where we fire "playing" and "paused" events when autoplay is blocked (to unbreak sites that weren't written with the concept of autoplay blocking in mind).  We might disable that quirk once the site used a modern playback detection API, and that would fit well here in Quirks, but wouldn't be site-specific.

>> Source/WebCore/page/Quirks.cpp:34
>> +Quirks::Quirks(Document& document)
> 
> The word "quirks" seems ambiguous. We are talking about site-specific quirks. Unless you envision this being a general purpose mechanism for all kinds of quirky behavior (site-specific, app-specific, et cetera) I suggest we name this class SiteSpecificQuirks. Then we would rename the accessor on document from quirks() to siteSpecificQuirks().

I do envision it to be the latter.

>> Source/WebCore/page/Quirks.cpp:41
>> +bool Quirks::disableEncryptedMediaAPIQuirk() const
> 
> The name of this function reads like it will disable the Encrypted Media API quirk because it starts with a verb, "disable". But purpose of this function is to determine whether to enable or disable the Encrypted Media API quirk and its the callers responsibility to actually take or not take the code path that represents the Encrypted Media API quirk. Moreover, the name of this function does not describe what the "quirk" is. We seem to be using the word "quirk" to describe a general incompatibility that some sites have for an entire Web API. Maybe a better name for this function would be isIncompatibleWithEncryptedMediaAPI? or misbehavesWithEncryptedMediaAPI?

Yeah, I agree that this name isn't great. "siteHasBrokenEncryptedMediaAPI" is another option. I'll mull it over.

>> Source/WebCore/page/Quirks.cpp:58
>> +        || topDomain.endsWith("hulu.com");
> 
> Do Encrypted Media APIs work in sub frames with documents served from these domains? Better question, how did you come to the decision to look at the origin of the top-most document as opposed to m_document?

Aha, that's another great point. Youtube served in an iframe should check the local document's origin, not the top document's. I'll make this change too.

[Jer Noble]

Created attachment 351288 [details]
Patch

[Jer Noble]

Created attachment 351290 [details]
Patch

[Jer Noble]

Created attachment 351291 [details]
Patch

[Jer Noble]

Created attachment 351294 [details]
Patch

Fixed ContentSecurityPolicySource, which wasn't including what it was using.

[Daniel Bates]

Comment on attachment 351294 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:49
> +    auto domain = m_document->securityOrigin().domain().convertToASCIILowercase();

This does not seem correct as SecurityOrigin::domain() corresponds to the JavaScript value document.domain, which may not be identical to the domain portion of the URL. It seems like we should look at the document's URL.

[Jer Noble]

(In reply to Daniel Bates from comment #16)
> Comment on attachment 351294 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351294&action=review
> 
> > Source/WebCore/page/Quirks.cpp:49
> > +    auto domain = m_document->securityOrigin().domain().convertToASCIILowercase();
> 
> This does not seem correct as SecurityOrigin::domain() corresponds to the
> JavaScript value document.domain, which may not be identical to the domain
> portion of the URL. It seems like we should look at the document's URL.

document.domain can only change the subdomain, which is immaterial to this patch as it's checking the TLD+1. Checking the document URL would be problematic for the window.open() case, which inherits the SecurityOrigin of it's parent document, but may not have an explicit URL of its own.

[Daniel Bates]

Comment on attachment 351294 [details]
Patch

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

>> Source/WebCore/page/Quirks.cpp:49
>> +    auto domain = m_document->securityOrigin().domain().convertToASCIILowercase();
> 
> This does not seem correct as SecurityOrigin::domain() corresponds to the JavaScript value document.domain, which may not be identical to the domain portion of the URL. It seems like we should look at the document's URL.

I should clarify. SecurityOrigin::domain() has a dynamism that makes it seems weird when trying to test for a specific domain and takes advantage of how security origins are inherited to handle child windows, empty URLs, and srcdoc documents. The latter Is a subtle detail and I would suggest adding a comment to explain that we are explicitly taking advantage of this inheritance to make this code work for about:blank, about:srcdoc and empty URLs. With regards to the code in this function, the use of SecurityOrigin::domain() will work out based on how we do the comparison and suffix comparisons below.

[Daniel Bates]

Comment on attachment 351294 [details]
Patch

I am not happy with this patch. If there is another Encrypted Media API expert that can ascertain whether this is patch represents the only choice we have to fixing the domains hardcoded in the patch then I would prefer they take a look at this. This patch will hinder the web author behind the domains hardcoded in this patch from fixing their site so as to Encrypted Media API until the next shipped release of WebKit. If there was a more surgical way to detect the bad usage of the Encrypted Media API and fix up these hardcoded sites then that would be preferable than an outright ban. From what Jer Noble has told me on IRC this patch represents the best approach to solving the media issues on the effected site together with evangelism.

[Jon Lee]

Comment on attachment 351294 [details]
Patch

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

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:32
> +#include "TextEncoding.h"

stray include?

[Daniel Bates]

Comment on attachment 351294 [details]
Patch

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

>>>> Source/WebCore/page/Quirks.cpp:49
>>>> +    auto domain = m_document->securityOrigin().domain().convertToASCIILowercase();
>>> 
>>> This does not seem correct as SecurityOrigin::domain() corresponds to the JavaScript value document.domain, which may not be identical to the domain portion of the URL. It seems like we should look at the document's URL.
>> 
>> document.domain can only change the subdomain, which is immaterial to this patch as it's checking the TLD+1. Checking the document URL would be problematic for the window.open() case, which inherits the SecurityOrigin of it's parent document, but may not have an explicit URL of its own.
> 
> I should clarify. SecurityOrigin::domain() has a dynamism that makes it seems weird when trying to test for a specific domain and takes advantage of how security origins are inherited to handle child windows, empty URLs, and srcdoc documents. The latter Is a subtle detail and I would suggest adding a comment to explain that we are explicitly taking advantage of this inheritance to make this code work for about:blank, about:srcdoc and empty URLs. With regards to the code in this function, the use of SecurityOrigin::domain() will work out based on how we do the comparison and suffix comparisons below.

There is no need to convert to lower case. We should use String::endsWithIgnoringASCIICase() and equalIgnoringASCIICase().

[Jer Noble]

(In reply to Daniel Bates from comment #21)
> Comment on attachment 351294 [details]
>
> There is no need to convert to lower case. We should use
> String::endsWithIgnoringASCIICase() and equalIgnoringASCIICase().

equalIgnoringASCIICase explicitly folds both sides of the input each time it's called. Folding the source once and doing a strict-case comparison is much less expensive.

[Jer Noble]

(In reply to Jon Lee from comment #20)
> Comment on attachment 351294 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351294&action=review
> 
> > Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:32
> > +#include "TextEncoding.h"
> 
> stray include?

No, adding a new file to Sources.txt reveals an existing bug where this file did not include a needed header (which was included by a previous file and the bug was covered up by the unified build system).

[WebKit Commit Bot]

Comment on attachment 351294 [details]
Patch

Clearing flags on attachment: 351294

Committed r236818: <https://trac.webkit.org/changeset/236818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.