New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 821171 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

MojoCdmService can be destructed before MojoCdm during normal teardown

Project Member Reported by xhw...@chromium.org, Mar 12 2018

Issue description

(The following description is for the Android model only. The model on desktop is similar and is subject to the same problem.)

The renderer process hosts a MojoCdm, which owns the media::mojom::ContentDecryptionModulePtr (CdmPtr). The MojoCdm is owned by the Javascript MediaKeys object.

In the remote process, MojoCdmService implements media::mojom::CDM, and is hosted in MediaService in the GPU process.

The MediaService owns a mojo::StrongBindingSet of media::mojom::InterfaceFactory implementations, which is media::InterfaceFactoryImpl. The corresponding media::mojom::InterfaceFactoryPtr is owned by the RenderFrameHostImpl in the browser process (see MediaInterfaceProxy). The renderer process needs the help of the browser to setup the connection.

InterfaceFactoryImpl owns a mojo::StrongBindingSet of media::mojom::ContentDecryptionModule, talking to the MojoCdm running in the render process.

Now, during teardown, before JS MediaKeys is destroyed or cleaned up. It's possible that RenderFrameHostImpl in the browser process is killed first. Then InterfaceFactoryImpl get a connection error and is destroyed, which will then destroy all MojoCdmService instances, causing a connection error on the MojoCdm, which could trigger some error conditions, e.g. MediaKeySession closed.

User will probably not notice this since this is during teardown. But Javascript may see it, and we see from UMA metrics. This is annoying since it'll contaminate metrics both for the JS app and Chrome. For example, the JS app cannot differentiate whether the closed MediaKeySession is a fatal error, or just work as expected during page teardown process.

Similar issue could also happen to other remote media components, e.g. audio/video decoders, causing decode errors during teardown.

Historically in the IPC world, if RenderFrameHostImpl is destroyed first, IPC messages sent from the renderer process simply will not be replied. This will not cause any error. With mojo, since we have connection error, and WrapCallbackWithDefaultInvokeIfNotRun, the callbacks will be run with failure conditions, and we could see this pattern more often.

ISTM the ultimate fix is to tighten the life time between RenderFrameImpl and RenderFrameHostImpl. For example, all JS objects (ActiveDomObjects) tied to the frame needs to be stopped before RenderFrameHostImpl destruction happens. However, I chat with dcheng@ offline on this, and this seems will not happen any time soon, or forever. 

Another potential fix is to not destroy InterfaceFactoryImpl when RenderFrameHostImpl is destroyed. This means that we cannot store InterfaceFactoryImpl in the mojo::StrongBindingSet as is, and need some additional logic so that after all MediaCdmServices are destructed, we have a way to destruct InterfaceFactoryImpl. I'll have to think more about it.
 

Comment 1 by xhw...@chromium.org, Mar 12 2018

Components: Internals>Media>Mojo Internals>Media>Encrypted

Comment 2 by xhw...@chromium.org, Mar 13 2018

Labels: -Pri-2 Pri-1
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71511b2c7decc205d99c471f79458300520debbd

commit 71511b2c7decc205d99c471f79458300520debbd
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Fri Apr 06 07:36:22 2018

media: Add DeferredDestroyStrongBindingSet

This CL adds DeferredDestroyStrongBindingSet such that mojom::Foo impl
will only be destroyed when:
- mojom::Foo connection error happens, AND FooImpl calls the
  DestroyCallback, or
- DeferredDestroyStrongBindingSet is destroyed.

This helps solve tricky lifetime issues where we do not want destroy the
implementation immediately.

For example, currently when connection error on mojom::CdmFactory
happens, CdmFactoryImpl will be destroyed, which will also destroy all
CDMs owned by CdmFactoryImpl. However, this could happen before MojoCdm
is stopped in the render process, causing unexpected behavior. See the
bug for more details.

This CL also fixes the CdmFactoryImpl lifetime issue by using the
DeferredDestroyStrongBindingSet, and let CdmFactoryImpl only call the
DestroyCallback when no outstanding CDMs exist.

For historical context, mojo::StrongBindingSet was originally added
also to solve lifetime issues around media::mojom::InterfaceFactory, the
predecessor of media::mojom::CdmFactory.
See https://codereview.chromium.org/2530613003/.

Bug:  821171 
Test: Added new BindingSetTest cases.
Change-Id: I24a660e8b62720f7fd772db1bafb9499cb71428c
Reviewed-on: https://chromium-review.googlesource.com/969655
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548704}
[modify] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/media/mojo/services/BUILD.gn
[modify] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/media/mojo/services/cdm_service.cc
[modify] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/media/mojo/services/cdm_service.h
[modify] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/media/mojo/services/cdm_service_unittest.cc
[add] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/media/mojo/services/deferred_destroy_strong_binding_set.h
[add] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/media/mojo/services/deferred_destroy_strong_binding_set_unittest.cc
[modify] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/mojo/public/cpp/bindings/strong_binding_set.h
[modify] https://crrev.com/71511b2c7decc205d99c471f79458300520debbd/mojo/public/cpp/bindings/unique_ptr_impl_ref_traits.h

Comment 4 by xhw...@chromium.org, Apr 14 2018

CdmService is fixed. I'll work on a similar fix on MediaService.

Comment 5 by xhw...@chromium.org, Apr 14 2018

Labels: FoundIn-64
Project Member

Comment 6 by bugdroid1@chromium.org, May 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23aeaa7aac3c8220908006fb569994627e88d914

commit 23aeaa7aac3c8220908006fb569994627e88d914
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Thu May 03 09:57:14 2018

media: Use DeferredDestroyStrongBindingSet in MediaService

This will prevent decoding error caused by destroyed MediaService during
page navigation. See Bug for more details.

Bug:  821171 
Change-Id: I515fe3731ced2c38c6ccbb364d05a6122c9a4461
Reviewed-on: https://chromium-review.googlesource.com/1018821
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555685}
[modify] https://crrev.com/23aeaa7aac3c8220908006fb569994627e88d914/media/mojo/services/interface_factory_impl.cc
[modify] https://crrev.com/23aeaa7aac3c8220908006fb569994627e88d914/media/mojo/services/interface_factory_impl.h
[modify] https://crrev.com/23aeaa7aac3c8220908006fb569994627e88d914/media/mojo/services/media_service.h
[modify] https://crrev.com/23aeaa7aac3c8220908006fb569994627e88d914/media/mojo/services/media_service_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment