EME: Allow temporary sessions to be removed for ClearKey only.

EME: Allow temporary sessions to be removed for ClearKey only.

The EME spec allows MediaKeySession::remove() to be called on temporary
sessions, so update the code to allow it. Doing this for ClearKey
only as other CDMs may not support it.

BUG=616166
TEST=updated tests pass

Review-Url: https://codereview.chromium.org/2831963003
Cr-Commit-Position: refs/heads/master@{#467106}
Committed: https://chromium.googlesource.com/chromium/src/+/74fbc3d0e83f922543913db7b8b86a65c9d176d7

[jrummell, 2017-04-21 00:17:16]

jrummell@chromium.org changed reviewers:
+ xhwang@chromium.org

[jrummell, 2017-04-21 00:17:16]

PTAL.

[jrummell, 2017-04-21 00:19:49]

The CQ bit was checked by jrummell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 00:20:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 00:22:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 00:22:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xhwang, 2017-04-21 18:00:28]

Thanks for working on this!

As commented below, this CL is limited to ClearKey and we should update the CL
title to reflect this. We probably don't want to enable this for other key
systems until we understand how they behave (with test coverage).

https://codereview.chromium.org/2831963003/diff/1/media/cdm/aes_decryptor_uni...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/2831963003/diff/1/media/cdm/aes_decryptor_uni...
media/cdm/aes_decryptor_unittest.cc:787: RemoveSession(session_id);
Can we decrypt again and make sure the keys are not available?

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-syntax.html
(right):

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-syntax.html:809:
function create_remove_exception_test(mediaKeys, type, initData)
Now there's no exception... what's the value of this test? Is this covered by
the create_remove_test below?

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-syntax.html:873:
return Promise.resolve();
We probably should have a new test covering the key status/expiration etc after
remove.

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:816: // 3.
Let promise be a new promise.
This change will also affect all key systems (e.g. Widevine) on all platforms. I
am not sure whether it's ready in those cases, e.g. WV on desktop/android,
ChromeCast, etc. And we probably should have browser tests covering those cases.

So, to avoid making this CL too big. Shall we limit this CL to only ClearKey?
Then we can fix it for Widevine in the next CL.

FWIW, to support WV on desktop, we need to update the CDM interface as well:
https://cs.chromium.org/chromium/src/media/cdm/api/content_decryption_module....

[jrummell, 2017-04-22 00:21:54]

Description was changed from

==========
EME: Allow temporary sessions to be removed.

The EME spec allows MediaKeySession::remove() to be called on temporary
sessions, so update the code to allow it.

BUG=616166
TEST=updated tests pass
==========

to

==========
EME: Allow temporary sessions to be removed for ClearKey only.

The EME spec allows MediaKeySession::remove() to be called on temporary
sessions, so update the code to allow it. Doing this for ClearKey
only as other CDMs may not support it.

BUG=616166
TEST=updated tests pass
==========

[jrummell, 2017-04-22 00:23:17]

Updated.

https://codereview.chromium.org/2831963003/diff/1/media/cdm/aes_decryptor_uni...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/2831963003/diff/1/media/cdm/aes_decryptor_uni...
media/cdm/aes_decryptor_unittest.cc:787: RemoveSession(session_id);
On 2017/04/21 18:00:28, xhwang_slow wrote:
> Can we decrypt again and make sure the keys are not available?

Done (and added a test to remove then close the session).

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-syntax.html
(right):

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-syntax.html:809:
function create_remove_exception_test(mediaKeys, type, initData)
On 2017/04/21 18:00:28, xhwang_slow wrote:
> Now there's no exception... what's the value of this test? Is this covered by
> the create_remove_test below?

"exception" means failure.

Test still verifies that "remove() on an uninitialized session should fail."
Previously it tested 2 failures, but now it only does 1.

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-syntax.html:873:
return Promise.resolve();
On 2017/04/21 18:00:28, xhwang_slow wrote:
> We probably should have a new test covering the key status/expiration etc
after
> remove.

New test added.

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/2831963003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:816: // 3.
Let promise be a new promise.
On 2017/04/21 18:00:28, xhwang_slow wrote:
> This change will also affect all key systems (e.g. Widevine) on all platforms.
I
> am not sure whether it's ready in those cases, e.g. WV on desktop/android,
> ChromeCast, etc. And we probably should have browser tests covering those
cases.
> 
> So, to avoid making this CL too big. Shall we limit this CL to only ClearKey?
> Then we can fix it for Widevine in the next CL.
> 
> FWIW, to support WV on desktop, we need to update the CDM interface as well:
>
https://cs.chromium.org/chromium/src/media/cdm/api/content_decryption_module....

