Closed Bug 1310879 Opened 4 years ago Closed 4 years ago

[EME] Disable EME persistent-license sessions

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

We have decided to disable persistent-license session types in our EME implementation.

https://lists.w3.org/Archives/Public/public-html-media/2016Oct/0034.html

So we should remove ClearKey's support for this session type, disable persistent-license mochitests, and disable the web platform tests that rely on it.
Comment on attachment 8801992 [details]
Bug 1310879 - Disable Web Platform Tests for persistent session types.

https://reviewboard.mozilla.org/r/86556/#review85580
Attachment #8801992 - Flags: review?(kikuo) → review+
Comment on attachment 8801990 [details]
Bug 1310879 - Remove EME persistent-license sessions.

https://reviewboard.mozilla.org/r/86552/#review85796
Attachment #8801990 - Flags: review?(kikuo) → review+
Comment on attachment 8801991 [details]
Bug 1310879 - Remove persistent license support from gmp-clearkey.

https://reviewboard.mozilla.org/r/86554/#review85804

Looks good to me.
Attachment #8801991 - Flags: review?(kikuo) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d160c261384
Remove EME persistent-license sessions. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/736a78915259
Remove persistent license support from gmp-clearkey. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/d344ae14671b
Disable Web Platform Tests for persistent session types. r=kikuo
Backed out for failing browser-chrome test browser/components/contextualidentity/test/browser/browser_eme.js:

https://hg.mozilla.org/integration/autoland/rev/27eef31f9434af59344a313481ada7366c151f59
https://hg.mozilla.org/integration/autoland/rev/9168ac4608163c233af299f6dc1b468c62199f85
https://hg.mozilla.org/integration/autoland/rev/d2d4f0ad9256faae13953dd3b422d9a7fe8e726f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d344ae14671b475b5cbc5374c7025a6ca3e6dce0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=5308255&repo=autoland

02:04:29     INFO -  64 INFO TEST-START | browser/components/contextualidentity/test/browser/browser_eme.js
02:04:29     INFO -  TEST-INFO | started process screentopng
02:04:31     INFO -  TEST-INFO | screentopng: exit 0
02:04:31     INFO -  65 INFO checking window state
02:04:31     INFO -  66 INFO Entering test bound setup
02:04:31     INFO -  67 INFO Leaving test bound setup
02:04:31     INFO -  68 INFO Entering test bound test
02:04:31     INFO -  69 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/browser/components/contextualidentity/test/browser/empty_file.html" line: 0}]
02:04:31     INFO -  70 INFO Console message: MediaKeySystemAccess::GetKeySystemStatus(org.w3.clearkey, minVer=-1) result=available version='1' msg=''
02:04:31     INFO -  71 INFO Console message: navigator.requestMediaKeySystemAccess promise rejected 0x80530009 'Key system configuration is not supported'
02:04:31     INFO -  72 INFO TEST-UNEXPECTED-FAIL | browser/components/contextualidentity/test/browser/browser_eme.js | Uncaught exception - NotSupportedError: Key system configuration is not supported
02:04:31     INFO -  73 INFO Leaving test bound test
Flags: needinfo?(cpearce)
OK. New plan. We pref of persistent-license in ClearKey instead of removing it, and pref it on again for those tests.

Longer term, we'll need a better way to test the clearing of persistent CDM data and CDM isolation.
MozReview-Commit-ID: 6tHlyRl4nWT
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
MozReview-Commit-ID: 6XkToIXzZL8
Attachment #8805361 - Flags: review?(gsquelart)
This removes the ability for ClearKey to instantiate persistent-license
sessions using the EME APIs.

MozReview-Commit-ID: FXj5YORxpas
Attachment #8805362 - Flags: review?(gsquelart)
Comment on attachment 8805358 [details] [diff] [review]
Patch 1 - Pass MediaKeySystemConfiguration to MediaKeys

Sorry, I can't request review using Mozreview, as you can't start a new request once you've been backed out.
Attachment #8805358 - Attachment description: Pass MediaKeySystemConfiguration to MediaKeys → Patch 1 - Pass MediaKeySystemConfiguration to MediaKeys
Flags: needinfo?(cpearce)
Attachment #8805358 - Flags: review?(gsquelart)
Attachment #8801990 - Attachment is obsolete: true
Attachment #8801991 - Attachment is obsolete: true
Attachment #8801992 - Attachment is obsolete: true
Attachment #8805358 - Flags: review?(gsquelart) → review+
Attachment #8805361 - Flags: review?(gsquelart) → review+
Comment on attachment 8805362 [details] [diff] [review]
Patch 3 - Remove EME persistent-license sessions

Review of attachment 8805362 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with suggestion and question:

::: dom/media/eme/MediaKeySystemAccess.cpp
@@ +431,5 @@
>        clearkey.mInitDataTypes.AppendElement(NS_LITERAL_STRING("webm"));
>        clearkey.mPersistentState = KeySystemFeatureSupport::Requestable;
>        clearkey.mDistinctiveIdentifier = KeySystemFeatureSupport::Prohibited;
>        clearkey.mSessionTypes.AppendElement(MediaKeySessionType::Temporary);
> +      if (Preferences::GetBool("media.clearkey.persistent-license.enabled", false)) {

Please consider using MediaPrefs for this pref, unless you think it's going to disappear soon.

Also, I see you didn't add this pref to all.js, is that intended? (I don't know if there are guidelines about that)
Attachment #8805362 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #17)
> Also, I see you didn't add this pref to all.js, is that intended? (I don't
> know if there are guidelines about that)

The convention I was aware of is that pref's not in all.js are "hidden" prefs, in that they're less likely to be discovered by users who go poking around in about:config. So if you don't want people toggling a pref (for example like in this case where the pref is only present to allow testing of some other feature) a hidden pref is a good way to achieve that.
That makes sense, thank you for the explanation.
You need to log in before you can comment on or make changes to this bug.