Add a quirk to disable Modern EME for sites which are broken with it enabled
<rdar://44816624>
Created attachment 350995 [details] Patch
Created attachment 350996 [details] Patch
Created attachment 351009 [details] Patch
Created attachment 351118 [details] Patch
Created attachment 351135 [details] Patch
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
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
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>.
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?
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.
Created attachment 351288 [details] Patch
Created attachment 351290 [details] Patch
Created attachment 351291 [details] Patch
Created attachment 351294 [details] Patch Fixed ContentSecurityPolicySource, which wasn't including what it was using.
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.
(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.
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.
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.
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?
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().
(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.
(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).
Comment on attachment 351294 [details] Patch Clearing flags on attachment: 351294 Committed r236818: <https://trac.webkit.org/changeset/236818>
All reviewed patches have been landed. Closing bug.