EME: Close existing sessions on CDM failure

EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=new EME tests pass

Committed: https://crrev.com/35c1aa881f655468ebafcf68e51ae2180f67a833
Cr-Commit-Position: refs/heads/master@{#432327}

[jrummell, 2016-10-17 23:53:52]

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

[jrummell, 2016-10-17 23:53:53]

PTAL.

[xhwang, 2016-10-18 18:07:29]

looking good, just a nit and a comment

https://chromiumcodereview.appspot.com/2426813002/diff/1/media/base/cdm_sessi...
File media/base/cdm_session_tracker.h (right):

https://chromiumcodereview.appspot.com/2426813002/diff/1/media/base/cdm_sessi...
media/base/cdm_session_tracker.h:33: void CloseRemainingSessions(SessionClosedCB
session_closed_cb);
nit: pass callbacks by const-ref

https://chromiumcodereview.appspot.com/2426813002/diff/1/media/mojo/clients/m...
File media/mojo/clients/mojo_cdm.cc (right):

https://chromiumcodereview.appspot.com/2426813002/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_cdm.cc:79: base::Bind(&MojoCdm::OnConnectionError,
base::Unretained(this)));
This is registered on line 117. Will that work as well?

[jrummell, 2016-10-20 22:11:25]

Updated the crash tests to verify that the session is closed.

https://codereview.chromium.org/2426813002/diff/1/media/base/cdm_session_trac...
File media/base/cdm_session_tracker.h (right):

https://codereview.chromium.org/2426813002/diff/1/media/base/cdm_session_trac...
media/base/cdm_session_tracker.h:33: void CloseRemainingSessions(SessionClosedCB
session_closed_cb);
On 2016/10/18 18:07:28, xhwang wrote:
> nit: pass callbacks by const-ref

Done.

https://codereview.chromium.org/2426813002/diff/1/media/mojo/clients/mojo_cdm.cc
File media/mojo/clients/mojo_cdm.cc (right):

https://codereview.chromium.org/2426813002/diff/1/media/mojo/clients/mojo_cdm...
media/mojo/clients/mojo_cdm.cc:79: base::Bind(&MojoCdm::OnConnectionError,
base::Unretained(this)));
On 2016/10/18 18:07:28, xhwang wrote:
> This is registered on line 117. Will that work as well?

There won't be any sessions created until Initialize() is done, so it won't be a
problem. Removed this.

[xhwang, 2016-10-20 22:38:59]

looking good, I have one question though.

https://codereview.chromium.org/2426813002/diff/20001/media/test/data/eme_pla...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/2426813002/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/player_utils.js:82: // If a crash is expected,
wait for the session to be closed.
We added the crash test to make sure if the CDM crashes, we'll hit a decode
error on the media element (we had a bug where the pipeline hangs and didn't
report the error before). With this change, are we losing that test coverage?

[jrummell, 2016-11-01 01:07:54]

Description was changed from

==========
EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=EME tests pass
==========

to

==========
EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=new EME tests pass
==========

[jrummell, 2016-11-01 01:08:46]

Updated with new test, restoring the old ones. PS#3 is a rebase to get current,
PS#4 is the changes for the new test.

https://codereview.chromium.org/2426813002/diff/20001/media/test/data/eme_pla...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/2426813002/diff/20001/media/test/data/eme_pla...
media/test/data/eme_player_js/player_utils.js:82: // If a crash is expected,
wait for the session to be closed.
On 2016/10/20 22:38:59, xhwang wrote:
> We added the crash test to make sure if the CDM crashes, we'll hit a decode
> error on the media element (we had a bug where the pipeline hangs and didn't
> report the error before). With this change, are we losing that test coverage?

Yes. Restored the original tests, added 2 new ones.

[xhwang, 2016-11-04 22:37:41]

Thank you and sorry for the delay. I somehow missed this one.

The new PS would work. But I wonder whether it's possible to merge these two
tests.

https://codereview.chromium.org/2426813002/diff/60001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/2426813002/diff/60001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:248: key_system_string !=
kExternalClearKeyCloseOnCrashKeySystem) {
Last time when I was here I thought maybe we should just accept all
org.chromium.externalclearkey.* key systems here. WDYT?

https://codereview.chromium.org/2426813002/diff/60001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:629: key_system_ ==
kExternalClearKeyCloseOnCrashKeySystem)
It seems odd that two tests execute exactly the same code path, but each test
only check part of the test result. See comment below.

https://codereview.chromium.org/2426813002/diff/60001/media/test/data/eme_pla...
File media/test/data/eme_player_js/clearkey_player.js (right):

https://codereview.chromium.org/2426813002/diff/60001/media/test/data/eme_pla...
media/test/data/eme_player_js/clearkey_player.js:35: // the decoder actually
detects and reports the error.
Since these two tests have exactly the same behavior, is it possible to check
both results together? For example, only finish the test when we observe the
decode error AND session close. Then in C++ we can expect
kErrorAndEmeSessionClosed.

[jrummell, 2016-11-07 22:03:48]

Tests now combined.

https://codereview.chromium.org/2426813002/diff/60001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/2426813002/diff/60001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:248: key_system_string !=
kExternalClearKeyCloseOnCrashKeySystem) {
On 2016/11/04 22:37:40, xhwang_OOO_11_07 wrote:
> Last time when I was here I thought maybe we should just accept all
> org.chromium.externalclearkey.* key systems here. WDYT?

No longer adding a key system so this is reverted now. Only problem with
checking org.chromium.externalclearkey.* is that now a typo in the key system
name will be harder to notice.

https://codereview.chromium.org/2426813002/diff/60001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:629: key_system_ ==
kExternalClearKeyCloseOnCrashKeySystem)
On 2016/11/04 22:37:40, xhwang_OOO_11_07 wrote:
> It seems odd that two tests execute exactly the same code path, but each test
> only check part of the test result. See comment below.

Done.

https://codereview.chromium.org/2426813002/diff/60001/media/test/data/eme_pla...
File media/test/data/eme_player_js/clearkey_player.js (right):

https://codereview.chromium.org/2426813002/diff/60001/media/test/data/eme_pla...
media/test/data/eme_player_js/clearkey_player.js:35: // the decoder actually
detects and reports the error.
On 2016/11/04 22:37:41, xhwang_OOO_11_07 wrote:
> Since these two tests have exactly the same behavior, is it possible to check
> both results together? For example, only finish the test when we observe the
> decode error AND session close. Then in C++ we can expect
> kErrorAndEmeSessionClosed.

Done.

[xhwang, 2016-11-09 00:14:40]

lgtm % comment

https://codereview.chromium.org/2426813002/diff/80001/media/test/data/eme_pla...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/2426813002/diff/80001/media/test/data/eme_pla...
media/test/data/eme_player_js/player_utils.js:35: // On failure the session
should have been closed, so verify.
As discussed offline, EME_SESSION_CLOSED_AFTER_ERROR and this comment (...
should have been closed) are a bit misleading since the "decode error" and
"session closed" event can happen in any order. The implementation doesn't care
since "closed" returns a promise which will work in both ways. But we should get
the naming/comment correct.

How about EME_SESSION_CLOSED_AND_ERROR?

[xhwang, 2016-11-09 00:15:04]

BTW, I like this CL! Thank you for the great work!

[jrummell, 2016-11-09 23:44:00]

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

[jrummell, 2016-11-09 23:44:01]

+raymes@ for OWNERS review of content/renderer/pepper/*

https://codereview.chromium.org/2426813002/diff/80001/media/test/data/eme_pla...
File media/test/data/eme_player_js/player_utils.js (right):

https://codereview.chromium.org/2426813002/diff/80001/media/test/data/eme_pla...
media/test/data/eme_player_js/player_utils.js:35: // On failure the session
should have been closed, so verify.
On 2016/11/09 00:14:40, xhwang wrote:
> As discussed offline, EME_SESSION_CLOSED_AFTER_ERROR and this comment (...
> should have been closed) are a bit misleading since the "decode error" and
> "session closed" event can happen in any order. The implementation doesn't
care
> since "closed" returns a promise which will work in both ways. But we should
get
> the naming/comment correct.
> 
> How about EME_SESSION_CLOSED_AND_ERROR?

Done.

[jrummell, 2016-11-15 20:16:15]

pinging raymes@ for OWNERS review of content/renderer/pepper/*. It's a small
change in these 2 files (~10 lines).

[raymes, 2016-11-15 21:38:23]

Sorry for the delay, I missed this. Thanks for pinging.

pepper rubberstamp lgtm

[jrummell, 2016-11-15 22:23:13]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-11-15 22:23:13]

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/2426813002/#ps100001
(title: "rename result (+rebase)")

[commit-bot: I haz the power, 2016-11-15 22:24:12]

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-11-16 00:49:43]

Description was changed from

==========
EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=new EME tests pass
==========

to

==========
EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=new EME tests pass
==========

[commit-bot: I haz the power, 2016-11-16 00:49:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-16 00:52:02]

Description was changed from

==========
EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=new EME tests pass
==========

to

==========
EME: Close existing sessions on CDM failure

When the CDM crashes, any existing open sessions should be closed.

BUG=652856
TEST=new EME tests pass

Committed: https://crrev.com/35c1aa881f655468ebafcf68e51ae2180f67a833
Cr-Commit-Position: refs/heads/master@{#432327}
==========

[commit-bot: I haz the power, 2016-11-16 00:52:03]

Patchset 6 (id:??) landed as
https://crrev.com/35c1aa881f655468ebafcf68e51ae2180f67a833
Cr-Commit-Position: refs/heads/master@{#432327}