How can WebContentsObserver talk to a specific RenderFrameObserver?

[Xiaohan Wang (王消寒)]

Hello Site Isolation Gurus,

Currently on Chrome for Android, the media pipeline is separated between the render and the browser processes: WebMediaPlayerAndroid (and JS/Blink) lives in the render process, but the real decoding work, which is backed by Android Java API, had to be conducted in the browser process. The two sides needs to talk to each other (via IPC messages in both directions) constantly during a media playback. To facilitate this communication, we created RendererMediaPlayerManager (a RenderViewObserver) at the render side, and BrowserMediaPlayerManager (a WebContentsObserver) at the browser side.

This model works now because WebContentsObserver::Send() uses WebContentsImpl::Send(), which calls RenderViewHost::Send(). This matches the RenderViewObserver we have at the render side.

Now it seems that RenderViewImpl and RenderViewObserver is going away, and RenderFrame* is the new hotness. I plan to update RMPM to be a RenderFrameObserver (media player in different frames don't need to talk to each other anyways). Yay! However, I found it's not easy....

At the browser side, if I keep using WebContentsObserver, I can receive messages from the render side. But when I need to send a message back to the render side, there's no way I can get the correct routing ID for the specific RenderFrame I want to talk to. Actually, when the message is sent from the renderer to the browser, we have the routing ID buried in the message, but it's lost somewhere in the IPC_MESSAGE_HANDLER macros. I can manually process the message and store the RenderFrame routing ID somewhere, but that seems hacky.

If I don't use WebContentsObserver, I don't see obvious solutions right now.

Question: Can we improve WebContentsObserver such that it can send messages back to different RenderFrameObservers easily? Or do you have any other suggestions?

Cheers,

Xiaohan

[Charlie Reis]

[+jam]

That's a great question.  John, any thoughts on this case?  This is in the theme of knowing who sent you a message, and being able to reply.

It's also hard to target a message to a frame from outside content, but at least this case is within content.

Charlie

[John Abd-El-Malek]

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost. I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

An alternate (to list them) would be to have a wrapper object that hides RFH details and which just has a Send() method and it could guard against the lifetime issue. But then it seems that in some cases the caller would want to know the RFH's URL etc for security checks, and this generic object would not be useful in that regard.

What do others think?

On Wed, Apr 2, 2014 at 3:43 PM, Charlie Reis <cr...@chromium.org> wrote:

[+jam]

That's a great question.  John, any thoughts on this case?  This is in the theme of knowing who sent you a message, and being able to reply.

It's also hard to target a message to a frame from outside content,

What do you mean? If one has a frame's process and routing id, they can send it an IPC? 

 

but at least this case is within content.

Charlie

On Wed, Apr 2, 2014 at 3:37 PM, Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Hello Site Isolation Gurus,

Currently on Chrome for Android, the media pipeline is separated between the render and the browser processes: WebMediaPlayerAndroid (and JS/Blink) lives in the render process, but the real decoding work, which is backed by Android Java API, had to be conducted in the browser process. The two sides needs to talk to each other (via IPC messages in both directions) constantly during a media playback. To facilitate this communication, we created RendererMediaPlayerManager (a RenderViewObserver) at the render side, and BrowserMediaPlayerManager (a WebContentsObserver) at the browser side.

This model works now because WebContentsObserver::Send() uses WebContentsImpl::Send(), which calls RenderViewHost::Send(). This matches the RenderViewObserver we have at the render side.

Now it seems that RenderViewImpl and RenderViewObserver is going away, and RenderFrame* is the new hotness. I plan to update RMPM to be a RenderFrameObserver (media player in different frames don't need to talk to each other anyways). Yay! However, I found it's not easy....

Thanks for helping with the refactoring!

[Nasko Oskov]

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one? Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

An alternate (to list them) would be to have a wrapper object that hides RFH details and which just has a Send() method and it could guard against the lifetime issue. But then it seems that in some cases the caller would want to know the RFH's URL etc for security checks, and this generic object would not be useful in that regard.

This circles back to what I've brought up in the past discussions - frame level object, exposed outside of content that is not the RFH and can hide the RFH complexities (pending, switching if navigation is made, so on). Sounds like a very good approach, if and only if we can hide pending RFHs from code. This week I learned that Task Manager will need to keep on knowing about pending ones, so that might not be ideal.

[John Abd-El-Malek]

On Thu, Apr 3, 2014 at 7:16 PM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one?

Right.

 

Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

The difference is that this is scoped to just the place that we need this in (IPCs dispatched in WCOs). In other places the code should have the process id already if it needs it.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

An alternate (to list them) would be to have a wrapper object that hides RFH details and which just has a Send() method and it could guard against the lifetime issue. But then it seems that in some cases the caller would want to know the RFH's URL etc for security checks, and this generic object would not be useful in that regard.

This circles back to what I've brought up in the past discussions - frame level object, exposed outside of content that is not the RFH and can hide the RFH complexities (pending, switching if navigation is made, so on). Sounds like a very good approach, if and only if we can hide pending RFHs from code. This week I learned that Task Manager will need to keep on knowing about pending ones, so that might not be ideal.

There are multiple things here:

-hiding pending frame/view: btw talking to Darin today, he mentioned the idea of never creating a pending RFH or RVH in the first place. Instead, make a request to the pending url independent of a RVH/RFH and then based on the result of that, create a new RVH/RFH if it's a cross-site navigation. That seemed like a good solution since then instead of hiding a live object (pending), we don't even have it.

-having one object that is constant across cross-site frame navigations: it's really not clear to me what the advantage is? It wouldn't solve the IPC sending issue we're discussing heree, since if it was kept around a Send() might send a reply to the wrong process.

Why does task manager need to know about pending views/frames?

[Nasko Oskov]

On Thu, Apr 3, 2014 at 7:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:16 PM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one?

Right.

 

Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

The difference is that this is scoped to just the place that we need this in (IPCs dispatched in WCOs). In other places the code should have the process id already if it needs it.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

An alternate (to list them) would be to have a wrapper object that hides RFH details and which just has a Send() method and it could guard against the lifetime issue. But then it seems that in some cases the caller would want to know the RFH's URL etc for security checks, and this generic object would not be useful in that regard.

This circles back to what I've brought up in the past discussions - frame level object, exposed outside of content that is not the RFH and can hide the RFH complexities (pending, switching if navigation is made, so on). Sounds like a very good approach, if and only if we can hide pending RFHs from code. This week I learned that Task Manager will need to keep on knowing about pending ones, so that might not be ideal.

There are multiple things here:

-hiding pending frame/view: btw talking to Darin today, he mentioned the idea of never creating a pending RFH or RVH in the first place. Instead, make a request to the pending url independent of a RVH/RFH and then based on the result of that, create a new RVH/RFH if it's a cross-site navigation. That seemed like a good solution since then instead of hiding a live object (pending), we don't even have it.

I'll defer to Charlie on this one, but don't we need process/routing id to be associated with making the request?

 

-having one object that is constant across cross-site frame navigations: it's really not clear to me what the advantage is? It wouldn't solve the IPC sending issue we're discussing heree, since if it was kept around a Send() might send a reply to the wrong process.

That would depend if we ever send IPCs to non-commited RVH/RFH from WCO objects, right? In this example of media, there is no way for them to initialize media player before the commit has happened and the document is loading.

 

Why does task manager need to know about pending views/frames?

I think it has to do with calling WCO RenderViewCreated/RenderFrameCreated at time of creating the pending one. Task Manager needs to keep track of all RVH/RFH objects, so it puts them in its data structures on creation. We toyed with the idea of delaying notifying the observers until the navigation commits, but this needs to be tested out to see if anything will break.

[Charlie Reis]

[+nick]

On Fri, Apr 4, 2014 at 6:56 AM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:16 PM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one?

Right.

 

Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

The difference is that this is scoped to just the place that we need this in (IPCs dispatched in WCOs). In other places the code should have the process id already if it needs it.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

On one hand, if we're trying to go down the path of not exposing RFH, it feels counterproductive to pass RFH to message handlers.  What if WebContents had versions of OnMessageReceived and Send for frames that had routing ID and process ID as parameters, so that callers never have to interact with RFH directly?

On the other hand, Nick brings up a totally reasonable point that we're putting the burden of filtering messages to the one frame you care about on all of the callers around the codebase.  That's the tradeoff for not having a RFHO, I suppose, and perhaps Nick can chime in more on his concerns here.

An alternate (to list them) would be to have a wrapper object that hides RFH details and which just has a Send() method and it could guard against the lifetime issue. But then it seems that in some cases the caller would want to know the RFH's URL etc for security checks, and this generic object would not be useful in that regard.

This circles back to what I've brought up in the past discussions - frame level object, exposed outside of content that is not the RFH and can hide the RFH complexities (pending, switching if navigation is made, so on). Sounds like a very good approach, if and only if we can hide pending RFHs from code. This week I learned that Task Manager will need to keep on knowing about pending ones, so that might not be ideal.

There are multiple things here:

-hiding pending frame/view: btw talking to Darin today, he mentioned the idea of never creating a pending RFH or RVH in the first place. Instead, make a request to the pending url independent of a RVH/RFH and then based on the result of that, create a new RVH/RFH if it's a cross-site navigation. That seemed like a good solution since then instead of hiding a live object (pending), we don't even have it.

I'll defer to Charlie on this one, but don't we need process/routing id to be associated with making the request?

There has been a long-standing desire (shared by both Darin and me) to totally change how navigation works.  Navigation would start with a message to the browser process, which would issue a network request.  The response's MIME type would determine what sort of process we hand the stream off to.  This is in contrast to navigations having to go through the renderer process to initiate a network request in the ResourceLoader.

I hadn't thought about the fact that this would let us eliminate pending RFHs.  That seems right, and it would be great.

On the flip side, that's an enormous overhaul of both navigation and the resource loader logic, from what I can tell.  It's outside the scope of the site isolation project in my opinion, since it would set us back substantially.  I absolutely want to see it happen, but I'd also like to get site isolation launched first if possible.

 

 

-having one object that is constant across cross-site frame navigations: it's really not clear to me what the advantage is? It wouldn't solve the IPC sending issue we're discussing heree, since if it was kept around a Send() might send a reply to the wrong process.

That would depend if we ever send IPCs to non-commited RVH/RFH from WCO objects, right? In this example of media, there is no way for them to initialize media player before the commit has happened and the document is loading.

 

Why does task manager need to know about pending views/frames?

I think it has to do with calling WCO RenderViewCreated/RenderFrameCreated at time of creating the pending one. Task Manager needs to keep track of all RVH/RFH objects, so it puts them in its data structures on creation. We toyed with the idea of delaying notifying the observers until the navigation commits, but this needs to be tested out to see if anything will break.

I might be wrong, but I think Nick solved this without needing to know about pending views/frames.  Adding a RenderFrameHostChanged method to WebContentsObserver was sufficient, if I understand correctly.  We don't have any immediate plans to show pending views/frames in the task manager itself, though that wouldn't be out of the question.  (I'd rather get rid of pending RFHs before that happens, though.)

Of course, the task manager is another very tricky example of code outside content needing to know about RFH.  Down the road, maybe parts of its model will migrate into content similar to DevTools?

Charlie 

[Nick Carter]

On Fri, Apr 4, 2014 at 10:31 AM, Charlie Reis <cr...@chromium.org> wrote:

[+nick]

On Fri, Apr 4, 2014 at 6:56 AM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:16 PM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one?

Right.

 

Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

The difference is that this is scoped to just the place that we need this in (IPCs dispatched in WCOs). In other places the code should have the process id already if it needs it.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

On one hand, if we're trying to go down the path of not exposing RFH, it feels counterproductive to pass RFH to message handlers.  What if WebContents had versions of OnMessageReceived and Send for frames that had routing ID and process ID as parameters, so that callers never have to interact with RFH directly?

On the other hand, Nick brings up a totally reasonable point that we're putting the burden of filtering messages to the one frame you care about on all of the callers around the codebase.  That's the tradeoff for not having a RFHO, I suppose, and perhaps Nick can chime in more on his concerns here.

My concern came from the following thought experiment: suppose you were trying to implement something like Xiaohan's example which started this thread, and suppose you were only given an WebContentsObserver interface that looked like:

OnMessageReceived(RenderFrameHost-ish* source, Message* m) {

}

It seems to me that, if your class needs to keep any state whatsoever that is per-frame, then the options are either (a) to rolling your own routing mechanism, keeping instances of an appropriate class (e.g. BrowserMediaPlayerManager) in a map keyed by |source|, or else (b) try to do in a way that handles multiple |source|s simultaneously thing even in a one-to-many relationship.

The problem I see with (b) is a particularly scary class of bugs, where things work properly until two renderer clients start making interleaved requests of the shared browser instance, and suddenly streams cross and privileges escalate. So I would think, in this world, that (a) is a best practice. And if we accept that, I feel it would be worthwhile to provide some mechanism for it, that eliminates the amount of boilerplate map<>-keeping?

Two BrowserMediaPlayerManagers that happen to belong to the same WebContents don't need to interact with each other in any special way, right? If not, then we can say this class doesn't care about WebContents qua WebContents at all. Instead it just wants to provide a connection from some Frame-level code running in a renderer to some other nugget of functionality that must live in the browser process, scoped to one Frame instance down in the renderer. When the browser process code in question obtains the program flow, it's simply an easier situation if we start in a narrow (Frame) context and widen as needed to a parent (WebContents) context, than it is to start in the parent (WebContents) context and use a parameter (|source|) to narrow things down.

I see presently 73 overrides of WebContentsObserver::OnMessageReceived, most of which handle multiple IPCs, and presumably many of these are going to be in a similar predicament as BMPM when their day of reckoning comes. Autofill, https://code.google.com/p/chromium/codesearch#chromium/src/components/autofill/content/browser/content_autofill_driver.cc&cl=GROK&l=175 is another juicy example that seems super stateful.

In short I wonder if we're creating trouble by forcing this use case to interact with messages at the WebContents granularity. They want to be affiliated with Frames, but we're forcing them to be affiliated with a page and to do their own frame-figuring.

An alternate (to list them) would be to have a wrapper object that hides RFH details and which just has a Send() method and it could guard against the lifetime issue. But then it seems that in some cases the caller would want to know the RFH's URL etc for security checks, and this generic object would not be useful in that regard.

Half-baked idea: what if clients could register a MessageObserverFactory with a WebContents, with the factory's Create() method to be called once per RFH, returning a client-specific MessageObserver instance for that frame? I am thinking the MessageObserver could contain just the IPC Listener/Sender senses of the existing WebContentsObserver, and it might not even need to see or know the RFH pointer, though it would have some informational needs like the URL as suggested above. This Factory ability could even be done via new methods on WebContentsObserver, avoiding the requirement that clients monkey around with a new interface.

This could eliminate the burden mentioned by John earlier: "It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime," because the lifetime management of the per-Frame observers would be provided by the WebContents. I'm less certain that it would suffice for all cases, but it sounds way easier than managing a map of RFHOs, particularly when it comes to thinking about destruction time.

This circles back to what I've brought up in the past discussions - frame level object, exposed outside of content that is not the RFH and can hide the RFH complexities (pending, switching if navigation is made, so on). Sounds like a very good approach, if and only if we can hide pending RFHs from code. This week I learned that Task Manager will need to keep on knowing about pending ones, so that might not be ideal.

I would caution against using the Task Manager as a representative example; let's focus on figuring out what makes sense for normal clients.

 

There are multiple things here:

-hiding pending frame/view: btw talking to Darin today, he mentioned the idea of never creating a pending RFH or RVH in the first place. Instead, make a request to the pending url independent of a RVH/RFH and then based on the result of that, create a new RVH/RFH if it's a cross-site navigation. That seemed like a good solution since then instead of hiding a live object (pending), we don't even have it.

I'll defer to Charlie on this one, but don't we need process/routing id to be associated with making the request?

There has been a long-standing desire (shared by both Darin and me) to totally change how navigation works.  Navigation would start with a message to the browser process, which would issue a network request.  The response's MIME type would determine what sort of process we hand the stream off to.  This is in contrast to navigations having to go through the renderer process to initiate a network request in the ResourceLoader.

I hadn't thought about the fact that this would let us eliminate pending RFHs.  That seems right, and it would be great.

On the flip side, that's an enormous overhaul of both navigation and the resource loader logic, from what I can tell.  It's outside the scope of the site isolation project in my opinion, since it would set us back substantially.  I absolutely want to see it happen, but I'd also like to get site isolation launched first if possible.

 

 

-having one object that is constant across cross-site frame navigations: it's really not clear to me what the advantage is? It wouldn't solve the IPC sending issue we're discussing heree, since if it was kept around a Send() might send a reply to the wrong process.

That would depend if we ever send IPCs to non-commited RVH/RFH from WCO objects, right? In this example of media, there is no way for them to initialize media player before the commit has happened and the document is loading.

 

Why does task manager need to know about pending views/frames?

I think it has to do with calling WCO RenderViewCreated/RenderFrameCreated at time of creating the pending one. Task Manager needs to keep track of all RVH/RFH objects, so it puts them in its data structures on creation. We toyed with the idea of delaying notifying the observers until the navigation commits, but this needs to be tested out to see if anything will break.

I might be wrong, but I think Nick solved this without needing to know about pending views/frames.  Adding a RenderFrameHostChanged method to WebContentsObserver was sufficient, if I understand correctly.  We don't have any immediate plans to show pending views/frames in the task manager itself, though that wouldn't be out of the question.  (I'd rather get rid of pending RFHs before that happens, though.)

I don't plan on making the TaskManager show pending views/frames.

[Nick Carter]

On Fri, Apr 4, 2014 at 12:51 PM, Nick Carter <ni...@chromium.org> wrote:

On Fri, Apr 4, 2014 at 10:31 AM, Charlie Reis <cr...@chromium.org> wrote:

[+nick]

On Fri, Apr 4, 2014 at 6:56 AM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:16 PM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one?

Right.

 

Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

The difference is that this is scoped to just the place that we need this in (IPCs dispatched in WCOs). In other places the code should have the process id already if it needs it.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

On one hand, if we're trying to go down the path of not exposing RFH, it feels counterproductive to pass RFH to message handlers.  What if WebContents had versions of OnMessageReceived and Send for frames that had routing ID and process ID as parameters, so that callers never have to interact with RFH directly?

On the other hand, Nick brings up a totally reasonable point that we're putting the burden of filtering messages to the one frame you care about on all of the callers around the codebase.  That's the tradeoff for not having a RFHO, I suppose, and perhaps Nick can chime in more on his concerns here.

My concern came from the following thought experiment: suppose you were trying to implement something like Xiaohan's example which started this thread, and suppose you were only given an WebContentsObserver interface that looked like:

OnMessageReceived(RenderFrameHost-ish* source, Message* m) {

}

It seems to me that, if your class needs to keep any state whatsoever that is per-frame, then the options are either (a) to rolling your own routing mechanism, keeping instances of an appropriate class (e.g. BrowserMediaPlayerManager) in a map keyed by |source|, or else (b) try to do in a way that handles multiple |source|s simultaneously thing even in a one-to-many relationship.

Ugh, sent a little early. Clarification: (b) should be, "try to write a class that handles multiple |source|s simultaneously, in a one-to-many relationship."

[John Abd-El-Malek]

Lots of replies, hopefully I didn't miss any :)

Regarding redoing navigation to get rid of pending views/frames, I completely agree that this shouldn't be part of site isolation as it's mostly orthogonal. No need to add such complicated things to an already complex project.

On Fri, Apr 4, 2014 at 12:54 PM, Nick Carter <ni...@chromium.org> wrote:

On Fri, Apr 4, 2014 at 12:51 PM, Nick Carter <ni...@chromium.org> wrote:

On Fri, Apr 4, 2014 at 10:31 AM, Charlie Reis <cr...@chromium.org> wrote:

[+nick]

On Fri, Apr 4, 2014 at 6:56 AM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 7:16 PM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, Apr 3, 2014 at 6:42 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Yeah very good question, we haven't hit this scenario yet. In the old RVH days, this was mostly (i.e. ignoring pending case) not a problem because WC->Send just went to the RV that sent this IPC.

Working backwards, it seems that we will eventually not have a WC::Send method since it won't make sense. That means that all IPC replies on the browser side would have to be made through RenderFrameHost. Which means that the OnMessageReceived would need a RenderFrameHost.

Does that mean that we need to create a version of OnMessageReceived that is different than the IPC::Listener one?

Right.

 

Wouldn't it be equivalent to just embed into the IPC message the process that originated it? Then receivers of the messages can do a RFH::FromID to look it up.

The difference is that this is scoped to just the place that we need this in (IPCs dispatched in WCOs). In other places the code should have the process id already if it needs it.

 

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

I do like having just one observer interface, so I'm all for avoiding creation of RFHO.

 

The cons here are that the callee needs to be careful in not holding on to RFH pointers after the stack unwinds, since it might go away. They need to be careful to keep its routing id pairs instead. Another con is that this will spread RFH usage across the code base a lot, making possibly hiding it in the future harder.

On one hand, if we're trying to go down the path of not exposing RFH, it feels counterproductive to pass RFH to message handlers.  What if WebContents had versions of OnMessageReceived and Send for frames that had routing ID and process ID as parameters, so that callers never have to interact with RFH directly?

On the other hand, Nick brings up a totally reasonable point that we're putting the burden of filtering messages to the one frame you care about on all of the callers around the codebase.  That's the tradeoff for not having a RFHO, I suppose, and perhaps Nick can chime in more on his concerns here.

My concern came from the following thought experiment: suppose you were trying to implement something like Xiaohan's example which started this thread, and suppose you were only given an WebContentsObserver interface that looked like:

OnMessageReceived(RenderFrameHost-ish* source, Message* m) {

}

It seems to me that, if your class needs to keep any state whatsoever that is per-frame, then the options are either (a) to rolling your own routing mechanism, keeping instances of an appropriate class (e.g. BrowserMediaPlayerManager) in a map keyed by |source|, or else (b) try to do in a way that handles multiple |source|s simultaneously thing even in a one-to-many relationship.

Ugh, sent a little early. Clarification: (b) should be, "try to write a class that handles multiple |source|s simultaneously, in a one-to-many relationship."

 

The problem I see with (b) is a particularly scary class of bugs, where things work properly until two renderer clients start making interleaved requests of the shared browser instance, and suddenly streams cross and privileges escalate. So I would think, in this world, that (a) is a best practice. And if we accept that, I feel it would be worthwhile to provide some mechanism for it, that eliminates the amount of boilerplate map<>-keeping?

I think we should wait for concrete examples when we're actually refactoring code. Changing over a few classes will be very helpful in understanding how much work it is to go with a vs b, or if there are other possible ways of doing this that are simpler.

 

Two BrowserMediaPlayerManagers that happen to belong to the same WebContents don't need to interact with each other in any special way, right? If not, then we can say this class doesn't care about WebContents qua WebContents at all. Instead it just wants to provide a connection from some Frame-level code running in a renderer to some other nugget of functionality that must live in the browser process, scoped to one Frame instance down in the renderer. When the browser process code in question obtains the program flow, it's simply an easier situation if we start in a narrow (Frame) context and widen as needed to a parent (WebContents) context, than it is to start in the parent (WebContents) context and use a parameter (|source|) to narrow things down.

I see presently 73 overrides of WebContentsObserver::OnMessageReceived, most of which handle multiple IPCs, and presumably many of these are going to be in a similar predicament as BMPM when their day of reckoning comes. Autofill, https://code.google.com/p/chromium/codesearch#chromium/src/components/autofill/content/browser/content_autofill_driver.cc&cl=GROK&l=175 is another juicy example that seems super stateful.

In short I wonder if we're creating trouble by forcing this use case to interact with messages at the WebContents granularity. They want to be affiliated with Frames, but we're forcing them to be affiliated with a page and to do their own frame-figuring.

It's not clear to me that embedders care much about the frames themselves, as these are an implementation detail of content. They might have to keep a pointer to them (or to their routing id pair) so that they can send IPCs to objects in the renderer. Again though, I think after converting a few pieces of code this stuff will be much clearer.

As an example, so far I have converted prerendering, content settings, plugins, and shared workers. I didn't run into all the issues outlined here.

[Xiaohan Wang (王消寒)]

Thanks for all the discussions. Just to clarify, in our case, media players that belongs to different frames don't need to talk to/see each other :)

[Nick Carter]

If the pattern you've used so far will generalize, what is your concrete solution to Xiaohan's problem?

[John Abd-El-Malek]

As a start, I had proposed exposing the RenderFrameHost when dispatching an IPC. We leave the OnMessageReceived signature the same, and have a method on WebContentsObserver like RenderFrameHost* GetRFHForDispatchingIPC that returns non-NULL when the IPC is coming from a RFH. 

[Nick Carter]

Working in this concrete example, suppose implement that proposal. Because BrowserMediaPlayerManager is a WebContentsObserver, we'll try to modify it to handle messages that are received for any of a WebContents's frames.

Now suppose we're receiving a CdmHostMsg_CreateSession.OnCreateSession message, which is handled asynchronously (because of a permissions UI check) -- it posts a task and expects a callback reply. In this case we have to call the new GetRFHForDispatchingIPC, and bind the result to the callback, right? Otherwise, the sender information is lost. So the RenderFrameHost* is now a parameter to the callback method, CreateSessionIfPermitted.

At that point, we have the following problems:

 - If the RenderFrameHosts goes away while the async request is pending, how would CreateSessionIfPermitted know?

 - The source RenderFrameHost* must also be passed to helper methods of this class, like OnSessionError (which does a Send()). However, because we're in a class that's a WebContentsObserver, it would be tempting to call GetRFHForDispatchingIPC() from OnSessionError instead of passing it explicitly. This would result in code that's correct when OnSessionError is called synchronously (most of the calls are), but crashes when called from a callback. The appropriate RFH* also needs to be plumbed into OnProtectedSurfaceRequested

 - A teardown message like MediaPlayerHostMsg_DestroyAllMediaPlayers changes semantics in the new world, because it goes from meaning "destroy all media players" to meaning "destroy all media players associated with GetRFHForDispatchingIPC()". So if you don't update this as well, then you could wind up with a bug where a navigation in a subframe would send the IPC message (it's sent by ~RendererMediaPlayerManager's dtor), and destroy a media player that belonged to the parent frame. This implies that internally, we'd need to keep track of the association between RenderFrameHosts and mediaplayers.

In a class implemented against the proposed interface, it seems unavoidable to me that the RenderFrameHost pointers wind up everywhere -- as map keys and as bound method arguments. Even though you're right that theses classes don't really care about frames, the need still arises to track the lifetimes and identities of frames, precisely because the WebContentsObserver's method interface forces them into a conversation with more than one RenderFrameHost at the same time.

 - nick

[John Abd-El-Malek]

Note: we never pass pointers to objects that aren't guaranteed to be alive in callbacks. This includes RenderFrameHost, and also RenderViewHost/RenderProcessHost/WebContents etc. So in this case, there would be a few options

-the callback takes in RenderFrameHost's render_process_id and render_frame_id, and it can use RenderFrameHost's static method to get a pointer if the object is alive

-the calling code can try to get the RenderFrameHost object itself, and not call the object if it's gone

Which particular approach depends on how the code is structures, i.e. if cleanup is needed in BrowserMediaPlayerManager, the first approach is necessary.

At that point, we have the following problems:

 - If the RenderFrameHosts goes away while the async request is pending, how would CreateSessionIfPermitted know? 

 

 - The source RenderFrameHost* must also be passed to helper methods of this class, like OnSessionError (which does a Send()). However, because we're in a class that's a WebContentsObserver, it would be tempting to call GetRFHForDispatchingIPC() from OnSessionError instead of passing it explicitly. This would result in code that's correct when OnSessionError is called synchronously (most of the calls are), but crashes when called from a callback. The appropriate RFH* also needs to be plumbed into OnProtectedSurfaceRequested

 - A teardown message like MediaPlayerHostMsg_DestroyAllMediaPlayers changes semantics in the new world, because it goes from meaning "destroy all media players" to meaning "destroy all media players associated with GetRFHForDispatchingIPC()". So if you don't update this as well, then you could wind up with a bug where a navigation in a subframe would send the IPC message (it's sent by ~RendererMediaPlayerManager's dtor), and destroy a media player that belonged to the parent frame.

Sure, I grant you that buggy code can exist in this transition, or even after. Similar scenarios can happen before though; i.e. with objects that hold state per render process, per profile, or per browser process. We always have the possibility of writing buggy code :)

This implies that internally, we'd need to keep track of the association between RenderFrameHosts and mediaplayers. 

In a class implemented against the proposed interface, it seems unavoidable to me that the RenderFrameHost pointers wind up everywhere -- as map keys and as bound method arguments.

Per above, this would really need to be the routing pairs as the raw pointers aren't safe. We already have this in tons of places today with RenderViewHost routing pairs, so I don't see this as anything new..

[Nick Carter]

I know that bugs are always possible, but I wonder if you've missed my point. To recap: today we have a bunch of classes that are presumed correct while in a 1-1 relationship with a RenderView. You're advocating putting them in a 1-many relationship with RenderFrames split across many processes. I have not argued that this can't be done correctly, I'm arguing that there's a way to avoid having to do it at all. Finding a way to avoid semantic changes and complexity growth in as many as fifty classes sounds extremely attractive to me, because it means not having to confront and slay bugs like the examples I provided. And it seems we can achieve this if we provide a way for these classes to be in a 1-1 relationship with RenderFrames.

What do we gain by providing no simple way for an api client to scope itself to just one RFH at a time? Reading back on the thread, my understanding is that our goals were to avoid exposing RFH like we exposed RVH, and generally to be conservative about adding new stuff to the content API. Is there another specific design objective here? I don't really understand how putting the RFH in a limited-time-only WCO getter helps us achieve the goal of hiding the RFH. But at the same time, I never experienced firsthand the pain that exposing RVH caused, so perhaps I just need to better understand what those problems were.

This implies that internally, we'd need to keep track of the association between RenderFrameHosts and mediaplayers. 

In a class implemented against the proposed interface, it seems unavoidable to me that the RenderFrameHost pointers wind up everywhere -- as map keys and as bound method arguments.

Per above, this would really need to be the routing pairs as the raw pointers aren't safe. We already have this in tons of places today with RenderViewHost routing pairs, so I don't see this as anything new.. 

I agree these should be routing pairs instead of pointers, and that this solves the validity check issue.

[John Abd-El-Malek]

It's not clear to me that all, or even most, of these 50 classes need to store per-RenderFrame functionality. I would expect that most of them are keeping per-page functionality and don't have objects that need to be torn down when a RenderView or RenderFrame is destructed.

What do we gain by providing no simple way for an api client to scope itself to just one RFH at a time? Reading back on the thread, my understanding is that our goals were to avoid exposing RFH like we exposed RVH, and generally to be conservative about adding new stuff to the content API. Is there another specific design objective here? I don't really understand how putting the RFH in a limited-time-only WCO getter helps us achieve the goal of hiding the RFH. But at the same time, I never experienced firsthand the pain that exposing RVH caused, so perhaps I just need to better understand what those problems were.

My motivation is avoiding adding any new abstractions until we're sure that the cost of new concepts that an embedder needs to be familiar with heavily outweight the cost without it. Without refactoring a lot of code, it's impossible to know that a given pattern is going to come up often, and how difficult it is to deal with it without new abstractions (i.e. the cost part of the equation). To use this example, maybe in BrowserMediaPlayerManagers, it would make things simpler, but if it's only that class or a few more that would benefit, then I don't think we should add new abstractions just for them.

[Charlie Reis]

Nick, Nasko, and I have been talking about this more, and Nick has come up with several more examples where a 1:1 frame relationship will be useful.  We've written up a rough proposal (open to feedback) for how to handle these cases here:

Frame Specific Clients outside Content API

It may turn out to be worthwhile to add things for other cases as well (per-process, all frames, etc), but there does seem to be at least 5-7 per-frame cases so far that this would benefit.

Charlie

[Nasko Oskov]

John and others,

Can you look over the document and comment on the proposal?

Thanks,

Nasko

[John Abd-El-Malek]

Apologies for the delay in responding.

So I've taken a look at this proposal, and been thinking about it for the last few hours. A few things:

-having this object and RenderFrameHost in the codebase is redundant. So I would not want us to add another concept without removing the existing one. Is it possible to completely remove RenderFrameHost, i.e. can the existing usages be switched to this new object?

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

[Fady Samuel]

I have an uninformed thought (please dismiss this if it doesn't make sense): Identifying that we're in a particular frame always starts in the content layer, right? Then some work is delegated to the chrome layer which then wants to call back into the content layer to complete the request.

Why not use callbacks? Why can't we have opaque callbacks to allow calling back to the appropriate frame from the content embedder?

Fady

[Nick Carter]

On Tue, Apr 15, 2014 at 12:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Apologies for the delay in responding.

So I've taken a look at this proposal, and been thinking about it for the last few hours. A few things:

-having this object and RenderFrameHost in the codebase is redundant. So I would not want us to add another concept without removing the existing one. Is it possible to completely remove RenderFrameHost, i.e. can the existing usages be switched to this new object?

With codesearch down it's hard to make declarations about all existing usages of RFH. But I don't currently see FrameMessagePipe as redundant to RenderFrameHost, any more than a WebContentsObserver is redundant to WebContents. One allows for interaction with the other.

To the extent that we need to traverse the frame tree (inside or outside of the content api), we'll still need some object in the codebase to represent a node's active frame, which is to say, a RenderFrameHost?

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

The problems that FrameMessagePipe proposes to solve, are the problems stated earlier in the thread about a proposed RFHO:

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

FrameMessagePipe gives clients a clearer place to put per-frame state and callbacks, without burdening WCOs with tracking and managing the lifetimes of individual per-frame objects.

[John Abd-El-Malek]

On Tue, Apr 15, 2014 at 3:00 PM, Nick Carter <ni...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 12:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Apologies for the delay in responding.

So I've taken a look at this proposal, and been thinking about it for the last few hours. A few things:

-having this object and RenderFrameHost in the codebase is redundant. So I would not want us to add another concept without removing the existing one. Is it possible to completely remove RenderFrameHost, i.e. can the existing usages be switched to this new object?

With codesearch down it's hard to make declarations about all existing usages of RFH. But I don't currently see FrameMessagePipe as redundant to RenderFrameHost, any more than a WebContentsObserver is redundant to WebContents.

I don't think it's the same comparison, because a class can't filter IPCs (or interesting events) from a WebContents alone.

 

One allows for interaction with the other.

To the extent that we need to traverse the frame tree (inside or outside of the content api),

Well, let's focus on outside content when discussing the content api. Internally, we can do whatever we want.

we'll still need some object in the codebase to represent a node's active frame, which is to say, a RenderFrameHost?

Inside content yes we need stuff. Outside, currently we have a small RenderFrameHost. As we agreed before, if we find ways to eliminate that everyone would be happy.

 

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

The problems that FrameMessagePipe proposes to solve, are the problems stated earlier in the thread about a proposed RFHO:

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

FrameMessagePipe gives clients a clearer place to put per-frame state and callbacks, without burdening WCOs with tracking and managing the lifetimes of individual per-frame objects.

Yes, but at the cost of having something that duplicates storing RenderFrameHost in a map, which is not much effort.

It may be the case that it's very clear to everyone that we absolutely need something like this after a lot of refactoring. So far, I haven't seen that. I am pretty conservative with adding new concepts to the content api, which is why I like to find overwhelming evidence that we need something (based on having many refactored pieces of code that have non-trivial bookeeping, say) before adding it.

[Charlie Reis]

On Tue, Apr 15, 2014 at 4:29 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 3:00 PM, Nick Carter <ni...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 12:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Apologies for the delay in responding.

So I've taken a look at this proposal, and been thinking about it for the last few hours. A few things:

-having this object and RenderFrameHost in the codebase is redundant. So I would not want us to add another concept without removing the existing one. Is it possible to completely remove RenderFrameHost, i.e. can the existing usages be switched to this new object?

To be clearer on this point, FrameMessagePipe is meant as a replacement for RenderFrameHostObserver (not RenderFrameHost), in such a way that it doesn't get a bunch of other observer functions added to it.  It's only for send/receive operations with a specific frame.

 

With codesearch down it's hard to make declarations about all existing usages of RFH. But I don't currently see FrameMessagePipe as redundant to RenderFrameHost, any more than a WebContentsObserver is redundant to WebContents.

I don't think it's the same comparison, because a class can't filter IPCs (or interesting events) from a WebContents alone.

 

One allows for interaction with the other.

To the extent that we need to traverse the frame tree (inside or outside of the content api),

Well, let's focus on outside content when discussing the content api. Internally, we can do whatever we want.

we'll still need some object in the codebase to represent a node's active frame, which is to say, a RenderFrameHost?

Inside content yes we need stuff. Outside, currently we have a small RenderFrameHost. As we agreed before, if we find ways to eliminate that everyone would be happy.

 

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

The problems that FrameMessagePipe proposes to solve, are the problems stated earlier in the thread about a proposed RFHO:

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

FrameMessagePipe gives clients a clearer place to put per-frame state and callbacks, without burdening WCOs with tracking and managing the lifetimes of individual per-frame objects.

Yes, but at the cost of having something that duplicates storing RenderFrameHost in a map, which is not much effort.

It may be the case that it's very clear to everyone that we absolutely need something like this after a lot of refactoring. So far, I haven't seen that. I am pretty conservative with adding new concepts to the content api, which is why I like to find overwhelming evidence that we need something (based on having many refactored pieces of code that have non-trivial bookeeping, say) before adding it.

Here's our concern: it's very easy to end up converting a bunch of clients to use their own RFH maps and do their own filtering and have them all be broken.  There appears to be a concrete example of this in the recently converted UIThreadExtensionFunction::RenderHostTracker:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser/extension_function.cc&l=34

The OnMessageReceived for this class sends any IPC message from the whole WebContents to the extension function, without filtering it down to the RVH or RFH it's associated with.  The extension function that processes the message (PageCaptureSaveAsMHTMLFunction::OnMessageReceived) doesn't filter it either.  That means other cross-process frames or pending RVHs could send extension messages and have them processed.

On top of that, Nick listed several cases at risk of similar bugs with potential security implications, if we continue to push that pattern:

https://docs.google.com/a/chromium.org/spreadsheets/d/1_q9R0aNehnL4HBS0r8L8ZggbJpE3kCLzDNYrxcoV8qk/edit#gid=0

To me, this is pretty strong evidence that an API that can avoid these bugs is worthwhile.  It seems preferable to hoping all the converted code gets something subtle like this right each time, and preferable to having to revisit all the work we're doing at the moment to find more cases of these bugs.

Charlie

[John Abd-El-Malek]

On Tue, Apr 15, 2014 at 5:55 PM, Charlie Reis <cr...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 4:29 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 3:00 PM, Nick Carter <ni...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 12:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Apologies for the delay in responding.

So I've taken a look at this proposal, and been thinking about it for the last few hours. A few things:

-having this object and RenderFrameHost in the codebase is redundant. So I would not want us to add another concept without removing the existing one. Is it possible to completely remove RenderFrameHost, i.e. can the existing usages be switched to this new object?

To be clearer on this point, FrameMessagePipe is meant as a replacement for RenderFrameHostObserver (not RenderFrameHost), in such a way that it doesn't get a bunch of other observer functions added to it.  It's only for send/receive operations with a specific frame.

Well, it's basically a RenderFrameHostObserver, just with no other methods. Given the experience with RenderViewHostObserver, which also started this small, my hunch would be that it would grow. Using the same logic below, instead of putting methods on WebContents that a few classes need to dispatch to a per-frame object (i.e. whatever implements FMP), then callbacks would be moved, or duplicated, in WCO and FMP/RFHO.

Even ignoring that though, the API as it is is just a combination of IPC::Sender and IPC::Listener. The first part is a duplicate of RFH, while the second part is a duplicate of a simple map.

 

With codesearch down it's hard to make declarations about all existing usages of RFH. But I don't currently see FrameMessagePipe as redundant to RenderFrameHost, any more than a WebContentsObserver is redundant to WebContents.

I don't think it's the same comparison, because a class can't filter IPCs (or interesting events) from a WebContents alone.

 

One allows for interaction with the other.

To the extent that we need to traverse the frame tree (inside or outside of the content api),

Well, let's focus on outside content when discussing the content api. Internally, we can do whatever we want.

we'll still need some object in the codebase to represent a node's active frame, which is to say, a RenderFrameHost?

Inside content yes we need stuff. Outside, currently we have a small RenderFrameHost. As we agreed before, if we find ways to eliminate that everyone would be happy.

 

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

The problems that FrameMessagePipe proposes to solve, are the problems stated earlier in the thread about a proposed RFHO:

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

FrameMessagePipe gives clients a clearer place to put per-frame state and callbacks, without burdening WCOs with tracking and managing the lifetimes of individual per-frame objects.

Yes, but at the cost of having something that duplicates storing RenderFrameHost in a map, which is not much effort.

It may be the case that it's very clear to everyone that we absolutely need something like this after a lot of refactoring. So far, I haven't seen that. I am pretty conservative with adding new concepts to the content api, which is why I like to find overwhelming evidence that we need something (based on having many refactored pieces of code that have non-trivial bookeeping, say) before adding it.

Here's our concern: it's very easy to end up converting a bunch of clients to use their own RFH maps and do their own filtering and have them all be broken.  There appears to be a concrete example of this in the recently converted UIThreadExtensionFunction::RenderHostTracker:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser/extension_function.cc&l=34

The OnMessageReceived for this class sends any IPC message from the whole WebContents to the extension function, without filtering it down to the RVH or RFH it's associated with.  The extension function that processes the message (PageCaptureSaveAsMHTMLFunction::OnMessageReceived) doesn't filter it either.  That means other cross-process frames or pending RVHs could send extension messages and have them processed.

To be fair, the pending RVH case exists now. A solution that exposes the RFH (i.e. OnMessageReceived(msg, frame) ) would also solve this.

For this specific example, ExtensionFunction isn't really working with frames yet. There was unreachable code that used it, so instead of deleting it (since the authors said it might be used in the future), I consciously put in the starting bits for RFH support, knowing that it's broken, to keep things compiling. When it comes to actually converting the extension code to use RenderFrames, any brokeness here would quickly be found, and ExtensionFunction and friends will be properly fixed up.

[Stanislas Polu]

Hi!

I've been following with great interest this discussion and wanted to give an external observer's (and also quite novice) point of vue on this:

It's my understanding that exposing a RFH-type object exposes the Content API user to the risk of keeping a reference to an object that represents a not yet ready or an already reclaimed frame. The proposal of a MessageObserverFactory::Create() called with a frame identifier solves this problem by exposing directly the Messaging layer which solves the former problem.

Only, exposing a MessageObserverFactory is equivalent to me to directly exposing implementation details. As far as I understand, there are messages in Content mainly because of the multi-process architecture and hence the IPC communication layer. But that's implementation specific, isn't it?

As far as I'm concerned, I'd rather add methods to the WebContentsDelegate to report frame states based on frame ids and have all APIs that are Frame specific on the WebContents directly and requiring a specific frame id in their signature. Most of these APIs would end up being asynchronous I guess, which is "verbose", but that's at least hiding the implementation details of Content. (If the frame does not exist anymore, theses calls could even be ignored altogether as the user must have had implemented WebContentsDelegate to get to know the frame ids, so it must have had been informed of the latest state of the frame)

Hope it somehow helps the discussion?

Best,

-stan

To unsubscribe from this group and stop receiving emails from it, send an email to site-isolation-...@chromium.org.

[Charlie Reis]

Thanks for the input Stan.  (John, replies to your note are below.)

On Wed, Apr 16, 2014 at 10:10 AM, Stanislas Polu <polu.st...@gmail.com> wrote:

Hi!

I've been following with great interest this discussion and wanted to give an external observer's (and also quite novice) point of vue on this:

It's my understanding that exposing a RFH-type object exposes the Content API user to the risk of keeping a reference to an object that represents a not yet ready or an already reclaimed frame. The proposal of a MessageObserverFactory::Create() called with a frame identifier solves this problem by exposing directly the Messaging layer which solves the former problem.

Only, exposing a MessageObserverFactory is equivalent to me to directly exposing implementation details. As far as I understand, there are messages in Content mainly because of the multi-process architecture and hence the IPC communication layer. But that's implementation specific, isn't it?

Content isn't trying to hide the multi-process architecture, though.  Content clients are free to add support for new IPC messages in both the browser and the renderer, such as the extension logic.  That's the main case we're trying to address with either FrameMessagePipe or OnMessageReceived(msg, frame).

 

As far as I'm concerned, I'd rather add methods to the WebContentsDelegate to report frame states based on frame ids and have all APIs that are Frame specific on the WebContents directly and requiring a specific frame id in their signature. Most of these APIs would end up being asynchronous I guess, which is "verbose", but that's at least hiding the implementation details of Content. (If the frame does not exist anymore, theses calls could even be ignored altogether as the user must have had implemented WebContentsDelegate to get to know the frame ids, so it must have had been informed of the latest state of the frame)

Hope it somehow helps the discussion?

Best,

-stan

On Wed, Apr 16, 2014 at 9:39 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 5:55 PM, Charlie Reis <cr...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 4:29 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 3:00 PM, Nick Carter <ni...@chromium.org> wrote:

On Tue, Apr 15, 2014 at 12:59 PM, John Abd-El-Malek <j...@chromium.org> wrote:

Apologies for the delay in responding.

So I've taken a look at this proposal, and been thinking about it for the last few hours. A few things:

-having this object and RenderFrameHost in the codebase is redundant. So I would not want us to add another concept without removing the existing one. Is it possible to completely remove RenderFrameHost, i.e. can the existing usages be switched to this new object?

To be clearer on this point, FrameMessagePipe is meant as a replacement for RenderFrameHostObserver (not RenderFrameHost), in such a way that it doesn't get a bunch of other observer functions added to it.  It's only for send/receive operations with a specific frame.

Well, it's basically a RenderFrameHostObserver, just with no other methods. Given the experience with RenderViewHostObserver, which also started this small, my hunch would be that it would grow. Using the same logic below, instead of putting methods on WebContents that a few classes need to dispatch to a per-frame object (i.e. whatever implements FMP), then callbacks would be moved, or duplicated, in WCO and FMP/RFHO.

Yes, it's RFHO named in such a way to deter having it grow.

 

Even ignoring that though, the API as it is is just a combination of IPC::Sender and IPC::Listener. The first part is a duplicate of RFH, while the second part is a duplicate of a simple map.

Yes, but it takes care of details that clients are currently getting wrong.  

 

 

With codesearch down it's hard to make declarations about all existing usages of RFH. But I don't currently see FrameMessagePipe as redundant to RenderFrameHost, any more than a WebContentsObserver is redundant to WebContents.

I don't think it's the same comparison, because a class can't filter IPCs (or interesting events) from a WebContents alone.

 

One allows for interaction with the other.

To the extent that we need to traverse the frame tree (inside or outside of the content api),

Well, let's focus on outside content when discussing the content api. Internally, we can do whatever we want.

we'll still need some object in the codebase to represent a node's active frame, which is to say, a RenderFrameHost?

Inside content yes we need stuff. Outside, currently we have a small RenderFrameHost. As we agreed before, if we find ways to eliminate that everyone would be happy.

 

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

The problems that FrameMessagePipe proposes to solve, are the problems stated earlier in the thread about a proposed RFHO:

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

FrameMessagePipe gives clients a clearer place to put per-frame state and callbacks, without burdening WCOs with tracking and managing the lifetimes of individual per-frame objects.

Yes, but at the cost of having something that duplicates storing RenderFrameHost in a map, which is not much effort.

It may be the case that it's very clear to everyone that we absolutely need something like this after a lot of refactoring. So far, I haven't seen that. I am pretty conservative with adding new concepts to the content api, which is why I like to find overwhelming evidence that we need something (based on having many refactored pieces of code that have non-trivial bookeeping, say) before adding it.

Here's our concern: it's very easy to end up converting a bunch of clients to use their own RFH maps and do their own filtering and have them all be broken.  There appears to be a concrete example of this in the recently converted UIThreadExtensionFunction::RenderHostTracker:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser/extension_function.cc&l=34

The OnMessageReceived for this class sends any IPC message from the whole WebContents to the extension function, without filtering it down to the RVH or RFH it's associated with.  The extension function that processes the message (PageCaptureSaveAsMHTMLFunction::OnMessageReceived) doesn't filter it either.  That means other cross-process frames or pending RVHs could send extension messages and have them processed.

To be fair, the pending RVH case exists now.

Yes, and something like FMP would fix it today for all of its clients without requiring them to maintain a map and remember to check it.

 

A solution that exposes the RFH (i.e. OnMessageReceived(msg, frame) ) would also solve this.

Yes, OnMessageReceived(msg, frame) would make it possible for the client to solve the problem using a map, but it doesn't help with clients that skip the filtering, as in UIThreadExtensionFunction.

 

For this specific example, ExtensionFunction isn't really working with frames yet. There was unreachable code that used it, so instead of deleting it (since the authors said it might be used in the future), I consciously put in the starting bits for RFH support, knowing that it's broken, to keep things compiling. When it comes to actually converting the extension code to use RenderFrames, any brokeness here would quickly be found, and ExtensionFunction and friends will be properly fixed up.

Right now RenderHostTracker can't do the filtering at all, because it doesn't know which RFH sent the message (whether we're talking about subframes or pending top-level frames).

In the interest of making progress towards having something that people like Xiaohan can adopt, let's try to get one of these solutions landed soon.  Obviously Nick, Nasko, and I would prefer something that makes it easier to filter down to the right frame of interest, but allowing the clients to manually filter to the right frame is better than nothing.

Charlie

[John Abd-El-Malek]

This isn't foolproof. When we had RenderViewHostObservers, there would sometimes be RVHOs and WCOs for the same feature. An RVHO would call to the WCO or other feature code, and they could also end up in code which calls WC->GetRenderViewHost.

 

 

With codesearch down it's hard to make declarations about all existing usages of RFH. But I don't currently see FrameMessagePipe as redundant to RenderFrameHost, any more than a WebContentsObserver is redundant to WebContents.

I don't think it's the same comparison, because a class can't filter IPCs (or interesting events) from a WebContents alone.

 

One allows for interaction with the other.

To the extent that we need to traverse the frame tree (inside or outside of the content api),

Well, let's focus on outside content when discussing the content api. Internally, we can do whatever we want.

we'll still need some object in the codebase to represent a node's active frame, which is to say, a RenderFrameHost?

Inside content yes we need stuff. Outside, currently we have a small RenderFrameHost. As we agreed before, if we find ways to eliminate that everyone would be happy.

 

-I may be missing something, but it's not clear to me how this is different from RenderFrameHost? perhaps a trimmed down version (and trimming RFH is always good)?

The problems that FrameMessagePipe proposes to solve, are the problems stated earlier in the thread about a proposed RFHO:

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

FrameMessagePipe gives clients a clearer place to put per-frame state and callbacks, without burdening WCOs with tracking and managing the lifetimes of individual per-frame objects.

Yes, but at the cost of having something that duplicates storing RenderFrameHost in a map, which is not much effort.

It may be the case that it's very clear to everyone that we absolutely need something like this after a lot of refactoring. So far, I haven't seen that. I am pretty conservative with adding new concepts to the content api, which is why I like to find overwhelming evidence that we need something (based on having many refactored pieces of code that have non-trivial bookeeping, say) before adding it.

Here's our concern: it's very easy to end up converting a bunch of clients to use their own RFH maps and do their own filtering and have them all be broken.  There appears to be a concrete example of this in the recently converted UIThreadExtensionFunction::RenderHostTracker:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser/extension_function.cc&l=34

The OnMessageReceived for this class sends any IPC message from the whole WebContents to the extension function, without filtering it down to the RVH or RFH it's associated with.  The extension function that processes the message (PageCaptureSaveAsMHTMLFunction::OnMessageReceived) doesn't filter it either.  That means other cross-process frames or pending RVHs could send extension messages and have them processed.

To be fair, the pending RVH case exists now.

Yes, and something like FMP would fix it today for all of its clients without requiring them to maintain a map and remember to check it.

This is one advantage, yes, but there are other disadvantages that I've tried to list before (maybe not very convincingly :) ). The reason I'm pushing back here is because of my experience having both RenderViewHostObservers and WebContentsObservers in the code. I initially thought having both would be clearer, and after adding RVHO I converted a bunch of code to it. I ended up regretting this after seeing how the code turned out. There was state scattered across different objects. We had issues with duplicate callbacks on RVHO and WCO, and WCOs having to proxy callbacks to the right RVHO. Even if we keep the new class only for filtering IPCs, then we have to choose whether we put state there or not. If we don't, then this class would just be calling out to the relevant WCO or other feature code, and this is equivalent to having WCO dispatch the IPC with the RFH pointer handy. If it has state and other logic, then it will need to get notifications from WCO callbacks, and then we have the problem of scattered state and lots of proxying of methods.

 

A solution that exposes the RFH (i.e. OnMessageReceived(msg, frame) ) would also solve this.

Yes, OnMessageReceived(msg, frame) would make it possible for the client to solve the problem using a map, but it doesn't help with clients that skip the filtering, as in UIThreadExtensionFunction.

Since UIThreadExtensionFunction isn't really started, and was changed just to avoid compilation failure, it's not a good example to use..

One way to make it harder to mess up would be to make IPCs from RenderFrameHost only go to the OnMsgRcvd(msg, frame) call. i.e. not fallback to OnMsgRcvd(msg) for these IPCs.

 

For this specific example, ExtensionFunction isn't really working with frames yet. There was unreachable code that used it, so instead of deleting it (since the authors said it might be used in the future), I consciously put in the starting bits for RFH support, knowing that it's broken, to keep things compiling. When it comes to actually converting the extension code to use RenderFrames, any brokeness here would quickly be found, and ExtensionFunction and friends will be properly fixed up.

Right now RenderHostTracker can't do the filtering at all, because it doesn't know which RFH sent the message (whether we're talking about subframes or pending top-level frames).

Sure, that's just because we haven't actually started converting that code yet. When that happens, or another piece that needs to know the RFH, that would be the impetus to expose the necessary information.

In the interest of making progress towards having something that people like Xiaohan can adopt, let's try to get one of these solutions landed soon.  Obviously Nick, Nasko, and I would prefer something that makes it easier to filter down to the right frame of interest, but allowing the clients to manually filter to the right frame is better than nothing.

Switching users between OnMsgRcv(msg, frame) and a separate object wouldn't be much work; let's go with OnMsgRcvd now, and if we find that it's error prone in practice, we can convert it later.

[Nick Carter]

This expression of the troubles with multiple observers is much clearer, so thanks, and I've come around to see your point much better now. One thing I understand and agree with now, is that the frame-focused classes really are a minority of the cases today.

Since we do have a short list of the frame-focused WCO implementations now, it might make sense to prioritize converting some of those, like the autofill or password manager cases. These will let us see how the privilege enforcement checks could be structured in practice with the API we have.

A solution that exposes the RFH (i.e. OnMessageReceived(msg, frame) ) would also solve this.

Yes, OnMessageReceived(msg, frame) would make it possible for the client to solve the problem using a map, but it doesn't help with clients that skip the filtering, as in UIThreadExtensionFunction.

Since UIThreadExtensionFunction isn't really started, and was changed just to avoid compilation failure, it's not a good example to use..

One way to make it harder to mess up would be to make IPCs from RenderFrameHost only go to the OnMsgRcvd(msg, frame) call. i.e. not fallback to OnMsgRcvd(msg) for these IPCs.

Trying to clarifying this idea. If IPCs from RFH only go to the two-parameter version, then which IPCs are left going to OnMsgRcvd(msg)? Is it just existing messages routed to a RVH -- which set we expect to dwindle to zero?

 

For this specific example, ExtensionFunction isn't really working with frames yet. There was unreachable code that used it, so instead of deleting it (since the authors said it might be used in the future), I consciously put in the starting bits for RFH support, knowing that it's broken, to keep things compiling. When it comes to actually converting the extension code to use RenderFrames, any brokeness here would quickly be found, and ExtensionFunction and friends will be properly fixed up.

Right now RenderHostTracker can't do the filtering at all, because it doesn't know which RFH sent the message (whether we're talking about subframes or pending top-level frames).

Sure, that's just because we haven't actually started converting that code yet. When that happens, or another piece that needs to know the RFH, that would be the impetus to expose the necessary information.

In the interest of making progress towards having something that people like Xiaohan can adopt, let's try to get one of these solutions landed soon.  Obviously Nick, Nasko, and I would prefer something that makes it easier to filter down to the right frame of interest, but allowing the clients to manually filter to the right frame is better than nothing.

Switching users between OnMsgRcv(msg, frame) and a separate object wouldn't be much work; let's go with OnMsgRcvd now, and if we find that it's error prone in practice, we can convert it later.

I'm on board with this as a way forward. Who should write the patch?

Some issues that may come up quickly:

 - It would seem that IPC_MESSAGE_FORWARD and IPC_MESSAGE_HANDLER aren't very useful inside of an implementation of OnMsgRcv(msg, frame), because it's not clear how to pass |frame| to the forwardee. The proposed GetRFHForDispatchingIPC() helps at least with the _HANDLER case. Do we need variants of these macros that would let us forward an extra parameter?

 - Does it make sense to remove or rename WCO::Send() (e.g. to SendToMainView) sooner rather than later? Will WCO ultimately implement neither the IPC::Sender nor IPC::Listener interface?

[John Abd-El-Malek]

Ok glad to hear, sorry for not being clear enough earlier.

 

Since we do have a short list of the frame-focused WCO implementations now, it might make sense to prioritize converting some of those, like the autofill or password manager cases. These will let us see how the privilege enforcement checks could be structured in practice with the API we have.

It sounds like the media stuff that started this thread will need this soon, so that could be the motivation to do it in the near term.

 

A solution that exposes the RFH (i.e. OnMessageReceived(msg, frame) ) would also solve this.

Yes, OnMessageReceived(msg, frame) would make it possible for the client to solve the problem using a map, but it doesn't help with clients that skip the filtering, as in UIThreadExtensionFunction.

Since UIThreadExtensionFunction isn't really started, and was changed just to avoid compilation failure, it's not a good example to use..

One way to make it harder to mess up would be to make IPCs from RenderFrameHost only go to the OnMsgRcvd(msg, frame) call. i.e. not fallback to OnMsgRcvd(msg) for these IPCs.

Trying to clarifying this idea. If IPCs from RFH only go to the two-parameter version, then which IPCs are left going to OnMsgRcvd(msg)? Is it just existing messages routed to a RVH -- which set we expect to dwindle to zero?

 

For this specific example, ExtensionFunction isn't really working with frames yet. There was unreachable code that used it, so instead of deleting it (since the authors said it might be used in the future), I consciously put in the starting bits for RFH support, knowing that it's broken, to keep things compiling. When it comes to actually converting the extension code to use RenderFrames, any brokeness here would quickly be found, and ExtensionFunction and friends will be properly fixed up.

Right now RenderHostTracker can't do the filtering at all, because it doesn't know which RFH sent the message (whether we're talking about subframes or pending top-level frames).

Sure, that's just because we haven't actually started converting that code yet. When that happens, or another piece that needs to know the RFH, that would be the impetus to expose the necessary information.

In the interest of making progress towards having something that people like Xiaohan can adopt, let's try to get one of these solutions landed soon.  Obviously Nick, Nasko, and I would prefer something that makes it easier to filter down to the right frame of interest, but allowing the clients to manually filter to the right frame is better than nothing.

Switching users between OnMsgRcv(msg, frame) and a separate object wouldn't be much work; let's go with OnMsgRcvd now, and if we find that it's error prone in practice, we can convert it later.

I'm on board with this as a way forward. Who should write the patch?

Some issues that may come up quickly:

 - It would seem that IPC_MESSAGE_FORWARD and IPC_MESSAGE_HANDLER aren't very useful inside of an implementation of OnMsgRcv(msg, frame), because it's not clear how to pass |frame| to the forwardee. The proposed GetRFHForDispatchingIPC() helps at least with the _HANDLER case. Do we need variants of these macros that would let us forward an extra parameter?

Both options sound fine to me. Off the top of my head, I don't know how much effort it'll be to update all the macros to support an extra parameter.

 

 - Does it make sense to remove or rename WCO::Send() (e.g. to SendToMainView) sooner rather than later?

My hunch is that it's too early to do something like this: there are a ton of call sites, and the distinction of that vs RFH::Send might not be clear to most devs.

 

Will WCO ultimately implement neither the IPC::Sender nor IPC::Listener interface?

Definitely not IPC::Sender. For Listener, it depends on how we expose the source RFH.

[Nick Carter]

For those following this discussion, mnaganov is proceeding with an implementation here: https://codereview.chromium.org/253013002/ .

[Xiaohan Wang (王消寒)]

That's exciting! To be honest, I wasn't following this thread closely (a lot of discussions!). So is there a quick guide or doc about how to use this new implementation?

[Mikhail Naganov]

Please correct me if I'm wrong. This is how I understand the idea.

If there is a WebContentsObserver ancestor that acts a "manager" for a set of per-frame objects on the browser side (taking Xiaohan's use case, a MediaPlayer implementation, so there is 1 MediaPlayer on browser side per each RenderFrame on the renderer side), then WebContents will be passing a pointer to a RFH to the main message handler of the "manager", and "manager's" message callbacks will look like this:

void BrowserMediaPlayerManager::OnBar(RenderFrameHost* rfh, X param1, Y param2, ...)

Thus, it will be possible for BrowserMediaPlayerManager to pick up the right MediaPlayer.

But this also means, that BrowserMediaPlayerManager should be listening to RenderFrameCreated / RenderFrameDestroyed in order to keep a live mapping from RFHs to MediaPlayer instances.

What I don't know, is how can one migrate from using RenderViewHosts to using RenderFrameHosts (that has been mentioned in the Xiaohan's original email). But this is out of the scope of my patch.

[Xiaohan Wang (王消寒)]

Thanks Mikhail!

I think this will work for me. CIL.

On Tue, Apr 29, 2014 at 1:31 PM, Mikhail Naganov <mnag...@chromium.org> wrote:

Please correct me if I'm wrong. This is how I understand the idea.

If there is a WebContentsObserver ancestor that acts a "manager" for a set of per-frame objects on the browser side (taking Xiaohan's use case, a MediaPlayer implementation, so there is 1 MediaPlayer on browser side per each RenderFrame on the renderer side), then WebContents will be passing a pointer to a RFH to the main message handler of the "manager", and "manager's" message callbacks will look like this:

void BrowserMediaPlayerManager::OnBar(RenderFrameHost* rfh, X param1, Y param2, ...)

Actually we can have multiple MediaPlayers in a RenderFrame :) But we are keeping our own |player_id| so I can still manage them. Being able to send IPCs to the correct RenderFrame is what I need. 

Thus, it will be possible for BrowserMediaPlayerManager to pick up the right MediaPlayer.

But this also means, that BrowserMediaPlayerManager should be listening to RenderFrameCreated / RenderFrameDestroyed in order to keep a live mapping from RFHs to MediaPlayer instances.

After we migrate the MediaPlayer from RenderView to RenderFrame, I assume the MediaPlayer will be destroyed before the RenderFrameDestroyed() is called. Is that correct? In this case, I don't see any issues.

What I don't know, is how can one migrate from using RenderViewHosts to using RenderFrameHosts (that has been mentioned in the Xiaohan's original email). But this is out of the scope of my patch. 

Yeah. Is there an ETA when this will happen? 

[Mikhail Naganov]

On Tue, Apr 29, 2014 at 10:00 PM, Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Thanks Mikhail!

I think this will work for me. CIL.

On Tue, Apr 29, 2014 at 1:31 PM, Mikhail Naganov <mnag...@chromium.org> wrote:

Please correct me if I'm wrong. This is how I understand the idea.

If there is a WebContentsObserver ancestor that acts a "manager" for a set of per-frame objects on the browser side (taking Xiaohan's use case, a MediaPlayer implementation, so there is 1 MediaPlayer on browser side per each RenderFrame on the renderer side), then WebContents will be passing a pointer to a RFH to the main message handler of the "manager", and "manager's" message callbacks will look like this:

void BrowserMediaPlayerManager::OnBar(RenderFrameHost* rfh, X param1, Y param2, ...)

Actually we can have multiple MediaPlayers in a RenderFrame :) But we are keeping our own |player_id| so I can still manage them. Being able to send IPCs to the correct RenderFrame is what I need. 

Thus, it will be possible for BrowserMediaPlayerManager to pick up the right MediaPlayer.

But this also means, that BrowserMediaPlayerManager should be listening to RenderFrameCreated / RenderFrameDestroyed in order to keep a live mapping from RFHs to MediaPlayer instances.

After we migrate the MediaPlayer from RenderView to RenderFrame, I assume the MediaPlayer will be destroyed before the RenderFrameDestroyed() is called. Is that correct? In this case, I don't see any issues.

In general that depends on the ownership model. What I was talking about is the case when on the browser side the Manager maintains a 1:1 relationship between RenderFrameHosts and its own per-frame objects by listening to RenderFrameCreated / RenderFrameDestroyed. This is how it works in Android Java Bridge. I'm absolutely unaware of the actual MediaPlayer model. 

 

What I don't know, is how can one migrate from using RenderViewHosts to using RenderFrameHosts (that has been mentioned in the Xiaohan's original email). But this is out of the scope of my patch. 

Yeah. Is there an ETA when this will happen? 

I'm working on the patch, but I can't predict what problems will I encounter and how long will it take to fix them, sorry.

[Xiaohan Wang (王消寒)]

Hello,

Now https://codereview.chromium.org/253013002/ is landed. I finished my BrowserMediaPlayerManager CL and it's working! Thank you all for the discussions and hard work.

During implementation, I found a small issue though. It seems a bit weird to use the new |render_frame_host| parameter in OnMessageReceived() function. Macros like IPC_MESSAGE_HANDLER won't pass render_frame_host to the handlers for me. In my CL, I use a class member variable (current_cdm_routing_id_) to remember the routing_id of the current IPC message being processed. It's working well but I feel it's a bit hacky:

bool BrowserMediaPlayerManager::OnMessageReceived(

    const IPC::Message& message,

    RenderFrameHost* render_frame_host) {

  current_cdm_routing_id_ = render_frame_host->GetRoutingID();

  DCHECK_NE(MSG_ROUTING_NONE, current_cdm_rounting_id_);

  bool handled = true;

  IPC_BEGIN_MESSAGE_MAP(BrowserMediaPlayerManager, message)

    IPC_MESSAGE_HANDLER(CdmHostMsg_InitializeCdm, OnInitializeCdm)

    ...

    IPC_MESSAGE_HANDLER(CdmHostMsg_DestroyCdm, OnDestroyCdm)

    IPC_MESSAGE_UNHANDLED(handled = false)

  IPC_END_MESSAGE_MAP()

  current_cdm_routing_id_ = MSG_ROUTING_NONE;

  return handled;

}

Is there a plan to support handling IPC messages with  |render_frame_host| or render frame routing ID in a more generic way? Does it make sense to add something like a new IPC_MESSAGE_HANDLER_WITH_ROUTING_ID so that the routing ID will be passed to the handlers automatically?

Cheers,

Xiaohan

[Mikhail Naganov]

One approach that is possible with existing message handling macros is to maintain a map from RFHs to helper objects and use IPC_MESSAGE_FORWARD and IPC_MESSAGE_FORWARD_DELAY_REPLY (for sync messages), so your code can look like this (pseudocode):

OnMessageReceived(message, rfh) {

  if ( !(rfh in mapping) )

     return

  helper = mapping[rfh];

  IPC_BEGIN_MESSAGE_MAP

      IPC_MESSAGE_FORWARD(HostMsg, helper, Helper::OnMsg)

  IPC_END_MESSAGE_MAP

}

(thanks to Nick for suggesting this for my case!)

Also, as John has suggested here (https://codereview.chromium.org/253013002/#msg6), we can extend IPC_MESSAGE_HANDLER macros to pass RFH to handler callbacks.

[Xiaohan Wang (王消寒)]

Thanks Mikhail,

Currently RendererMediaPlayerManager is still a RenderViewObserver. In the next CL I'll move it to use RendererFrameObserver, at which point I can make BrowserMediaPlayerManager a per RenderFrame object. Then I can apply what you suggested (BMPM will be the "helper").

Thanks for the suggestion!

Xiaohan

[Nasko Oskov]

Xiaohan,

I just want to point out one concern I see in the code snippet above. Routing ids for RenderFrameHost objects are process specific. Once we enable frames to be in different processes, you cannot rely only on the routing id for identifying RenderFrameHost objects, so I would suggest making this change now. The unique identifier is a combination of the process id and the routing id.

Thanks,

Nasko

[Xiaohan Wang (王消寒)]

Ah, I see. I am actually still using Send(render_host_routing_id, ....) in my CL, which won't work in your case either (and is hacky). I should find the correct RenderFrameHost, and use RenderFrameHost::Send() to send IPCs back to the RenderFrameObservers. I'll update my CL.

Also, I think I need to handle RenderFrameDeleted() in my CL as well.

Thanks!

[Mikhail Naganov]

Yes, you need to listen to RenderFrameCreated / Deleted to make sure you are tracking live RFHs.

A little heads-up about my patch -- it has been reverted during yesterday firefighting (for no good reason, actually), the revert CL is here: https://codereview.chromium.org/271863005, r269075. The revert of the revert was also  uploaded, but hasn't landed yet: https://codereview.chromium.org/275673003

[Mikhail Naganov]

It turns out, the revert of the revert has been created incorrectly, so I have created a re-land patch that also includes a small fix for "Search Google for this image": https://codereview.chromium.org/275873002/

[Xiaohan Wang (王消寒)]

No worries. I haver your CL patched locally so I can work on top of it.

[John Abd-El-Malek]

On Thu, May 8, 2014 at 1:34 AM, Mikhail Naganov <mnag...@chromium.org> wrote:

One approach that is possible with existing message handling macros is to maintain a map from RFHs to helper objects and use IPC_MESSAGE_FORWARD and IPC_MESSAGE_FORWARD_DELAY_REPLY (for sync messages), so your code can look like this (pseudocode):

OnMessageReceived(message, rfh) {

  if ( !(rfh in mapping) )

     return

  helper = mapping[rfh];

  IPC_BEGIN_MESSAGE_MAP

      IPC_MESSAGE_FORWARD(HostMsg, helper, Helper::OnMsg)

  IPC_END_MESSAGE_MAP

}

(thanks to Nick for suggesting this for my case!)

Also, as John has suggested here (https://codereview.chromium.org/253013002/#msg6), we can extend IPC_MESSAGE_HANDLER macros to pass RFH to handler callbacks.

btw I'm working on this now

[Xiaohan Wang (王消寒)]

+boliu, ycheo, who I discussed about this offline.

Hello,

While working on my RenderViewObserver -> RenderFrameObserver CL, I noticed that DidCommitCompositorFrame() call is dispatched to all RenderViewObservers. It's also dispatched to all swapped_out_frames_ through RenderWidget::DidCommitCompositorFrame().

I wonder that in the future, when we move all RenderViewObservers to RenderFrameObservers, does it make sense to dispatch this call to all RenderFrameObservers of all RenderFrameImpls? boliu@ and I actually looked at this. Currently RenderWidget doesn't have the full list of RenderFrameImpls. So there's no easy way to dispatch a call to all RenderFrameImpls :(

I am asking this because for AndroidWebView, there's no swapped out frames. But media players needs to listen to this call. I can manually register render frames with media players when AndroidWebView is used (similar to how we register swapped_out_frames_). But this seems a bit hacky.

I wonder what's the best way to do this? Any suggestions?

Thanks!

Xiaohan

[Nasko Oskov]

On Thu, May 22, 2014 at 9:43 PM, Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

+boliu, ycheo, who I discussed about this offline.

Hello,

While working on my RenderViewObserver -> RenderFrameObserver CL, I noticed that DidCommitCompositorFrame() call is dispatched to all RenderViewObservers. It's also dispatched to all swapped_out_frames_ through RenderWidget::DidCommitCompositorFrame().

In general, swapped out objects do their rendering in a different process. I'm not sure long term whether notifying swapped out frames will still exist, but Ken can confirm, as he is the expert in that area.

 

I wonder that in the future, when we move all RenderViewObservers to RenderFrameObservers, does it make sense to dispatch this call to all RenderFrameObservers of all RenderFrameImpls? boliu@ and I actually looked at this. Currently RenderWidget doesn't have the full list of RenderFrameImpls. So there's no easy way to dispatch a call to all RenderFrameImpls :(

In the future, there will be one RenderWidget for a set of frames in the same process. If RenderFrameImpls need to know about DidCommitCompositorFrame, it makes sense to support this.

 

I am asking this because for AndroidWebView, there's no swapped out frames. But media players needs to listen to this call. I can manually register render frames with media players when AndroidWebView is used (similar to how we register swapped_out_frames_). But this seems a bit hacky.

Why do you need to listen to swapped out objects? Those do not render in the current process and their compositor frames will come from the real frame in another process.

I'm asking this, because the notion of swapped out will only exist for top-level frames and will disappear as soon as it can. Those will be replaced by RenderFrameProxy/RenderFrameProxyHost and in general they should be an implementation detail not exposed (especially not outside of content).

[Xiaohan Wang (王消寒)]

On Fri, May 23, 2014 at 7:37 AM, Nasko Oskov <na...@chromium.org> wrote:

On Thu, May 22, 2014 at 9:43 PM, Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

+boliu, ycheo, who I discussed about this offline.

Hello,

While working on my RenderViewObserver -> RenderFrameObserver CL, I noticed that DidCommitCompositorFrame() call is dispatched to all RenderViewObservers. It's also dispatched to all swapped_out_frames_ through RenderWidget::DidCommitCompositorFrame().

In general, swapped out objects do their rendering in a different process. I'm not sure long term whether notifying swapped out frames will still exist, but Ken can confirm, as he is the expert in that area.

 

I wonder that in the future, when we move all RenderViewObservers to RenderFrameObservers, does it make sense to dispatch this call to all RenderFrameObservers of all RenderFrameImpls? boliu@ and I actually looked at this. Currently RenderWidget doesn't have the full list of RenderFrameImpls. So there's no easy way to dispatch a call to all RenderFrameImpls :(

In the future, there will be one RenderWidget for a set of frames in the same process. If RenderFrameImpls need to know about DidCommitCompositorFrame, it makes sense to support this.

This makes sense.

 

I am asking this because for AndroidWebView, there's no swapped out frames. But media players needs to listen to this call. I can manually register render frames with media players when AndroidWebView is used (similar to how we register swapped_out_frames_). But this seems a bit hacky.

Why do you need to listen to swapped out objects? Those do not render in the current process and their compositor frames will come from the real frame in another process.

I'm asking this, because the notion of swapped out will only exist for top-level frames and will disappear as soon as it can. Those will be replaced by RenderFrameProxy/RenderFrameProxyHost and in general they should be an implementation detail not exposed (especially not outside of content).

I don't care about swapped out frames. I was saying that currently only swapped out frames are listening to DidCommitCompositorFrame, which breaks AndroidWebView in my CL. What I really need is to let some other RenderFrameImpls listen to DidCommitCompositorFrame as well. My current approach is to have those frames register themselves with RenderWidget so that they get notified of DidCommitCompositorFrame. This is similar to what swapped out frames is doing. Does this approach make sense to you?

[Bo Liu]

In Xiaohan's patch, DidCommitCompositorFrame is exposed to RenderFrameObserver, which means all RenderFrames need to know about it.

Poking around RenderWidget, I'm not sure how to get the list of "all RenderFrames rendered by this RenderWidgetCompositor". It looks like the RenderFrame tree is kept in the browser side?

Xiaohan's current CL has RenderFrames register themselves in RenderWidget. Is that a good way to go, or do you have better recommendations?

...

[Message clipped]  

[Ken Buchanan]

I just chatted with Nasko about this to catch up on the discussion, and I think that makes sense. Your requirement is the same as what we needed for swapped out frames.

[Nasko Oskov]

I also posted some of the details on another thread of similar nature. If you are curious, just read the last email in this thread: https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/2Yq83p3IPe0/-XIgM2K3guMJ