Nested objects with RenderFrameHost pointers. Example case: MediaPlayerRenderer

[dan...@chromium.org]

Hello,

In attempting to clarify lifetimes, we are trying to make classes which are mojo-receivers but also tied to the document lifetime become DocumentService instances. This ensures there's no time window between the RenderFrameHost being destroyed and the task which notifies of the mojo connection being closed. Without doing this, any class with a pointer to RenderFrameHost has to check if it's alive on every use of the pointer, just to avoid this race.

In many cases, there is currently a mojo::SelfOwnedReceiver, which is tied to the life of the mojo connection, and we can replace that with making the class inherit content::DocumentService, and drop the SelfOwnedReceiver as the class becomes "self-owned" though actually owned indirectly through RenderFrameHost.

content::MediaPlayerRenderer is a more complicated example however. Like more common cases, it is a mojo receiver, and it has a pointer to RenderFrameHost (encoded as routing ids at the moment). But it is also a nested object, owned by an outer mojo receiver (media:: MojoRendererService) which is currently held in a mojo::SelfOwnedReceiver.

1. Since the nested content::MediaPlayerRenderer is owned externally by media::MojoRendererService, it can not be self-owned.

2.1. Therefore, we'd like to have the outer class become a DocumentService, instead of being owned by mojo::SelfOwnedReceiver.

2.2. Then the outer _and the nested class_ would have a deterministic lifetime relative to RenderFrameHost, and we could hold a base::SafeRef<RenderFrameHost> in both classes instead of routing ids, and they'd never have to worry about racing with RenderFrameHost's destruction.

However.... layering....

3.1. content::DocumentService is part of content.

3.2. The nested class content::MediaPlayerRenderer knows about content (it knows about RenderFrameHost)

3.3. But the outer class is in media, which can not know about content, meaning we can't do 2.1.

4. So I thought we can move the outer class from media/ to content/, however it is also instantiated from chromecast/ and media/, where it is given an inner nested object that does _not_ know about content.

4.1. Thus media::MojoRendererService needs to be able to work without content.

4.2. But we also want it to be lifetime-associated to RenderFrameHost, which we normally do through content::DocumentService.

To break the contradictions of 4.1 and 4.2, we would need the concept of a "mojo::Receiver that is self-owned and lifetime-associated with T, with a safe pointer to T" to be generalized outside of content::DocumentService.

Then, content::DocumentService is a specialization of this concept for content::RenderFrameHost.

We attempted to generalize DocumentService before, as base::StrongRef, however that went further to generalize ownership-and-a-backpointer without mojo::Receiver. We determined that it was too restrictive and hard to use so generally. I think that we should instead consider generalizing content::DocumentService but specifically around mojo::Receivers still - as a sibling to the mojo::SelfOwnedReceiver which is a self-owned-non-lifetime-associated mojo::Receiver.

Concrete proposal with code here: https://godbolt.org/z/fEMoTj3hj

High level:

5. Our content-specific lifetime-associated-to-RenderFrameHost is generalized into mojo.

content::DocumentService<Interface> becomes mojo::LifetimeAssociatedReceiver<Interface, RenderFrameHost*, DocumentServiceDestroyedReason>.

content::DocumentServiceBase becomes mojo::LifetimeAssociatedBase (this is needed to hold collections of heterogeneous LifetimeAssociatedReceivers)

6. The media::MojoRendererService becomes a mojo::LifetimeAssociatedReceiver instead of being owned by mojo::SelfOwnedReceiver

7.1. Then content code can instantiate it with a content::MediaPlayerRenderer and associate their lifetimes to RenderFrameHost.

7.2. And media code, or cast code, can instantiate it with other media::Renderer types and not need to associate their lifetimes to any other type.

Thoughts?

Thanks,

Dana

[Ken Rockot]

On Thu, Oct 7, 2021 at 6:59 AM <dan...@chromium.org> wrote:

Hello,

In attempting to clarify lifetimes, we are trying to make classes which are mojo-receivers but also tied to the document lifetime become DocumentService instances. This ensures there's no time window between the RenderFrameHost being destroyed and the task which notifies of the mojo connection being closed. Without doing this, any class with a pointer to RenderFrameHost has to check if it's alive on every use of the pointer, just to avoid this race.

In many cases, there is currently a mojo::SelfOwnedReceiver, which is tied to the life of the mojo connection, and we can replace that with making the class inherit content::DocumentService, and drop the SelfOwnedReceiver as the class becomes "self-owned" though actually owned indirectly through RenderFrameHost.

