[eme] Break mojo connection during call

Test breaking the mojo connection to MojoCdm

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass

Review-Url: https://codereview.chromium.org/2592913002
Cr-Commit-Position: refs/heads/master@{#442104}
Committed: https://chromium.googlesource.com/chromium/src/+/0d5d318987dde8b60ca474976128c5543b2e2563

[jrummell, 2016-12-21 01:22:38]

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

[jrummell, 2016-12-21 01:23:02]

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

[jrummell, 2016-12-21 01:23:03]

PTAL (when you get back).

[commit-bot: I haz the power, 2016-12-21 01:23:11]

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-21 01:57:00]

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

[commit-bot: I haz the power, 2016-12-21 01:57:01]

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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jrummell, 2017-01-05 00:47:53]

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

[commit-bot: I haz the power, 2017-01-05 00:48:20]

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-01-05 00:50:15]

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

[commit-bot: I haz the power, 2017-01-05 00:50:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jrummell, 2017-01-05 02:19:24]

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

[commit-bot: I haz the power, 2017-01-05 02:19:58]

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-01-05 02:56:15]

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

[commit-bot: I haz the power, 2017-01-05 02:56:16]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[xhwang, 2017-01-05 06:59:53]

Nice patch! I only have a few comments on the test.

https://codereview.chromium.org/2592913002/diff/20001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2592913002/diff/20001/media/base/mock_filters...
media/base/mock_filters.h:473: uint32_t promise_id));
Instead of using the promise adapter to convert the promise to an ID, I think we
could also do something like 

MOCK_METHOD4(OnCreateSessionAndGenerateRequest,
                                ....
                                std::unique_ptr<NewSessionCdmPromise>& promise);

Note that the promise is passed as a reference. I didn't try this but if it can
work it'll be simpler, as we don't need to keep the id around and ask the
MockCdm to resolve/reject the promise, we can just resolve/reject it directly.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_cdm_unittest.cc (right):

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:92: remote_cdm_ = new
NiceMock<MockCdm>();
Can we use StrictMock?

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:101: WithArg<7>(CdmCreated(remote_cdm_,
""))));
This is pretty cool but seems a bit overkill. For the foreseeable future,
MockCdmFactory will only create MockCdm. Does it make sense to just have a
concrete MockCdmFactory implementation which will either return a MockCdm, or
nullptr, based on a flag (similar to how we construct MockCdmPromise)? Then this
code and MockCdm impl will be a bit simpler.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:179: EXPECT_CALL(*remote_cdm_.get(),
OnSetServerCertificate(_, _)).Times(0);
If you use a StrongMock, you don't need Times(0) because uninteresting call
would also fail the test.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:188:
WithArg<1>(ResolvePromise(remote_cdm_.get()))));
nit: Can we try not resolving the promise here? I guess it's important that even
if the remote CDM resolves the promise, MojoCdm will still get a rejected
promise. But it's equally or more important that even if the remote CDM does
nothing, e.g. the remote CDM process crashed, MojoCdm will also get a rejected
promise.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:242: }
There are a lot of duplication on this switch-case block between this and other
*AndExpect() helper functions. Is it possible to do something like:

if (expected_result == CONNECTION_ERROR_BEFORE) {
  ForceConnectionError();
} else {
  EXPECT_CALL(*remote_cdm_.get(), OnCreateSessionAndGenerateRequest(
                                             session_type, data_type, key_id,
_))
      .WillOnce(WithArg<3>(ProcessPromise(..., expected_result)));
}

Then in ProcessPromise, we can check |expected_result| and decide whether to
resolve or reject the promise, or trigger a connection error.

[ddorwin, 2017-01-05 18:21:30]

Thanks for adding these tests. Please update the first line of the description
to make it clear that this is testing the breaking of the connection.

[jrummell, 2017-01-05 23:43:45]

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

[commit-bot: I haz the power, 2017-01-05 23:44:28]

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

[jrummell, 2017-01-05 23:45:56]

Description was changed from

==========
[eme] Break mojo connection during call

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass
==========

to

==========
Test breaking of mojo connection during call

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass
==========

[jrummell, 2017-01-05 23:46:40]

Description was changed from

==========
Test breaking of mojo connection during call

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass
==========

