media: Only unregister a CDM when it has been registered.

media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.

Committed: https://crrev.com/0435065730586ac4b546942a62e0d5bfb014bfe3
Cr-Commit-Position: refs/heads/master@{#370786}

[xhwang, 2016-01-20 18:56:45]

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

[xhwang, 2016-01-20 18:56:45]

PTAL

rockot: How can I test the new EDK?

[Ken Rockot(use gerrit already), 2016-01-20 19:02:23]

I wouldn't try if I were you :)  It's not landed so you'd be patching it in from
a large CL that's still churning.

AFAICT on ToT you can run this locally with the current EDK and the test will
CHECK but still pass for some reason. That's probably good enough to test?

[Ken Rockot(use gerrit already), 2016-01-20 19:03:00]

On 2016/01/20 at 19:02:23, Ken Rockot wrote:
> I wouldn't try if I were you :)  It's not landed so you'd be patching it in
from a large CL that's still churning.
> 
> AFAICT on ToT you can run this locally with the current EDK and the test will
CHECK but still pass for some reason. That's probably good enough to test?

Also this change seems to pretty clearly address the issue

[xhwang, 2016-01-21 06:32:14]

Description was changed from

==========
media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.
==========

to

==========
media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.
==========

[xhwang, 2016-01-21 06:32:14]

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

[xhwang, 2016-01-21 06:33:50]

On 2016/01/20 19:03:00, Ken Rockot wrote:
> On 2016/01/20 at 19:02:23, Ken Rockot wrote:
> > I wouldn't try if I were you :)  It's not landed so you'd be patching it in
> from a large CL that's still churning.
> > 
> > AFAICT on ToT you can run this locally with the current EDK and the test
will
> CHECK but still pass for some reason. That's probably good enough to test?
> 
> Also this change seems to pretty clearly address the issue

Ken: Thanks for the suggestion! That worked. And it's confirmed that the DCHECK
is fixed by this CL.

msw: Any idea on why the test is passing even though a DCHECK is fired?

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from MediaAppTest
[ RUN      ] MediaAppTest.Lifetime
[1155:1155:0120/223000:FATAL:mojo_cdm_service.cc(53)] Check failed:
cdm_map_.count(cdm_id).
#0 0x7f844df43a9e base::debug::StackTrace::StackTrace()
#1 0x7f844df9943f logging::LogMessage::~LogMessage()
#2 0x7f844ccdff34 media::(anonymous namespace)::CdmManager::UnregisterCdm()
#3 0x7f844ccdfd43 media::MojoCdmService::~MojoCdmService()
#4 0x7f844ccdffa9 media::MojoCdmService::~MojoCdmService()
#5 0x7f844cce71d1 mojo::StrongBinding<>::OnConnectionError()
#6 0x7f844cce7188
_ZZN4mojo13StrongBindingIN5media10interfaces23ContentDecryptionModuleEE4BindENS_16InterfaceRequestIS3_EEPK15MojoAsyncWaiterENKUlvE_clEv
#7 0x7f844cce7169
_ZNK4mojo8CallbackIFvvEE14FunctorAdapterIZNS_13StrongBindingIN5media10interfaces23ContentDecryptionModuleEE4BindENS_16InterfaceRequestIS7_EEPK15MojoAsyncWaiterEUlvE_E3RunEv
#8 0x7f844ccd5b7d mojo::Callback<>::Run()
#9 0x7f844ccd5b2c
_ZZN4mojo8internal12BindingStateIN5media10interfaces14ServiceFactoryELb0EE4BindENS_16ScopedHandleBaseINS_17MessagePipeHandleEEEPK15MojoAsyncWaiterENKUlvE_clEv
#10 0x7f844ccd5b09
_ZNK4mojo8CallbackIFvvEE14FunctorAdapterIZNS_8internal12BindingStateIN5media10interfaces14ServiceFactoryELb0EE4BindENS_16ScopedHandleBaseINS_17MessagePipeHandleEEEPK15MojoAsyncWaiterEUlvE_E3RunEv
#11 0x7f844ccd5b7d mojo::Callback<>::Run()
#12 0x7f844de50aa7 mojo::internal::Connector::HandleError()
#13 0x7f844de5186d mojo::internal::Connector::OnHandleReady()
#14 0x7f844de516a3 mojo::internal::Connector::CallOnHandleReady()
#15 0x7f844dea5c48 mojo::internal::(anonymous namespace)::OnHandleReady()
#16 0x7f844dea5fe7 base::internal::RunnableAdapter<>::Run()
#17 0x7f844dea5f64
_ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIPFvPN4mojo6common13HandleWatcherEPFvPvjES7_jEEENS0_8TypeListIJRKS6_RKS9_RKS7_RKjEEEE8MakeItSoESC_SF_SH_SJ_SL_
.........................
#46 0x000000488bee base::Callback<>::Run()
#47 0x000000a3ac69 mojo::runner::(anonymous namespace)::Blocker::Block()
#48 0x000000a399c4 mojo::runner::ChildProcessMain()
#49 0x00000049aec2 mojo::runner::RunnerMain()
#50 0x00000048400c main
#51 0x7f845139aec5 __libc_start_main
#52 0x000000483ee5 <unknown>
  r8: 00000000050f62e5  r9: 0000000000000000 r10: 0000000000000008 r11:
0000000000000202
 r12: 0000000000483ebc r13: 00007ffe6fd97d40 r14: 0000000000000000 r15:
0000000000000000
  di: 0000000000000483  si: 0000000000000483  bp: 00007ffe6fd939f0  bx:
0000000000000000
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp:
00007ffe6fd938b8
  ip: 00007f84513afcb7 efl: 0000000000000202 cgf: 0000000000000033 erf:
0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
[       OK ] MediaAppTest.Lifetime (398 ms)
[----------] 1 test from MediaAppTest (398 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (398 ms total)
[  PASSED  ] 1 test.

[xhwang, 2016-01-21 06:34:31]

jrummell: PTAL

[Ken Rockot(use gerrit already), 2016-01-21 06:39:20]

I think the reason it passes is that the crash is in the media app, not in
the test runner. The test runner doesn't actually have any expectation of
the media app not crashing. It's not clear to me how you'd set such an
expectation without modifying your interfaces, but that seems unfortunate.

Maybe the test runner needs to expose an API for checking that an arbitrary
application under test was exited cleanly.

On Wed, Jan 20, 2016 at 10:34 PM, <xhwang@chromium.org> wrote:

> jrummell: PTAL
>
> https://chromiumcodereview.appspot.com/1603413004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jrummell, 2016-01-21 19:58:05]

lgtm

[xhwang, 2016-01-21 20:56:39]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-01-21 20:56:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1603413004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1603413004/1

[commit-bot: I haz the power, 2016-01-21 21:17:18]

Description was changed from

==========
media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.
==========

to

==========
media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.
==========

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

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-21 21:18:25]

Description was changed from

==========
media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.
==========

to

==========
media: Only unregister a CDM when it has been registered.

A CDM is registered after it's succussfully created (so the CDM ID is valid).
Check the CDM ID to decide whether we need to unregister it.

Committed: https://crrev.com/0435065730586ac4b546942a62e0d5bfb014bfe3
Cr-Commit-Position: refs/heads/master@{#370786}
==========

[commit-bot: I haz the power, 2016-01-21 21:18:26]

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