content::MediaPlayerRenderer is a more complicated example however. Like more common cases, it is a mojo receiver, and it has a pointer to RenderFrameHost (encoded as routing ids at the moment). But it is also a nested object, owned by an outer mojo receiver (media:: MojoRendererService) which is currently held in a mojo::SelfOwnedReceiver.

1. Since the nested content::MediaPlayerRenderer is owned externally by media::MojoRendererService, it can not be self-owned.

2.1. Therefore, we'd like to have the outer class become a DocumentService, instead of being owned by mojo::SelfOwnedReceiver.

2.2. Then the outer _and the nested class_ would have a deterministic lifetime relative to RenderFrameHost, and we could hold a base::SafeRef<RenderFrameHost> in both classes instead of routing ids, and they'd never have to worry about racing with RenderFrameHost's destruction.

However.... layering....

3.1. content::DocumentService is part of content.

3.2. The nested class content::MediaPlayerRenderer knows about content (it knows about RenderFrameHost)

3.3. But the outer class is in media, which can not know about content, meaning we can't do 2.1.

4. So I thought we can move the outer class from media/ to content/, however it is also instantiated from chromecast/ and media/, where it is given an inner nested object that does _not_ know about content.

4.1. Thus media::MojoRendererService needs to be able to work without content.

4.2. But we also want it to be lifetime-associated to RenderFrameHost, which we normally do through content::DocumentService.

What about introducing a new Content class that can be a DocumentService and own a MojoRenderService?

To break the contradictions of 4.1 and 4.2, we would need the concept of a "mojo::Receiver that is self-owned and lifetime-associated with T, with a safe pointer to T" to be generalized outside of content::DocumentService.

Then, content::DocumentService is a specialization of this concept for content::RenderFrameHost.

We attempted to generalize DocumentService before, as base::StrongRef, however that went further to generalize ownership-and-a-backpointer without mojo::Receiver. We determined that it was too restrictive and hard to use so generally. I think that we should instead consider generalizing content::DocumentService but specifically around mojo::Receivers still - as a sibling to the mojo::SelfOwnedReceiver which is a self-owned-non-lifetime-associated mojo::Receiver.

Concrete proposal with code here: https://godbolt.org/z/fEMoTj3hj

High level:

5. Our content-specific lifetime-associated-to-RenderFrameHost is generalized into mojo.

content::DocumentService<Interface> becomes mojo::LifetimeAssociatedReceiver<Interface, RenderFrameHost*, DocumentServiceDestroyedReason>.

content::DocumentServiceBase becomes mojo::LifetimeAssociatedBase (this is needed to hold collections of heterogeneous LifetimeAssociatedReceivers)

6. The media::MojoRendererService becomes a mojo::LifetimeAssociatedReceiver instead of being owned by mojo::SelfOwnedReceiver

7.1. Then content code can instantiate it with a content::MediaPlayerRenderer and associate their lifetimes to RenderFrameHost.

7.2. And media code, or cast code, can instantiate it with other media::Renderer types and not need to associate their lifetimes to any other type.

Thoughts?

Thanks,

Dana

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAHtyhaSk%2BVbXzxAmWc2tAYGYOOVwF877VKedFYKi-NEWGV3jyw%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

MojoRenderService is owned by InterfaceFactoryImpl, which is then owned by MediaService.

The reason a lot of these classes live in media/ is that it's supposed to be genetic. For example, MediaService/InterfaceFactoryImpl can be hosted in a utility process, or the GPU process, where you won't have the concept of RenderFrameHost.

InterfaceFactoryImpl is already tied to the lifetime of a Document. So when the document is destroyed in the browser process, MediaInterfaceProxy in the browser process will be destroyed, which will cause a disconnection so that InterfaceFactoryImpl in MediaService will be destroyed (via the use of mojo::UniqueReceiverSet), which will destroy MojoRendererService.

Then it's assumed that other classes affiliated with MojoRendererService, e.g. MediaPlayerRenderer will manage things by itself to make sure things are safe during the teardown/destruction process. This is so again because MediaService/InterfaceFactoryImpl can be used in many different contexts and it's hard to have a general rule to ensure things like raw-pointer safety.

MediaPlayerRenderer runs in the browser process. So I assume when the document is destroyed, MediaInterfaceProxy will be destroyed, which will then destroy InterfaceFactoryImpl (synchronously since in-process???), which will then destroy MojoRendererService and MediaPlayerRenderer. Does this look right? It's a longer stack than what I'd like, but I feel the best/simplest fix might be to fix  MediaPlayerRenderer directly, e.g. check whether the RFH still exists...