to

==========
Test breaking the mojo connection to MojoCdm

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass
==========

[jrummell, 2017-01-05 23:47:22]

Updated.

https://codereview.chromium.org/2592913002/diff/20001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2592913002/diff/20001/media/base/mock_filters...
media/base/mock_filters.h:473: uint32_t promise_id));
On 2017/01/05 06:59:53, xhwang wrote:
> Instead of using the promise adapter to convert the promise to an ID, I think
we
> could also do something like 
> 
> MOCK_METHOD4(OnCreateSessionAndGenerateRequest,
>                                 ....
>                                 std::unique_ptr<NewSessionCdmPromise>&
promise);
> 
> Note that the promise is passed as a reference. I didn't try this but if it
can
> work it'll be simpler, as we don't need to keep the id around and ask the
> MockCdm to resolve/reject the promise, we can just resolve/reject it directly.
> 

Done.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_cdm_unittest.cc (right):

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:92: remote_cdm_ = new
NiceMock<MockCdm>();
On 2017/01/05 06:59:53, xhwang wrote:
> Can we use StrictMock?

Yes, but then we have to ignore calls to GetCdmContext(). Done.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:101: WithArg<7>(CdmCreated(remote_cdm_,
""))));
On 2017/01/05 06:59:53, xhwang wrote:
> This is pretty cool but seems a bit overkill. For the foreseeable future,
> MockCdmFactory will only create MockCdm. Does it make sense to just have a
> concrete MockCdmFactory implementation which will either return a MockCdm, or
> nullptr, based on a flag (similar to how we construct MockCdmPromise)? Then
this
> code and MockCdm impl will be a bit simpler.

Done.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:179: EXPECT_CALL(*remote_cdm_.get(),
OnSetServerCertificate(_, _)).Times(0);
On 2017/01/05 06:59:53, xhwang wrote:
> If you use a StrongMock, you don't need Times(0) because uninteresting call
> would also fail the test.

Done.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:188:
WithArg<1>(ResolvePromise(remote_cdm_.get()))));
On 2017/01/05 06:59:53, xhwang wrote:
> nit: Can we try not resolving the promise here? I guess it's important that
even
> if the remote CDM resolves the promise, MojoCdm will still get a rejected
> promise. But it's equally or more important that even if the remote CDM does
> nothing, e.g. the remote CDM process crashed, MojoCdm will also get a rejected
> promise.

Rejecting or resolving the promise doesn't get passed back to MojoCdm. This is
simply done so that the promise doesn't complain that it's never fulfilled.

https://codereview.chromium.org/2592913002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:242: }
On 2017/01/05 06:59:53, xhwang wrote:
> There are a lot of duplication on this switch-case block between this and
other
> *AndExpect() helper functions. Is it possible to do something like:
> 
> if (expected_result == CONNECTION_ERROR_BEFORE) {
>   ForceConnectionError();
> } else {
>   EXPECT_CALL(*remote_cdm_.get(), OnCreateSessionAndGenerateRequest(
>                                              session_type, data_type, key_id,
> _))
>       .WillOnce(WithArg<3>(ProcessPromise(..., expected_result)));
> }
> 
> Then in ProcessPromise, we can check |expected_result| and decide whether to
> resolve or reject the promise, or trigger a connection error.

ProcessPromise is a bit of work, but done.

[commit-bot: I haz the power, 2017-01-06 01:28:46]

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

[commit-bot: I haz the power, 2017-01-06 01:28:47]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-01-06 06:21:36]

looking much cleaner! just a few more ideas for discussion.

https://codereview.chromium.org/2592913002/diff/40001/media/base/mock_filters.cc
File media/base/mock_filters.cc (right):

https://codereview.chromium.org/2592913002/diff/40001/media/base/mock_filters...
media/base/mock_filters.cc:282: session_keys_change_cb,
session_expiration_update_cb);
Can we create the CDM within this factory so that we can pass these callbacks in
MockCdm's ctor? Then we don't need SetCallbacks().

Then, instead of having to pass in a CDM in Initialize(), we can provide a get()
function to get a raw pointer or a  ref to the created CDM, so the test can set
expectations on it.