Limited this to ClearKey only. Unfortunately there doesn't appear to be a common
place that knows about key_system and session type, so added
|is_persistent_session_| to webcontentdecryptionmodulesession_impl and checked
there.

[xhwang, 2017-04-25 16:43:49]

lgtm % nits, thanks!

https://codereview.chromium.org/2831963003/diff/20001/media/blink/webcontentd...
File media/blink/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/2831963003/diff/20001/media/blink/webcontentd...
media/blink/webcontentdecryptionmodulesession_impl.cc:473: "The session type is
not persistent.");
This string is not an error... How about something like "remove() on temporary
session is not supported for the key system"?

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-remove.html
(right):

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-remove.html:4:
<title>Test MediaKeySession remove() function</title>
About the file name and this title, should this be limited to temporary session?
Just to avoid the confusion since people may naturally think of persistent
session when looking at remove().

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js
(right):

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js:187:
// { expected: [key1], unexpected: [key2] }, 'usable');
nit: Since we are already using a dictionary for the expectation, does it make
sense to put the |status| in |keys| as well? e.g.
{ expected: {key1: 'usable', key2: 'usable'}, unexpected: [key2] })

[jrummell, 2017-04-25 17:54:10]

Thanks for the reviews.

https://codereview.chromium.org/2831963003/diff/20001/media/blink/webcontentd...
File media/blink/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/2831963003/diff/20001/media/blink/webcontentd...
media/blink/webcontentdecryptionmodulesession_impl.cc:473: "The session type is
not persistent.");
On 2017/04/25 16:43:48, xhwang_slow wrote:
> This string is not an error... How about something like "remove() on temporary
> session is not supported for the key system"?

I just copied the error from MKS. Done.

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-remove.html
(right):

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-remove.html:4:
<title>Test MediaKeySession remove() function</title>
On 2017/04/25 16:43:48, xhwang_slow wrote:
> About the file name and this title, should this be limited to temporary
session?
> Just to avoid the confusion since people may naturally think of persistent
> session when looking at remove().

Done.

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js
(right):

https://codereview.chromium.org/2831963003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js:187:
// { expected: [key1], unexpected: [key2] }, 'usable');
On 2017/04/25 16:43:48, xhwang_slow wrote:
> nit: Since we are already using a dictionary for the expectation, does it make
> sense to put the |status| in |keys| as well? e.g.
> { expected: {key1: 'usable', key2: 'usable'}, unexpected: [key2] })

Good idea, except I think it needs to be { expected: [[key1, 'usable'], [key2,
'usable']], unexpected: [key2] } since we need the values of the keys as well.
Future CL since this is used in multiple places.

[jrummell, 2017-04-25 17:54:31]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2017-04-25 17:54:31]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2831963003/#ps40001
(title: "final changes")

[commit-bot: I haz the power, 2017-04-25 17:54:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 17:58:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 17:58:40]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jrummell, 2017-04-25 18:54:52]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2017-04-25 18:54:52]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2831963003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-25 18:55:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 20:54:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493146492519890,
"parent_rev": "e0e2dc3f7bd8b62483c2c4b8f1739158a1b05c79", "commit_rev":
"74fbc3d0e83f922543913db7b8b86a65c9d176d7"}

[commit-bot: I haz the power, 2017-04-25 20:54:41]

Description was changed from

==========
EME: Allow temporary sessions to be removed for ClearKey only.

The EME spec allows MediaKeySession::remove() to be called on temporary
sessions, so update the code to allow it. Doing this for ClearKey
only as other CDMs may not support it.

BUG=616166
TEST=updated tests pass
==========

to

==========
EME: Allow temporary sessions to be removed for ClearKey only.

The EME spec allows MediaKeySession::remove() to be called on temporary
sessions, so update the code to allow it. Doing this for ClearKey
only as other CDMs may not support it.

BUG=616166
TEST=updated tests pass

Review-Url: https://codereview.chromium.org/2831963003
Cr-Commit-Position: refs/heads/master@{#467106}
Committed:
https://chromium.googlesource.com/chromium/src/+/74fbc3d0e83f922543913db7b8b8...
==========

[commit-bot: I haz the power, 2017-04-25 20:54:42]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/74fbc3d0e83f922543913db7b8b8...