[eme] Handle multiple calls to MediaKeySession.close()

[eme] Handle multiple calls to MediaKeySession.close()

Since the close() operation is asynchronous, there is a window where
close() can be called a second time before the session is actually
closed. Changes allow the second close() to succeed if it is called
while the first close() is being processed.

BUG=668312
TEST=new test passes

Committed: https://crrev.com/38f66de2415c72b0e37ead48bba85d335d37fc0c
Cr-Commit-Position: refs/heads/master@{#438591}

[jrummell, 2016-12-03 01:29:52]

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

[jrummell, 2016-12-03 01:29:53]

PTAL.

[xhwang, 2016-12-03 03:11:51]

This change will push the complexity down to every CDM implementation. I feel we
should keep the chromium CDM API as simple as possible and fix this issue in
blink. Given that the BUG isn't high priority and we have a spec bug filed, can
we follow up in the spec land and try to find a solution first?

[jrummell, 2016-12-06 00:16:41]

On 2016/12/03 03:11:51, xhwang wrote:
> This change will push the complexity down to every CDM implementation. I feel
we
> should keep the chromium CDM API as simple as possible and fix this issue in
> blink. Given that the BUG isn't high priority and we have a spec bug filed,
can
> we follow up in the spec land and try to find a solution first?

The spec allows CDMs to close sessions at any time, so the CDM will always have
to handle a close() happening after the session is closed, due to inter-process
communication. Even if the spec is updated, I think this window will always
exist. Plus even if it is completely handled in blink, this change just won't be
exercised. (AesDecryptor doesn't close sessions itself, so it's a special case.)

[xhwang, 2016-12-07 06:51:42]

Thanks! Could you please also update media_keys.h and CDM.h to make this clear?

https://codereview.chromium.org/2545083004/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/2545083004/diff/1/media/cdm/aes_decryptor.cc#...
media/cdm/aes_decryptor.cc:332: // TODO(jrummell): Convert back to a DCHECK once
prefixed EME is removed.
Can we do this now?

https://codereview.chromium.org/2545083004/diff/1/media/cdm/aes_decryptor.cc#...
media/cdm/aes_decryptor.cc:418: if (it == valid_sessions_.end()) {
Now we cannot detect an invalid sesion_id... Since the spec actually doesn't say
removing the session, shall we keep a list of closed_sessions_?

Also valid_sessions_ seems ambiguous. It really should be open_sessions_.

https://codereview.chromium.org/2545083004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-multiple-close.html
(right):

https://codereview.chromium.org/2545083004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-multiple-close.html:4:
<title>Test multiple calls to MediaKeySession.close()</title>
Please add a reference to the spec bug.

[jrummell, 2016-12-07 22:26:50]

Updated.

> Could you please also update media_keys.h and CDM.h to make this clear?

media_keys.h done here, added comment in bug for CDM_9.

https://codereview.chromium.org/2545083004/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/2545083004/diff/1/media/cdm/aes_decryptor.cc#...
media/cdm/aes_decryptor.cc:332: // TODO(jrummell): Convert back to a DCHECK once
prefixed EME is removed.
On 2016/12/07 06:51:42, xhwang wrote:
> Can we do this now?

Nope (due to this being called asynchronously). Added comment and link to EME
spec bug.

https://codereview.chromium.org/2545083004/diff/1/media/cdm/aes_decryptor.cc#...
media/cdm/aes_decryptor.cc:418: if (it == valid_sessions_.end()) {
On 2016/12/07 06:51:42, xhwang wrote:
> Now we cannot detect an invalid sesion_id... Since the spec actually doesn't
say
> removing the session, shall we keep a list of closed_sessions_?

I don't think there is any way to get a invalid session_id, unless due to a bug
somewhere. So it's not necessary to bother with closed session IDs. Worst case
is we indicate that the invalid session ID is closed, which I guess is
technically true as it doesn't exist.

> Also valid_sessions_ seems ambiguous. It really should be open_sessions_.

Done.

https://codereview.chromium.org/2545083004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-multiple-close.html
(right):

https://codereview.chromium.org/2545083004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-multiple-close.html:4:
<title>Test multiple calls to MediaKeySession.close()</title>
On 2016/12/07 06:51:42, xhwang wrote:
> Please add a reference to the spec bug.

Done (in comments below).

[xhwang, 2016-12-07 22:36:55]

Thanks! lgtm

[jrummell, 2016-12-07 23:09:01]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2016-12-07 23:09:39]

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, 2016-12-08 00:40:03]

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

[commit-bot: I haz the power, 2016-12-08 00:40:04]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jrummell, 2016-12-08 00:49:14]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2016-12-08 00:50:01]

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, 2016-12-08 02:23:26]

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

[commit-bot: I haz the power, 2016-12-08 02:23:26]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jrummell, 2016-12-08 23:52:42]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:53:08]

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, 2016-12-09 00:05:39]

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

[commit-bot: I haz the power, 2016-12-09 00:05:40]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[jrummell, 2016-12-12 22:47:52]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-12-12 22:47:53]

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/2545083004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-12 22:49:05]

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, 2016-12-13 01:19:38]

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

[commit-bot: I haz the power, 2016-12-13 01:19:39]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2016-12-13 22:31:48]

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

[commit-bot: I haz the power, 2016-12-13 22:33:02]

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, 2016-12-14 00:42:44]

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

[commit-bot: I haz the power, 2016-12-14 00:42:45]

Dry run: This issue passed the CQ dry run.

[jrummell, 2016-12-14 19:23:00]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-12-14 19:23:00]

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/2545083004/#ps60001
(title: "rebase for MediaKeys rename")

[commit-bot: I haz the power, 2016-12-14 19:23:30]

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, 2016-12-14 19:31:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481743380057990,
"parent_rev": "cbc4c3972b317b7933679a5e630085d129187e71", "commit_rev":
"d3506ca26e72123529ba76b96b2a7b7d9866b305"}

[commit-bot: I haz the power, 2016-12-14 19:34:00]

Description was changed from

==========
[eme] Handle multiple calls to MediaKeySession.close()

Since the close() operation is asynchronous, there is a window where
close() can be called a second time before the session is actually
closed. Changes allow the second close() to succeed if it is called
while the first close() is being processed.

BUG=668312
TEST=new test passes
==========

to

==========
[eme] Handle multiple calls to MediaKeySession.close()

Since the close() operation is asynchronous, there is a window where
close() can be called a second time before the session is actually
closed. Changes allow the second close() to succeed if it is called
while the first close() is being processed.

BUG=668312
TEST=new test passes

Committed: https://crrev.com/38f66de2415c72b0e37ead48bba85d335d37fc0c
Cr-Commit-Position: refs/heads/master@{#438591}
==========

[commit-bot: I haz the power, 2016-12-14 19:34:01]

Patchset 4 (id:??) landed as
https://crrev.com/38f66de2415c72b0e37ead48bba85d335d37fc0c
Cr-Commit-Position: refs/heads/master@{#438591}