media: Avoid access violation in MojoCdm after connection error

media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

Added a unittest to cover the case where CDM creation failed due to
connection error.

BUG=652856, 657069
TEST=Manually tested. Also added a new unittest.

Committed: https://crrev.com/1f3429ba51d768e7461bb702afb74b234efb7337
Cr-Commit-Position: refs/heads/master@{#436857}

[xhwang, 2016-12-05 22:08:32]

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

[xhwang, 2016-12-05 22:08:55]

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

[xhwang, 2016-12-05 22:08:55]

PTAL

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

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

[jrummell, 2016-12-05 23:25:48]

lgtm

[commit-bot: I haz the power, 2016-12-06 00:08:24]

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

[commit-bot: I haz the power, 2016-12-06 00:08:24]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-12-06 20:44:34]

I added a unittest with which I can repro the crash without the fix in this CL.
Please take a look again at the test code.

[xhwang, 2016-12-06 20:54:15]

Description was changed from

==========
media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

BUG=652856
TEST=Manually tested.
==========

to

==========
media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

Added a unittest to cover the case where CDM creation failed due to
connection error.

BUG=652856,657069
TEST=Manually tested. Also added a new unittest.
==========

[jrummell, 2016-12-06 21:07:07]

Test looks good. Still lgtm w/nits.

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

https://codereview.chromium.org/2551333002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:80: const std::string& error_message) {
Maybe pass expected_result in the Bind() above, and put
EXPECT_EQ(expected_result, CONNECTION_ERROR); (or SUCCESS) as appropriate in
this function.

https://codereview.chromium.org/2551333002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:82: LOG(ERROR) << error_message;
I think it would be better if this simply did
EXPECT_FALSE(error_message.empty()). No need to log the message.

https://codereview.chromium.org/2551333002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:95: // TODO(jrummell): Use a
MockCdmFactory to create a MockCdm here to more test
s/to more/for more/

[xhwang, 2016-12-06 21:33:43]

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

https://codereview.chromium.org/2551333002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:80: const std::string& error_message) {
On 2016/12/06 21:07:07, jrummell wrote:
> Maybe pass expected_result in the Bind() above, and put
> EXPECT_EQ(expected_result, CONNECTION_ERROR); (or SUCCESS) as appropriate in
> this function.

In the future we may have other types of failures, so the only thing we can
reliably check is

if (cdm) {
  EXPECT_EQ(expected_result, SUCCESS);
}

https://codereview.chromium.org/2551333002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:82: LOG(ERROR) << error_message;
On 2016/12/06 21:07:07, jrummell wrote:
> I think it would be better if this simply did
> EXPECT_FALSE(error_message.empty()). No need to log the message.

We don't "force" all impls to provide a non-empty message upon failure.  
Updating this to DVLOG() to be less noisy.

https://codereview.chromium.org/2551333002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm_unittest.cc:95: // TODO(jrummell): Use a
MockCdmFactory to create a MockCdm here to more test
On 2016/12/06 21:07:07, jrummell wrote:
> s/to more/for more/

Done.

[xhwang, 2016-12-06 21:33:57]

comments addressed

[xhwang, 2016-12-07 00:58:23]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-12-07 00:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org
Link to the patchset: https://codereview.chromium.org/2551333002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-12-07 00:59: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-12-07 04:16:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481072303083120,
"parent_rev": "bcaf7d826df9938910d429c6862fcc3393808358", "commit_rev":
"0089d77cb7828edaf21bf36d938a69520b501778"}

[commit-bot: I haz the power, 2016-12-07 04:16:49]

Description was changed from

==========
media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

Added a unittest to cover the case where CDM creation failed due to
connection error.

BUG=652856,657069
TEST=Manually tested. Also added a new unittest.
==========

to

==========
media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

Added a unittest to cover the case where CDM creation failed due to
connection error.

BUG=652856,657069
TEST=Manually tested. Also added a new unittest.
==========

[commit-bot: I haz the power, 2016-12-07 04:16:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-07 04:19:19]

Description was changed from

==========
media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

Added a unittest to cover the case where CDM creation failed due to
connection error.

BUG=652856,657069
TEST=Manually tested. Also added a new unittest.
==========

to

==========
media: Avoid access violation in MojoCdm after connection error

Upon connection error during CDM initialization, after we rejected and
dropped the promise, MojoCdm could be destructed. We should not try to
close pending sessions in this case, and in fact we shouldn't have any
sessions open before the CDM is successfully initialized.

Added a unittest to cover the case where CDM creation failed due to
connection error.

BUG=652856,657069
TEST=Manually tested. Also added a new unittest.

Committed: https://crrev.com/1f3429ba51d768e7461bb702afb74b234efb7337
Cr-Commit-Position: refs/heads/master@{#436857}
==========

[commit-bot: I haz the power, 2016-12-07 04:19:19]

Patchset 3 (id:??) landed as
https://crrev.com/1f3429ba51d768e7461bb702afb74b234efb7337
Cr-Commit-Position: refs/heads/master@{#436857}