Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561263002:
[eme] Reject CDM calls after connection error (Closed)
|
Created:
4 years ago by jrummell Modified:
4 years ago Reviewers:
xhwang CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[eme] Reject CDM calls after connection error
Once the mojo connection is broken, all subsequent calls to the CDM
should fail.
BUG=671362
TEST=new tests pass
Committed: https://crrev.com/8ef30a35f16fe89ee4b4f8d9ea15d5bbcf7dc9cf
Cr-Commit-Position: refs/heads/master@{#439032}
Patch Set 1 #
Total comments: 20
Patch Set 2 : changes #
Total comments: 10
Patch Set 3 : improved mock promises #
Total comments: 2
Patch Set 4 : nit (+rebase for MediaKeys rename) #
Messages
Total messages: 16 (6 generated)
jrummell@chromium.org changed reviewers: + xhwang@chromium.org
PTAL.
https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm.cc File media/mojo/clients/mojo_cdm.cc (right): https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm.cc:153: if (!remote_cdm_.is_bound()) { will this work? if (!remote_cdm_) { ... } https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm.cc:155: "CDM has failed."); This message will be received by JS app, so it'd be nice to have more details. How about: CDM connection lost https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm.h File media/mojo/clients/mojo_cdm.h (right): https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm.h:80: friend class MojoCdmTest; This is not needed, see below. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... File media/mojo/clients/mojo_cdm_unittest.cc (right): https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:27: MATCHER(IsNotEmpty, "") { s/IsNotEmpty/NotEmpty https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:71: FAIL(); Can you actually add a test to cover initialization failure? You can probably use an unsupported key system. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:95: EXPECT_EQ(CONNECTION_ERROR, expected_result); If you actually have a init failure test, this will not be true. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:113: } You can reset the cdm_binding_ to trigger a connection error. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:149: } This really is testing AesDecryptor, not MojoCdm... https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:192: const std::string& error_message)); We have this in multiple tests. Maybe we should have a MockCdmPromise in mock_filters.h. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:252: } If you add a MockCdm first and use it in this test instead of the AesDecryptor, it'll be more convenient to add more tests (e.g. update).
Updated. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm.cc File media/mojo/clients/mojo_cdm.cc (right): https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm.cc:153: if (!remote_cdm_.is_bound()) { On 2016/12/09 06:13:30, xhwang wrote: > will this work? > > if (!remote_cdm_) { > ... > } Done. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm.cc:155: "CDM has failed."); On 2016/12/09 06:13:30, xhwang wrote: > This message will be received by JS app, so it'd be nice to have more details. > How about: > > CDM connection lost Done. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm.h File media/mojo/clients/mojo_cdm.h (right): https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm.h:80: friend class MojoCdmTest; On 2016/12/09 06:13:30, xhwang wrote: > This is not needed, see below. Done. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... File media/mojo/clients/mojo_cdm_unittest.cc (right): https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:27: MATCHER(IsNotEmpty, "") { On 2016/12/09 06:13:30, xhwang wrote: > s/IsNotEmpty/NotEmpty Done. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:71: FAIL(); On 2016/12/09 06:13:30, xhwang wrote: > Can you actually add a test to cover initialization failure? You can probably > use an unsupported key system. Done. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:95: EXPECT_EQ(CONNECTION_ERROR, expected_result); On 2016/12/09 06:13:30, xhwang wrote: > If you actually have a init failure test, this will not be true. Fixed. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:113: } On 2016/12/09 06:13:30, xhwang wrote: > You can reset the cdm_binding_ to trigger a connection error. Done. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:149: } On 2016/12/09 06:13:30, xhwang wrote: > This really is testing AesDecryptor, not MojoCdm... In the failure case MojoCdm will fail the promise. But you're right about the success case. I only added a success test to make sure the code actually worked if the CDM was connected. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:192: const std::string& error_message)); On 2016/12/09 06:13:30, xhwang wrote: > We have this in multiple tests. Maybe we should have a MockCdmPromise in > mock_filters.h. Done. I'll update the other places in another CL. https://codereview.chromium.org/2561263002/diff/1/media/mojo/clients/mojo_cdm... media/mojo/clients/mojo_cdm_unittest.cc:252: } On 2016/12/09 06:13:30, xhwang wrote: > If you add a MockCdm first and use it in this test instead of the AesDecryptor, > it'll be more convenient to add more tests (e.g. update). Agreed. Later :)
This looks much better. I just have a few more comments. https://codereview.chromium.org/2561263002/diff/20001/media/base/mock_filters.h File media/base/mock_filters.h (right): https://codereview.chromium.org/2561263002/diff/20001/media/base/mock_filters... media/base/mock_filters.h:441: class MockCdmPromise : public SimpleCdmPromise { nit: please move these closer to other CDM related mocks, e.g. around line 417. https://codereview.chromium.org/2561263002/diff/20001/media/base/mock_filters... media/base/mock_filters.h:445: MockCdmPromise(bool expect_success); explicit https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm.cc (right): https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm.cc:138: cdm_promise_adapter_.Clear(); Thanks, I think you already fixed the second problem in the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=671362#c2 However, there's no test coverage on that case. Also adding test for that would need the MockCdm. For completeness, could you please remove the CdmPromiseAdapter related changes from this CL, and add it in another CL with proper test coverage? Thanks! https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_unittest.cc (right): https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_unittest.cc:70: case FAILURE: I think this is equivalent to CONNECTION_ERROR. I thought "FAILURE" means that the connection is setup, but the CDM running in MojoCdmService would fail to initialize. https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_unittest.cc:184: Initialize("org.random.cdm", FAILURE); See comment above. I think this is failing because of the connection error and has nothing to do with the invalid key system string.
Updated. https://codereview.chromium.org/2561263002/diff/20001/media/base/mock_filters.h File media/base/mock_filters.h (right): https://codereview.chromium.org/2561263002/diff/20001/media/base/mock_filters... media/base/mock_filters.h:441: class MockCdmPromise : public SimpleCdmPromise { On 2016/12/14 18:43:54, xhwang wrote: > nit: please move these closer to other CDM related mocks, e.g. around line 417. Done. https://codereview.chromium.org/2561263002/diff/20001/media/base/mock_filters... media/base/mock_filters.h:445: MockCdmPromise(bool expect_success); On 2016/12/14 18:43:54, xhwang wrote: > explicit Done. https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm.cc (right): https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm.cc:138: cdm_promise_adapter_.Clear(); On 2016/12/14 18:43:54, xhwang wrote: > Thanks, I think you already fixed the second problem in the bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=671362#c2 > > However, there's no test coverage on that case. Also adding test for that would > need the MockCdm. For completeness, could you please remove the > CdmPromiseAdapter related changes from this CL, and add it in another CL with > proper test coverage? Thanks! Done. https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_unittest.cc (right): https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_unittest.cc:70: case FAILURE: On 2016/12/14 18:43:54, xhwang wrote: > I think this is equivalent to CONNECTION_ERROR. I thought "FAILURE" means that > the connection is setup, but the CDM running in MojoCdmService would fail to > initialize. Fixed. Now I get "ERROR: org.random.cdm is not a known system" from key_systems.cc. https://codereview.chromium.org/2561263002/diff/20001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_unittest.cc:184: Initialize("org.random.cdm", FAILURE); On 2016/12/14 18:43:54, xhwang wrote: > See comment above. I think this is failing because of the connection error and > has nothing to do with the invalid key system string. Fixed.
Thanks! LGTM % nit https://codereview.chromium.org/2561263002/diff/40001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_unittest.cc (right): https://codereview.chromium.org/2561263002/diff/40001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_unittest.cc:182: Initialize("org.random.cdm", FAILURE); Please add a comment that this is failing because DefaultCdmFactory doesn't support this.
The CQ bit was checked by jrummell@chromium.org
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/2561263002/#ps60001 (title: "nit (+rebase for MediaKeys rename)")
Updated. https://codereview.chromium.org/2561263002/diff/40001/media/mojo/clients/mojo... File media/mojo/clients/mojo_cdm_unittest.cc (right): https://codereview.chromium.org/2561263002/diff/40001/media/mojo/clients/mojo... media/mojo/clients/mojo_cdm_unittest.cc:182: Initialize("org.random.cdm", FAILURE); On 2016/12/16 01:21:13, xhwang wrote: > Please add a comment that this is failing because DefaultCdmFactory doesn't > support this. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481855526639090,
"parent_rev": "c296e2b96395d1596a9e05af5fd9e6c6456f1c88", "commit_rev":
"7a353cdeadb230659c2bc0902ca4065b82377faf"}
Message was sent while issue was closed.
Description was changed from ========== [eme] Reject CDM calls after connection error Once the mojo connection is broken, all subsequent calls to the CDM should fail. BUG=671362 TEST=new tests pass ========== to ========== [eme] Reject CDM calls after connection error Once the mojo connection is broken, all subsequent calls to the CDM should fail. BUG=671362 TEST=new tests pass Review-Url: https://codereview.chromium.org/2561263002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [eme] Reject CDM calls after connection error Once the mojo connection is broken, all subsequent calls to the CDM should fail. BUG=671362 TEST=new tests pass Review-Url: https://codereview.chromium.org/2561263002 ========== to ========== [eme] Reject CDM calls after connection error Once the mojo connection is broken, all subsequent calls to the CDM should fail. BUG=671362 TEST=new tests pass Committed: https://crrev.com/8ef30a35f16fe89ee4b4f8d9ea15d5bbcf7dc9cf Cr-Commit-Position: refs/heads/master@{#439032} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8ef30a35f16fe89ee4b4f8d9ea15d5bbcf7dc9cf Cr-Commit-Position: refs/heads/master@{#439032} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
