MojoCdmService can be destructed before MojoCdm during normal teardown |
||||
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.
,
Mar 13 2018
,
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
,
Apr 14 2018
CdmService is fixed. I'll work on a similar fix on MediaService.
,
Apr 14 2018
,
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
,
May 3 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by xhw...@chromium.org
, Mar 12 2018