https://codereview.chromium.org/2592913002/diff/40001/media/base/mock_filters...
media/base/mock_filters.cc:289: before_creation_cb_ = before_creation_cb;
If we create the CDM within this factory, we don't need pass in the CDM here. So
this function is more like set_before_creation_cb() instead of Initialize().

[jrummell, 2017-01-06 18:58:11]

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

[commit-bot: I haz the power, 2017-01-06 18:58:44]

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

[jrummell, 2017-01-06 18:59:28]

Factory changed.

https://codereview.chromium.org/2592913002/diff/40001/media/base/mock_filters.cc
File media/base/mock_filters.cc (right):

https://codereview.chromium.org/2592913002/diff/40001/media/base/mock_filters...
media/base/mock_filters.cc:282: session_keys_change_cb,
session_expiration_update_cb);
On 2017/01/06 06:21:36, xhwang wrote:
> Can we create the CDM within this factory so that we can pass these callbacks
in
> MockCdm's ctor? Then we don't need SetCallbacks().
> 
> Then, instead of having to pass in a CDM in Initialize(), we can provide a
get()
> function to get a raw pointer or a  ref to the created CDM, so the test can
set
> expectations on it.

Done.

https://codereview.chromium.org/2592913002/diff/40001/media/base/mock_filters...
media/base/mock_filters.cc:289: before_creation_cb_ = before_creation_cb;
On 2017/01/06 06:21:36, xhwang wrote:
> If we create the CDM within this factory, we don't need pass in the CDM here.
So
> this function is more like set_before_creation_cb() instead of Initialize().

Done.

[commit-bot: I haz the power, 2017-01-06 20:29:22]

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

[commit-bot: I haz the power, 2017-01-06 20:29:22]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-01-06 21:00:21]

Thanks! LGTM with nits

https://codereview.chromium.org/2592913002/diff/60001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2592913002/diff/60001/media/base/mock_filters...
media/base/mock_filters.h:551: MockCdm* Cdm();
nit: Functions typically start with a verb. How about GetCdm() or
GetCreatedCdm()?

https://codereview.chromium.org/2592913002/diff/60001/media/base/mock_filters...
media/base/mock_filters.h:558: MockCdm* created_cdm_;
nit: It's probably safer to keep a ref to it to avoid lifetime issues. But it's
up to you.

[jrummell, 2017-01-06 22:49:16]

Thansk for the reviews.

https://codereview.chromium.org/2592913002/diff/60001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2592913002/diff/60001/media/base/mock_filters...
media/base/mock_filters.h:551: MockCdm* Cdm();
On 2017/01/06 21:00:21, xhwang wrote:
> nit: Functions typically start with a verb. How about GetCdm() or
> GetCreatedCdm()?

Done (GetCreatedCdm()).

https://codereview.chromium.org/2592913002/diff/60001/media/base/mock_filters...
media/base/mock_filters.h:558: MockCdm* created_cdm_;
On 2017/01/06 21:00:21, xhwang wrote:
> nit: It's probably safer to keep a ref to it to avoid lifetime issues. But
it's
> up to you.

Done.

[jrummell, 2017-01-06 22:49:21]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2017-01-06 22:49:21]

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/2592913002/#ps80001
(title: "remaining nits")

[commit-bot: I haz the power, 2017-01-06 22:49:48]

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-01-07 00:15:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483742961034630,
"parent_rev": "51b59b3aa445abf8ed7d378d565136b602d1726b", "commit_rev":
"0d5d318987dde8b60ca474976128c5543b2e2563"}

[commit-bot: I haz the power, 2017-01-07 00:15:54]

Description was changed from

==========
Test breaking the mojo connection to MojoCdm

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass
==========

to

==========
Test breaking the mojo connection to MojoCdm

Improve the testing on MojoCdm by adding a MockCdm. This allows for
testing that:
- resolving the promise works
- rejecting the promise works
- closing the connection before a call works
- closing the connection while executing the call works.

BUG=671362
TEST=new tests pass

Review-Url: https://codereview.chromium.org/2592913002
Cr-Commit-Position: refs/heads/master@{#442104}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d5d318987dde8b60ca474976128...
==========

[commit-bot: I haz the power, 2017-01-07 00:15:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0d5d318987dde8b60ca474976128...