Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530613003:
media: Fix lifetime of InterfaceFactoryImpl and created impls (Closed)
|
Created:
4 years ago by xhwang Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, alokp, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, qsr+mojo_chromium.org, tguilbert, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Fix lifetime of InterfaceFactoryImpl and created impls
Makes sure that when InterfaceFactoryImpl is destructed, all mojo
interface implemenations (e.g. MojoAudioDecoderService) are destructed.
To achieve this, a helper class StrongBindingSet is introduced such
that:
- When connection error happens, the entry will be removed from the
binding set, and the bound impl object will be destructed.
- When StrongBindingSet is destructed, it'll destruct all outstanding
impl objects in the binding set.
BUG=604912
TEST=Added a unittest for StrongBindingSet.
Committed: https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7
Cr-Commit-Position: refs/heads/master@{#435141}
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments addressed #
Total comments: 2
Messages
Total messages: 32 (18 generated)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xhwang@chromium.org changed reviewers: + rockot@chromium.org, sandersd@chromium.org
This is not ready to commit. I need to add some unittests to StrongBindingSet and figure out how to solve the problem related MojoRendererService. But I'd like to get some early high level opinion to make sure I am on the right path. PTAL! sandersd: This should fix the bug you filed. Please check whether this solution looks good to you. rockot: Please help quickly check StrongBindingSet to see whether this is what you expected :) tguilbert: Please see the question about MojoRendererService. alokp: This change will make sure we destruct all mojo media services (e.g. MojoRendererService) when we destruct the MediaService (together with TestMojoMediaClient). I tried it locally and it worked around the DCHECK around AudioManager destruction. https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... media/mojo/services/mojo_renderer_service.cc:276: binding_->Close(); tguilbert: Today the |binding_| is a StrongBindingPtr which means that MojoRendererService can only be bound with StrongBinding. I want to change MojoRendererService so it can be used with both mojo::Binding or mojo::StrongBinding (see the rest of this CL). The check here would "work", but I feel it totally defies the original purpose of this code. After this change, only when using MediaPlayerRenderer would we set the |binding_| :) To fix it, what do you think of returning an error to signal "failure" so that the client side can drive the destruction process? https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... media/mojo/services/mojo_renderer_service.h:81: public: This is a hack, I'll fix it. https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... File media/mojo/services/strong_binding_set.h (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... media/mojo/services/strong_binding_set.h:26: class StrongBindingSet { rockot: Following our offline discussion. Please take a high level look to see whether this makes sense :) I'll add unittests if it does look okay.
Seems reasonable to me https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... File media/mojo/services/strong_binding_set.h (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... media/mojo/services/strong_binding_set.h:53: base::Bind(&Entry::OnConnectionError, base::Unretained(this))); nit, you could just bind directly to BindingSet::OnConnectionError with the ID here.
lgtm. I don't currently foresee needing MojoVideoDecoderService to outlive the factory, so the particular mechanism used isn't something I have a strong opinion about. On the plus side, it's straightforward to opt-out of this mechanism later if necessary.
tguilbert@chromium.org changed reviewers: + tguilbert@chromium.org
https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... media/mojo/services/mojo_renderer_service.cc:276: binding_->Close(); On 2016/11/23 22:27:09, xhwang wrote: > tguilbert: > > Today the |binding_| is a StrongBindingPtr which means that MojoRendererService > can only be bound with StrongBinding. I want to change MojoRendererService so it > can be used with both mojo::Binding or mojo::StrongBinding (see the rest of this > CL). > > The check here would "work", but I feel it totally defies the original purpose > of this code. After this change, only when using MediaPlayerRenderer would we > set the |binding_| :) > > To fix it, what do you think of returning an error to signal "failure" so that > the client side can drive the destruction process? I originally used a base::Optional<UnguessableToken>, and sent back a nullopt when this failed. However, at review time, I was asked about how likely this scenario was to happen; the conclusion was that only a misbehaving/compromised client would ever call InitiateScopedSurfaceRequest when |initiate_surface_request_cb_| is null. The guidance I received was then to assume that any unexpected call to this method was from a compromised client (and should be treated as a security issue). In that regards, I don't think that sending a failure flag to the client would accomplish much, since we couldn't trust a compromised client to destroy itself. I am not sure what would be the right thing to do here then. I guess, if we are using a strong binding, we close the binding (because it owns |this|, and it's the only way to safely delete |this|) and if we use a weak binding, we delete |this|. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
comments addressed
https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... media/mojo/services/mojo_renderer_service.cc:276: binding_->Close(); On 2016/11/23 23:45:06, tguilbert wrote: > On 2016/11/23 22:27:09, xhwang wrote: > > tguilbert: > > > > Today the |binding_| is a StrongBindingPtr which means that > MojoRendererService > > can only be bound with StrongBinding. I want to change MojoRendererService so > it > > can be used with both mojo::Binding or mojo::StrongBinding (see the rest of > this > > CL). > > > > The check here would "work", but I feel it totally defies the original purpose > > of this code. After this change, only when using MediaPlayerRenderer would we > > set the |binding_| :) > > > > To fix it, what do you think of returning an error to signal "failure" so that > > the client side can drive the destruction process? > > I originally used a base::Optional<UnguessableToken>, and sent back a nullopt > when this failed. However, at review time, I was asked about how likely this > scenario was to happen; the conclusion was that only a misbehaving/compromised > client would ever call InitiateScopedSurfaceRequest when > |initiate_surface_request_cb_| is null. The guidance I received was then to > assume that any unexpected call to this method was from a compromised client > (and should be treated as a security issue). > > In that regards, I don't think that sending a failure flag to the client would > accomplish much, since we couldn't trust a compromised client to destroy itself. > > I am not sure what would be the right thing to do here then. I guess, if we are > using a strong binding, we close the binding (because it owns |this|, and it's > the only way to safely delete |this|) and if we use a weak binding, we delete > |this|. > > WDYT? tguilbert: I have a solution to use a bad_message_cb instead of binding: https://chromiumcodereview.appspot.com/2539703002/ It works, but that makes me feel that we might have some layering/design issues. ISTM that InitiateScopedSurfaceRequest() doesn't belong to the mojom::Renderer interface. For example, it doesn't exist in the C++ media::Renderer interface. It also look very special that in MojoRendererService we don't call into the |renderer_| to do the work, but will call a callback. MediaPlayerRenderer is a Renderer, but it also has some additional functionality, for example, to handle this surface request. In C++ layer, this is done by binding additional features in the constructor, but in mojo, we don't have this ability. One idea I have is to introduce an additional mojom interface, e.g. interface MediaPlayerRenderer { InitiateScopedSurfaceRequest() => ... GetRenderer(Renderer& renderer); } Then at RenderFrameHostImpl, we register a MediaPlayerRenderer service, and connect to it from MediaPlayerRendererClient. At the service side, we need a simple impl of mojom::MediaPlayerRenderer, which delegate all work to C++ media::MediaPlayerRenderer. In GetRenderer(), we can create a MojoRendererService to bind the RendererRequest. You can see similar model where we create a MojoDecryptorService in MojoCdmService: https://cs.chromium.org/chromium/src/media/mojo/services/mojo_cdm_service.cc?... I feel overall this will be a cleaner design and it will eliminate the bad message handling here. I am sad that this means we'll have some special code for MediaPlayerRenderer and it'll be different from other mojo based Renderers. In the future, if "requesting a surface" becomes a common pattern in media::Renderer, maybe we should add native support throughout the full Renderer stack. WDYT? https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... media/mojo/services/mojo_renderer_service.h:81: public: On 2016/11/23 22:27:09, xhwang wrote: > This is a hack, I'll fix it. Done. https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... File media/mojo/services/strong_binding_set.h (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... media/mojo/services/strong_binding_set.h:26: class StrongBindingSet { On 2016/11/23 22:27:09, xhwang wrote: > rockot: Following our offline discussion. Please take a high level look to see > whether this makes sense :) I'll add unittests if it does look okay. Test added. https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/strong_... media/mojo/services/strong_binding_set.h:53: base::Bind(&Entry::OnConnectionError, base::Unretained(this))); On 2016/11/23 22:33:31, Ken Rockot wrote: > nit, you could just bind directly to BindingSet::OnConnectionError with the ID > here. Done.
LGTM Discussed offline. Opened crbug.com/669606. https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2530613003/diff/1/media/mojo/services/mojo_re... media/mojo/services/mojo_renderer_service.cc:276: binding_->Close(); On 2016/11/29 10:20:57, xhwang wrote: > On 2016/11/23 23:45:06, tguilbert wrote: > > On 2016/11/23 22:27:09, xhwang wrote: > > > tguilbert: > > > > > > Today the |binding_| is a StrongBindingPtr which means that > > MojoRendererService > > > can only be bound with StrongBinding. I want to change MojoRendererService > so > > it > > > can be used with both mojo::Binding or mojo::StrongBinding (see the rest of > > this > > > CL). > > > > > > The check here would "work", but I feel it totally defies the original > purpose > > > of this code. After this change, only when using MediaPlayerRenderer would > we > > > set the |binding_| :) > > > > > > To fix it, what do you think of returning an error to signal "failure" so > that > > > the client side can drive the destruction process? > > > > I originally used a base::Optional<UnguessableToken>, and sent back a nullopt > > when this failed. However, at review time, I was asked about how likely this > > scenario was to happen; the conclusion was that only a misbehaving/compromised > > client would ever call InitiateScopedSurfaceRequest when > > |initiate_surface_request_cb_| is null. The guidance I received was then to > > assume that any unexpected call to this method was from a compromised client > > (and should be treated as a security issue). > > > > In that regards, I don't think that sending a failure flag to the client would > > accomplish much, since we couldn't trust a compromised client to destroy > itself. > > > > I am not sure what would be the right thing to do here then. I guess, if we > are > > using a strong binding, we close the binding (because it owns |this|, and it's > > the only way to safely delete |this|) and if we use a weak binding, we delete > > |this|. > > > > WDYT? > > tguilbert: > > I have a solution to use a bad_message_cb instead of binding: > https://chromiumcodereview.appspot.com/2539703002/ > > It works, but that makes me feel that we might have some layering/design issues. > ISTM that InitiateScopedSurfaceRequest() doesn't belong to the mojom::Renderer > interface. For example, it doesn't exist in the C++ media::Renderer interface. > It also look very special that in MojoRendererService we don't call into the > |renderer_| to do the work, but will call a callback. > > MediaPlayerRenderer is a Renderer, but it also has some additional > functionality, for example, to handle this surface request. In C++ layer, this > is done by binding additional features in the constructor, but in mojo, we don't > have this ability. One idea I have is to introduce an additional mojom > interface, e.g. > > interface MediaPlayerRenderer { > InitiateScopedSurfaceRequest() => ... > GetRenderer(Renderer& renderer); > } > > Then at RenderFrameHostImpl, we register a MediaPlayerRenderer service, and > connect to it from MediaPlayerRendererClient. > > At the service side, we need a simple impl of mojom::MediaPlayerRenderer, which > delegate all work to C++ media::MediaPlayerRenderer. In GetRenderer(), we can > create a MojoRendererService to bind the RendererRequest. You can see similar > model where we create a MojoDecryptorService in MojoCdmService: > > https://cs.chromium.org/chromium/src/media/mojo/services/mojo_cdm_service.cc?... > > I feel overall this will be a cleaner design and it will eliminate the bad > message handling here. I am sad that this means we'll have some special code for > MediaPlayerRenderer and it'll be different from other mojo based Renderers. In > the future, if "requesting a surface" becomes a common pattern in > media::Renderer, maybe we should add native support throughout the full Renderer > stack. > > WDYT? We discussed offline. I agree with everything that was said. I am opening a bug for future work and will add it to this CL.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2530613003/#ps20001 (title: "comments addressed")
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": 20001, "attempt_start_ts": 1480482603810500,
"parent_rev": "7bfc1a9539ab17aea7fa9b3e3fd114e8fb9d1c0d", "commit_rev":
"a79166c884f637e32cb267f6878941137893389d"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== media: Fix lifetime of InterfaceFactoryImpl and created impls Makes sure that when InterfaceFactoryImpl is destructed, all mojo interface implemenations (e.g. MojoAudioDecoderService) are destructed. To achieve this, a helper class StrongBindingSet is introduced such that: - When connection error happens, the entry will be removed from the binding set, and the bound impl object will be destructed. - When StrongBindingSet is destructed, it'll destruct all outstanding impl objects in the binding set. BUG=604912 TEST=Added a unittest for StrongBindingSet. ========== to ========== media: Fix lifetime of InterfaceFactoryImpl and created impls Makes sure that when InterfaceFactoryImpl is destructed, all mojo interface implemenations (e.g. MojoAudioDecoderService) are destructed. To achieve this, a helper class StrongBindingSet is introduced such that: - When connection error happens, the entry will be removed from the binding set, and the bound impl object will be destructed. - When StrongBindingSet is destructed, it'll destruct all outstanding impl objects in the binding set. BUG=604912 TEST=Added a unittest for StrongBindingSet. Committed: https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7 Cr-Commit-Position: refs/heads/master@{#435141} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7 Cr-Commit-Position: refs/heads/master@{#435141}
Message was sent while issue was closed.
Hello, this causes the following build error: ../../media/mojo/services/interface_factory_impl.cc:82:3: error: use of undeclared identifier 'video_decoder_bindings_'; did you mean 'DemuxerStream::video_decoder_config'? video_decoder_bindings_.AddBinding( ^~~~~~~~~~~~~~~~~~~~~~~ DemuxerStream::video_decoder_config https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... File media/mojo/services/interface_factory_impl.h (right): https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... media/mojo/services/interface_factory_impl.h:61: #if defined(ENABLE_MOJO_AUDIO_DECODER) Should be ENABLE_MOJO_VIDEO_DECODER
Message was sent while issue was closed.
https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... File media/mojo/services/interface_factory_impl.h (right): https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... media/mojo/services/interface_factory_impl.h:61: #if defined(ENABLE_MOJO_AUDIO_DECODER) On 2016/11/30 16:30:44, Julien Isorce wrote: > Should be ENABLE_MOJO_VIDEO_DECODER oops, thanks! I'll fix it now. OOC, are you using mojo video decoder?
Message was sent while issue was closed.
On 2016/11/30 17:38:49, xhwang wrote: > https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... > File media/mojo/services/interface_factory_impl.h (right): > > https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... > media/mojo/services/interface_factory_impl.h:61: #if > defined(ENABLE_MOJO_AUDIO_DECODER) > On 2016/11/30 16:30:44, Julien Isorce wrote: > > Should be ENABLE_MOJO_VIDEO_DECODER > > oops, thanks! > > I'll fix it now. > > OOC, are you using mojo video decoder? yes with https://codereview.chromium.org/2115603002/
Message was sent while issue was closed.
On 2016/11/30 17:41:28, Julien Isorce wrote: > On 2016/11/30 17:38:49, xhwang wrote: > > > https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... > > File media/mojo/services/interface_factory_impl.h (right): > > > > > https://codereview.chromium.org/2530613003/diff/20001/media/mojo/services/int... > > media/mojo/services/interface_factory_impl.h:61: #if > > defined(ENABLE_MOJO_AUDIO_DECODER) > > On 2016/11/30 16:30:44, Julien Isorce wrote: > > > Should be ENABLE_MOJO_VIDEO_DECODER > > > > oops, thanks! > > > > I'll fix it now. > > > > OOC, are you using mojo video decoder? > > yes with https://codereview.chromium.org/2115603002/ Cool and interesting :) Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
