Syncing "is_prerendering" between browser, renderer, and Mojo capability control

[Matt Falkenhagen]

Hi,

https://crbug.com/1185965 illustrates a problem where the browser sends an ActivateForPrerendering message to the renderer, but the document in the renderer does not think it is being prerendered so fails a DCHECK. (We do have an exception for `is_initial_empty_document_` before the DCHECK but it does not appear to completely work and is a bit awkward anyway.)

Currently we only tell the renderer that it is being prerendered in the CommitNavigation message, via NavigationParams::is_prerendering. But there are some cases like about:blank iframes, and maybe others, where the frame can be created before the CommitNavigation IPC is sent, so they can be live without knowing they are being prerendered.

It seems that to fix this, RenderFrameHostImpl should remember whether it sent a CommitNavigation with NavigationParams::is_prerendering. Then on activation, it only sends the Activation IPC if it previously sent the commit with prerendering.

However after playing around with this, I feel it exposes another problem: the document in the renderer can have access to the BrowserInterfaceBroker even before CommitNavigation. This is problematic because the BIB will have Mojo Capability Control applied, so if the renderer does not realize it is being prerendered it can unwittingly send a "kUnexpected" message which causes ReportBadMessage() to be called.

This is surprising: conventional knowledge is that the BIB is bound in DidCommitNavigation. However it turns out there are many callsites of BindBrowserInterfaceBrokerReceiver() other than that one.

For example, an about:blank iframe appears to get the broker by sending the receiver in a CreateChildFrame message:

#2 0x7fd0a1af0345 content::RenderFrameHostImpl::BindBrowserInterfaceBrokerReceiver()
#3 0x7fd0a1a05eb3 content::FrameTree::AddFrame()
#4 0x7fd0a1af2847 content::RenderFrameHostImpl::OnCreateChildFrame()
#5 0x7fd0a1af2a0b content::RenderFrameHostImpl::CreateChildFrame()

Thus it seems we are stuck. The renderer already has the BIB remote here, and it does not know it is being prerendered, so it can send unexpected messages.

Another option is for subframes to get the prerendering state from their parent whenever a new document is installed. Because we don't load cross-origin subframes, all the frames should be in the same process.

Even still, this doesn't look totally foolproof, because it appears main frames may be able to get a BIB via:

#2 0x7fd0a1af0345 content::RenderFrameHostImpl::BindBrowserInterfaceBrokerReceiver()
#3 0x7fd0a1b5d63f content::RenderViewHostImpl::CreateRenderView()
#4 0x7fd0a1ce9a08 content::WebContentsImpl::CreateRenderViewForRenderManager()
#5 0x7fd0a1b2fba6 content::RenderFrameHostManager::ReinitializeMainRenderFrame()
#6 0x7fd0a1b2ea95 content::RenderFrameHostManager::GetFrameHostForNavigation()
#7 0x7fd0a1b2dd4d content::RenderFrameHostManager::DidCreateNavigationRequest()
#8 0x7fd0a1a0a7c5 content::FrameTreeNode::CreatedNavigationRequest()
#9 0x7fd0a1ad7882 content::Navigator::Navigate()
#10 0x7fd0a1a94b0e content::NavigationControllerImpl::NavigateWithoutEntry()
#11 0x7fd0a1a9386e content::NavigationControllerImpl::LoadURLWithParams()

In this case, the BIB remote is sent via AgentSchedulingGroup::CreateView which ends up making a RenderFrameImpl in the renderer with a BIB remote. If it's possible to make prerendered main frames via this path, the renderer will not know it is being prerendered while it has a BIB remote.

I'm mildly optimistic that there is no way to prerender via that path, and getting the state from the parent should work.

But are there any thoughts on a great approach here?

Thanks,
Matt

[Kinuko Yasuda]

For subframes / about:blank cases my assumption is (optimistically) same that the frame can be created before navigation but usually navigation happens right after that (e.g. in HTMLFrameOwnerElement::LoadOrRedirectSubframe) which should make CommitNavigation at least except for about:blank cases, so inheriting the parent's one seems to work.  If others can confirm or just correct me it'd be great though.

For main frames: it looks like this one is for somewhat similar case where we create a frame (as the destination of the navigation) before navigation really takes place.  Could the BIB be used before the navigation commits?   Hope navigation / CSA people can also shed light here.

If so content::NavigationControllerImpl::LoadURLWithParams() is at least used in PrerenderHost::StartPrerendering() and we may need to propagate some parameter (i.e. is_for_prerendering or something...) via LoadURLParams and then up until AgentSchedulingGroup::CreateView and then CreateMainFrame.

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

[Lingqi Chi]

From MCC's point of view:

• Regarding the first case of "For example, an about:blank iframe appears to get the broker by sending the receiver in a CreateChildFrame message:
• That's why we start applying MojoBinderPolicies at the moment when a RFH instance is created. 
• For the prerendering state, their parent can teach them. (as FramePolicy does?
  
• Regarding the second case of binding BIB in CreateRenderView 
• Under the old architecture, i.e., prerender pages have their own WebContents, we noticed that for the main frame of prerendering page, BindBrowserInterfaceBrokerReceiver() is called twice.   (I'm not sure whether it is turn under the MPArch, but it would be easy to confirm.
• one is called when RenderView is created, the other is called in DidCommitNavigation()
• I feel it would still be safe and would not break our design, because:
• a newly created main document without a navigation committed is nearly empty, which means the Mojo messages it sends are not decided by sites owners, but by us, so it would be easy to collect the data we want.
• (In Feb. or Jan.) So, I collected pending receivers that RFH's BIB received before RFH committed the main frame navigation(on my gLinux device), and found RFH's BIB doesn't receive anything before the navigation is committed . 
• ( we may want to collect this kind of data again on other devices and on the current branch, but feel it would be 0.)

[Lingqi Chi]

Revision: 

For the second case: Realized my experimental design is not persuasive enough; I should have given renderer processes enough time to execute binding requests.

A tricky interface is mojom::blink::AppCacheBackend, which I cannot confirm for now.  (but still feel like the risk is controllable. 

[Alexander Timin]

Thanks Matt for looking into this — it indeed looks like a problem which we should fix. 

A few thoughts from my side:

BrowserInterfaceBroker is per-document (IIUC, this includes provisional / speculative documents and initial empty documents).

There are two ways to create a new document: 

• (1) create a new RenderFrame(Host) (and some document will be created together with it)
• (2) commit a cross-document navigation in the RenderFrameHost.

(1): There are two ways to create a RenderFrameHost as well: 

• (1a) create a speculative RenderFrameHost for cross-RFH navigation in a frame. This RFH will be created in a live state (with a RenderFrame) with a provisional document (if I got the Blink terminology correctly).
• (1b) When a new FrameTreeNode is created, a starting RenderFrameHost is created with it (see rakina@'s initial empty document for more details). 
• For (most of) the main frames, we just make their RFH live when we start to navigate. This, as well as the case in (1a) are done through the browser sending an IPC to the renderer.
• However, there is a different story for subframes (and main frames created via window.open) — there the renderer needs to have a functioning RenderFrame synchronously, so it creates it itself and sends the necessary parameters (including BIB) to the browser and then the browser creates the RenderFrameHost.
  (Loading a cross-origin subframe is a two-step procedure: create a frame with about:blank RFH (case 1b) and then perform a cross-RFH navigation in the new frame (case 2)).

(2): This case is more straightforward — the only thing to keep in mind is that the navigation commit can both replace an old document in the active RFH (cross-document same-RFH navigation) and replace a provisional document in a speculative / commit pending RFH.

With that said, usually the case (2) is more interesting for most features (as they don't interact with provisional documents and most of the properties are set when the "real" document is created), but here I think we have a different situation: for the purpose of `is_prerendering` tracker, the case (2) actually doesn't change much.

(RFH can't transition from `is_prerendered=false` to `is_prerendered=true` state, and the `is_prerendered=true` to `is_prerendered=false` transition is not triggered by the navigation itself).

So the first step of my proposal here would be "let's move setting is_prerendering=true" to the place where RenderFrame is created rather than setting it via the commit params.

It's fairly straightforward to do for cases where the browser creates RenderFrame, but in the subframe case, whether this process is initiated by the renderer is trickier.

As you mentioned, inheriting the `is_prerendered` value from the parent RenderFrame seems possible, but it seems fragile and I think we might have a better solution here:

Let's store `is_prerenderer` in RenderView, not RenderFrame :).

The main benefit is that for the subframe case the renderer creates a new RenderFrame, but uses the same RenderView, so we'll get "inherit from parent" behaviour by default.

(there is a case when the renderer creates a RenderView for a window.open, but this is disallowed in prerendering, so we won't have to worry about that). 

The added bonus is that it will be aligned with how bfcache state is tracked in the renderer :).

[dan...@chromium.org]

On Tue, Jul 6, 2021 at 9:52 AM Alexander Timin <alt...@chromium.org> wrote:

Thanks Matt for looking into this — it indeed looks like a problem which we should fix. 

A few thoughts from my side:

BrowserInterfaceBroker is per-document (IIUC, this includes provisional / speculative documents and initial empty documents).

There are two ways to create a new document: 

• (1) create a new RenderFrame(Host) (and some document will be created together with it)
• (2) commit a cross-document navigation in the RenderFrameHost.

(1): There are two ways to create a RenderFrameHost as well: 

• (1a) create a speculative RenderFrameHost for cross-RFH navigation in a frame. This RFH will be created in a live state (with a RenderFrame) with a provisional document (if I got the Blink terminology correctly).
• (1b) When a new FrameTreeNode is created, a starting RenderFrameHost is created with it (see rakina@'s initial empty document for more details). 
• For (most of) the main frames, we just make their RFH live when we start to navigate. This, as well as the case in (1a) are done through the browser sending an IPC to the renderer.
• However, there is a different story for subframes (and main frames created via window.open) — there the renderer needs to have a functioning RenderFrame synchronously, so it creates it itself and sends the necessary parameters (including BIB) to the browser and then the browser creates the RenderFrameHost.
  (Loading a cross-origin subframe is a two-step procedure: create a frame with about:blank RFH (case 1b) and then perform a cross-RFH navigation in the new frame (case 2)).

(2): This case is more straightforward — the only thing to keep in mind is that the navigation commit can both replace an old document in the active RFH (cross-document same-RFH navigation) and replace a provisional document in a speculative / commit pending RFH.

With that said, usually the case (2) is more interesting for most features (as they don't interact with provisional documents and most of the properties are set when the "real" document is created), but here I think we have a different situation: for the purpose of `is_prerendering` tracker, the case (2) actually doesn't change much.

(RFH can't transition from `is_prerendered=false` to `is_prerendered=true` state, and the `is_prerendered=true` to `is_prerendered=false` transition is not triggered by the navigation itself).

So the first step of my proposal here would be "let's move setting is_prerendering=true" to the place where RenderFrame is created rather than setting it via the commit params.

It's fairly straightforward to do for cases where the browser creates RenderFrame, but in the subframe case, whether this process is initiated by the renderer is trickier.

As you mentioned, inheriting the `is_prerendered` value from the parent RenderFrame seems possible, but it seems fragile and I think we might have a better solution here:

Let's store `is_prerenderer` in RenderView, not RenderFrame :).

The main benefit is that for the subframe case the renderer creates a new RenderFrame, but uses the same RenderView, so we'll get "inherit from parent" behaviour by default.

I just want to correct the mental model here a bit. The same RenderView is only used if the subframe is in the same site. A cross-site subframe has a separate RenderView

 

(there is a case when the renderer creates a RenderView for a window.open, but this is disallowed in prerendering, so we won't have to worry about that). 

The added bonus is that it will be aligned with how bfcache state is tracked in the renderer :).

On Tue, 6 Jul 2021 at 12:16, Lingqi Chi <lin...@chromium.org> wrote:

From MCC's point of view:

• Regarding the first case of "For example, an about:blank iframe appears to get the broker by sending the receiver in a CreateChildFrame message:
• That's why we start applying MojoBinderPolicies at the moment when a RFH instance is created. 
• For the prerendering state, their parent can teach them. (as FramePolicy does?
  
• Regarding the second case of binding BIB in CreateRenderView 
• Under the old architecture, i.e., prerender pages have their own WebContents, we noticed that for the main frame of prerendering page, BindBrowserInterfaceBrokerReceiver() is called twice.   (I'm not sure whether it is turn under the MPArch, but it would be easy to confirm.
• one is called when RenderView is created, the other is called in DidCommitNavigation()
• I feel it would still be safe and would not break our design, because:
• a newly created main document without a navigation committed is nearly empty, which means the Mojo messages it sends are not decided by sites owners, but by us, so it would be easy to collect the data we want.
• (In Feb. or Jan.) So, I collected pending receivers that RFH's BIB received before RFH committed the main frame navigation(on my gLinux device), and found RFH's BIB doesn't receive anything before the navigation is committed . 
• ( we may want to collect this kind of data again on other devices and on the current branch, but feel it would be 0.)

--

You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CALHg4nnTh41vGi1vKYRZD8OHzyNq1kL8Y4HpWD5a_CCvHEbL%3Dg%40mail.gmail.com.

[Matt Falkenhagen]

Thanks all. I started formulating an approach just before Alexander's last email came in, so I'll write what I had been thinking. I think there is some subtlety here due to MCC....

I also found it unsatisfying to completely rely on the renderer getting the prerendering state from the parent. It seems possible to hit races where the frame in the renderer looks at the parent just as activation is happening, and the parent may or may not have received the Activate IPC yet. It also feels error-prone that RFHI does not know with certainty whether the corresponding frame in the renderer thinks it's prerendering, since RFHI decides when to apply/release MCC.

I feel MCC is really related to the prerendering state. The RFHI needs to know whether to apply the policies, and whether to expect a DidActivate IPC to release them.

With that in mind I'm starting to feel we need to do something like this:

1. Whenever the renderer sends a BIB receiver to the browser, it needs to pass an "is_prerendering" along with it. The "is_prerendering" is computed either from the CommitNavigation param or from the parent. This way in BindBrowserInterfaceBrokerReceiver(), the RFHI can know whether it needs to apply MCC, and if it does, it knows it needs to send the Activate IPC on activation and wait for DidActivate before releasing. In cases where the browser can compute what it expects "is_prerendering" to be, it can validate what the renderer is saying (e.g., if it previously sent a CommitNavigation with is_prerendering).

Cases: DidCommitNavigation, CreateChildFrame,

2. Whenever the browser pushes a BIB remote to the renderer, it needs to pass an "is_prerendering" along with it.

Cases: AgentSchedulingGroup.CreateFrame, AgentSchedulingGroup.CreateView

This is a complicated web of things because unfortunately there is not a single place where BIB is given to the renderer, but it seems this is what we have to deal with.

2021年7月6日(火) 22:59 <dan...@chromium.org>:

[Alexander Timin]

Yep, sorry — I mentioned this above (about loading a cross-origin subframe being a two-step procedure), but I probably should have clarified that I'm talking about the initial frame creation here (and every subframe starts as a local subframe).

We'll need to ensure that the new RenderView created for a cross-origin navigation gets the correct value, but I think that we would actually get it by default (as this case should be similar to creating the initial RenderView in the prerendered FrameTree).

[Kinuko Yasuda]

Ensuring that we always pass the right is_prerender along with BIB sounds good to me.

[Daniel Cheng]

Just a few questions from my side:

On Tue, 6 Jul 2021 at 07:01, Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks all. I started formulating an approach just before Alexander's last email came in, so I'll write what I had been thinking. I think there is some subtlety here due to MCC....

I also found it unsatisfying to completely rely on the renderer getting the prerendering state from the parent. It seems possible to hit races where the frame in the renderer looks at the parent just as activation is happening, and the parent may or may not have received the Activate IPC yet. It also feels error-prone that RFHI does not know with certainty whether the corresponding frame in the renderer thinks it's prerendering, since RFHI decides when to apply/release MCC.

I feel MCC is really related to the prerendering state. The RFHI needs to know whether to apply the policies, and whether to expect a DidActivate IPC to release them.

With that in mind I'm starting to feel we need to do something like this:

1. Whenever the renderer sends a BIB receiver to the browser, it needs to pass an "is_prerendering" along with it. The "is_prerendering" is computed either from the CommitNavigation param or from the parent. This way in BindBrowserInterfaceBrokerReceiver(), the RFHI can know whether it needs to apply MCC, and if it does, it knows it needs to send the Activate IPC on activation and wait for DidActivate before releasing. In cases where the browser can compute what it expects "is_prerendering" to be, it can validate what the renderer is saying (e.g., if it previously sent a CommitNavigation with is_prerendering).

Cases: DidCommitNavigation, CreateChildFrame,

2. Whenever the browser pushes a BIB remote to the renderer, it needs to pass an "is_prerendering" along with it.

Cases: AgentSchedulingGroup.CreateFrame, AgentSchedulingGroup.CreateView

For AgentSchedulingGroup.CreateView:

Just to confirm, when we create the renderer for a prerendered page, we still create the RenderView via CreateRenderViewForRenderManager() right? The main frame in this case seems like it probably wouldn't /need/ to know about prerendering, as it shouldn't be doing anything--but maybe there's stuff injected by content embedders that would trigger interface requests, et cetera?

For AgentSchedulingGroup.CreateFrame:

I expect we wouldn't hit this currently at all for prerendered pages. I believe it's used for recreating RenderFrames after a crash, and also for creating provisional RenderFrames. Per the chat, my understanding is that prerendering is limited to same-origin subframes currently, so we would never hit either of these cases--though we would hit the latter with RenderDocument.

That being said, it makes sense to me to plumb it for consistency. Will RenderFrameObservers and such also need to know about prerendering?

Daniel

[Matt Falkenhagen]

Thanks Daniel.

2021年7月7日(水) 16:49 Daniel Cheng <dch...@chromium.org>:

Just a few questions from my side:

On Tue, 6 Jul 2021 at 07:01, Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks all. I started formulating an approach just before Alexander's last email came in, so I'll write what I had been thinking. I think there is some subtlety here due to MCC....

I also found it unsatisfying to completely rely on the renderer getting the prerendering state from the parent. It seems possible to hit races where the frame in the renderer looks at the parent just as activation is happening, and the parent may or may not have received the Activate IPC yet. It also feels error-prone that RFHI does not know with certainty whether the corresponding frame in the renderer thinks it's prerendering, since RFHI decides when to apply/release MCC.

I feel MCC is really related to the prerendering state. The RFHI needs to know whether to apply the policies, and whether to expect a DidActivate IPC to release them.

With that in mind I'm starting to feel we need to do something like this:

1. Whenever the renderer sends a BIB receiver to the browser, it needs to pass an "is_prerendering" along with it. The "is_prerendering" is computed either from the CommitNavigation param or from the parent. This way in BindBrowserInterfaceBrokerReceiver(), the RFHI can know whether it needs to apply MCC, and if it does, it knows it needs to send the Activate IPC on activation and wait for DidActivate before releasing. In cases where the browser can compute what it expects "is_prerendering" to be, it can validate what the renderer is saying (e.g., if it previously sent a CommitNavigation with is_prerendering).

Cases: DidCommitNavigation, CreateChildFrame,

2. Whenever the browser pushes a BIB remote to the renderer, it needs to pass an "is_prerendering" along with it.

Cases: AgentSchedulingGroup.CreateFrame, AgentSchedulingGroup.CreateView

For AgentSchedulingGroup.CreateView:

Just to confirm, when we create the renderer for a prerendered page, we still create the RenderView via CreateRenderViewForRenderManager() right? The main frame in this case seems like it probably wouldn't /need/ to know about prerendering, as it shouldn't be doing anything--but maybe there's stuff injected by content embedders that would trigger interface requests, et cetera?

That's right. As Kinuko pointed out, I was completely incorrect when I said "this probably never happens for prerendering"ーit always happens when we make the Prerendered main frame via NavigationControllerImpl::LoadURLWithParams. I'm not sure how I missed that.

I agree with that analysis about embedders, etc,... it feels error-prone to give the BIB to the renderer and expect it to be OK by expecting the renderer to not use it.

For AgentSchedulingGroup.CreateFrame:

I expect we wouldn't hit this currently at all for prerendered pages. I believe it's used for recreating RenderFrames after a crash, and also for creating provisional RenderFrames. Per the chat, my understanding is that prerendering is limited to same-origin subframes currently, so we would never hit either of these cases--though we would hit the latter with RenderDocument.

That being said, it makes sense to me to plumb it for consistency. Will RenderFrameObservers and such also need to know about prerendering?

Yep, plumbing for consistency/safety is also the conclusion I'm arriving at.  I think the RenderView idea Alexander had can augment but not entirely replace this, because of the way each Document needs its own prerendering state which gets reset after the Document finishes dispatching the prerenderingchange event, per the specification steps.

I'm not sure about RenderFrameObservers. I think so far we have not needed to touch them.

[Alexander Timin]

On Wed, 7 Jul 2021 at 09:18, Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks Daniel.

2021年7月7日(水) 16:49 Daniel Cheng <dch...@chromium.org>:

Just a few questions from my side:

On Tue, 6 Jul 2021 at 07:01, Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks all. I started formulating an approach just before Alexander's last email came in, so I'll write what I had been thinking. I think there is some subtlety here due to MCC....

I also found it unsatisfying to completely rely on the renderer getting the prerendering state from the parent. It seems possible to hit races where the frame in the renderer looks at the parent just as activation is happening, and the parent may or may not have received the Activate IPC yet. It also feels error-prone that RFHI does not know with certainty whether the corresponding frame in the renderer thinks it's prerendering, since RFHI decides when to apply/release MCC.

I feel MCC is really related to the prerendering state. The RFHI needs to know whether to apply the policies, and whether to expect a DidActivate IPC to release them.

With that in mind I'm starting to feel we need to do something like this:

1. Whenever the renderer sends a BIB receiver to the browser, it needs to pass an "is_prerendering" along with it. The "is_prerendering" is computed either from the CommitNavigation param or from the parent. This way in BindBrowserInterfaceBrokerReceiver(), the RFHI can know whether it needs to apply MCC, and if it does, it knows it needs to send the Activate IPC on activation and wait for DidActivate before releasing. In cases where the browser can compute what it expects "is_prerendering" to be, it can validate what the renderer is saying (e.g., if it previously sent a CommitNavigation with is_prerendering).

Cases: DidCommitNavigation, CreateChildFrame,

2. Whenever the browser pushes a BIB remote to the renderer, it needs to pass an "is_prerendering" along with it.

Cases: AgentSchedulingGroup.CreateFrame, AgentSchedulingGroup.CreateView

For AgentSchedulingGroup.CreateView:

Just to confirm, when we create the renderer for a prerendered page, we still create the RenderView via CreateRenderViewForRenderManager() right? The main frame in this case seems like it probably wouldn't /need/ to know about prerendering, as it shouldn't be doing anything--but maybe there's stuff injected by content embedders that would trigger interface requests, et cetera?

That's right. As Kinuko pointed out, I was completely incorrect when I said "this probably never happens for prerendering"ーit always happens when we make the Prerendered main frame via NavigationControllerImpl::LoadURLWithParams. I'm not sure how I missed that.

I agree with that analysis about embedders, etc,... it feels error-prone to give the BIB to the renderer and expect it to be OK by expecting the renderer to not use it.

For AgentSchedulingGroup.CreateFrame:

I expect we wouldn't hit this currently at all for prerendered pages. I believe it's used for recreating RenderFrames after a crash, and also for creating provisional RenderFrames. Per the chat, my understanding is that prerendering is limited to same-origin subframes currently, so we would never hit either of these cases--though we would hit the latter with RenderDocument.

That being said, it makes sense to me to plumb it for consistency. Will RenderFrameObservers and such also need to know about prerendering?

Yep, plumbing for consistency/safety is also the conclusion I'm arriving at.  I think the RenderView idea Alexander had can augment but not entirely replace this, because of the way each Document needs its own prerendering state which gets reset after the Document finishes dispatching the prerenderingchange event, per the specification steps.

I'd like to understand the problem a bit more here — in particular, I wonder if we could iterate all LocalFrames belonging to a given blink::WebView on the blink side and dispatch the necessary `prerenderingchange` events to each Document there?

[Matt Falkenhagen]

2021年7月7日(水) 20:57 Alexander Timin <alt...@chromium.org>:

On Wed, 7 Jul 2021 at 09:18, Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks Daniel.

2021年7月7日(水) 16:49 Daniel Cheng <dch...@chromium.org>:

Just a few questions from my side:

On Tue, 6 Jul 2021 at 07:01, Matt Falkenhagen <fal...@chromium.org> wrote:

Thanks all. I started formulating an approach just before Alexander's last email came in, so I'll write what I had been thinking. I think there is some subtlety here due to MCC....

I also found it unsatisfying to completely rely on the renderer getting the prerendering state from the parent. It seems possible to hit races where the frame in the renderer looks at the parent just as activation is happening, and the parent may or may not have received the Activate IPC yet. It also feels error-prone that RFHI does not know with certainty whether the corresponding frame in the renderer thinks it's prerendering, since RFHI decides when to apply/release MCC.

I feel MCC is really related to the prerendering state. The RFHI needs to know whether to apply the policies, and whether to expect a DidActivate IPC to release them.

With that in mind I'm starting to feel we need to do something like this:

1. Whenever the renderer sends a BIB receiver to the browser, it needs to pass an "is_prerendering" along with it. The "is_prerendering" is computed either from the CommitNavigation param or from the parent. This way in BindBrowserInterfaceBrokerReceiver(), the RFHI can know whether it needs to apply MCC, and if it does, it knows it needs to send the Activate IPC on activation and wait for DidActivate before releasing. In cases where the browser can compute what it expects "is_prerendering" to be, it can validate what the renderer is saying (e.g., if it previously sent a CommitNavigation with is_prerendering).

Cases: DidCommitNavigation, CreateChildFrame,

2. Whenever the browser pushes a BIB remote to the renderer, it needs to pass an "is_prerendering" along with it.

Cases: AgentSchedulingGroup.CreateFrame, AgentSchedulingGroup.CreateView

For AgentSchedulingGroup.CreateView:

Just to confirm, when we create the renderer for a prerendered page, we still create the RenderView via CreateRenderViewForRenderManager() right? The main frame in this case seems like it probably wouldn't /need/ to know about prerendering, as it shouldn't be doing anything--but maybe there's stuff injected by content embedders that would trigger interface requests, et cetera?

That's right. As Kinuko pointed out, I was completely incorrect when I said "this probably never happens for prerendering"ーit always happens when we make the Prerendered main frame via NavigationControllerImpl::LoadURLWithParams. I'm not sure how I missed that.

I agree with that analysis about embedders, etc,... it feels error-prone to give the BIB to the renderer and expect it to be OK by expecting the renderer to not use it.

For AgentSchedulingGroup.CreateFrame:

I expect we wouldn't hit this currently at all for prerendered pages. I believe it's used for recreating RenderFrames after a crash, and also for creating provisional RenderFrames. Per the chat, my understanding is that prerendering is limited to same-origin subframes currently, so we would never hit either of these cases--though we would hit the latter with RenderDocument.

That being said, it makes sense to me to plumb it for consistency. Will RenderFrameObservers and such also need to know about prerendering?

Yep, plumbing for consistency/safety is also the conclusion I'm arriving at.  I think the RenderView idea Alexander had can augment but not entirely replace this, because of the way each Document needs its own prerendering state which gets reset after the Document finishes dispatching the prerenderingchange event, per the specification steps.

I'd like to understand the problem a bit more here — in particular, I wonder if we could iterate all LocalFrames belonging to a given blink::WebView on the blink side and dispatch the necessary `prerenderingchange` events to each Document there?

We can, and I think we should do that, but it alone doesn't fully solve the problem. Here's a sequence I'm worried about:

1. Browser does activation and sends "Activate" IPC to the renderer.

2. Renderer did not yet receive IPC. It makes a Document in prerendering mode and creates a BIB remote, sending the receiver to the browser, e.g., in CreateChildFrame.

3. Browser makes a new RFHI for the Document and binds the BIB receiver. It considers this RFHI not in prerendering mode, and doesn't apply Mojo capability control.

4. Renderer sends a Foo receiver over BIB and starts using the Foo remote. Because Foo is on a different message pipe, Renderer can start getting responses to Foo messages. Foo APIs that should be deferred while document.prerendering is true are not deferred, breaking web-exposed behavior.

5. "Activate IPC" arrives on the renderer. The Document dispatches `prerenderingchange`, which is fine, but it's too late, it should have happened before step 4.

I think this sequence can happen regardless of whether the Activate IPC in step 1 is per-RenderFrame or per-RenderView.

To solve this problem, I think the renderer should tell the browser whether it thinks it's in prerendered mode along with the BIB remote it sends to it.

Here is a rough design sketch I'd like to try:

- The browser process only tells the renderer that it's prerendering when it asks it to make a RenderView/WebView (no need for the "is_prerendering" param in CommitNavigation anymore).

- Whenever the renderer makes a Document, it consults RenderView to know whether to set document.prerendering to true.

- Whenever the renderer sends a per-frame BIB receiver to the browser, it additionally tells the browser whether the Document thinks it's prerendering. I believe we can do this without compromising securityーthe browser can detect if the renderer is lying in the bad direction. This param only allows the renderer to opt-in to a more restricted BIB.

- On activation, the browser process sends a single IPC to RenderView/WebView. This triggers prerenderingchange on each Document.

- RenderFrameHostImpl releases Mojo capability control once the Document acknowledges that it is no longer prerendering (it finished dispatching `prerenderingchange`).

Note that we used to use a single Activation IPC but it was reversed, I think the reasons listed there are tractable though.

[Alexander Timin]

Overall that makes sense, thanks for writing up the details!

One question / suggestion, however — do we really need to send additional information from the renderer to the browser with each request or whether we could use the existing information in the browser process?

In particular, for bfcache we have the "last acknowledged state" (e.g. whether the renderer has finished storing the page in bfcache, including running `pagehide` handlers — some of our eligibility checks run only after that point).

This seems like a very similar problem to what we have here — I wonder if querying last_acknowledged_state_->is_prerendering would work for the purposes of the MCC?  

Note that we used to use a single Activation IPC but it was reversed, I think the reasons listed there are tractable though.

Yep, I think that we should fix the problems there — it should be easier given that the multiple WC implementation was removed.  

[Matt Falkenhagen]

2021年7月7日(水) 23:59 Alexander Timin <alt...@chromium.org>:

I agree this would be ideal. But I don't think we can do this without changing the protocol more. If the browser sent the Activate IPC and then gets a renderer-initiated BIB receiver, it does not know if that Document thinks it's prerendering or not. If it applies MCC and the Document does not think it's prerendering, the RFHI will forever wait for a DidActivate IPC from the Document that will never arrive, so will apply MCC indefinitely. One solution is to add another IPC from Renderer to Browser sent to ACK that the RenderView received the Activate IPC. Then the browser knows that all renderer-initiated BIBs no longer not need MCC. But this is weirdly asymmetric: the Browser either waits for a DidActivate IPC from the Document or an overall DidActivate IPC from the RenderView.

I'm starting to think we should just have a single Activate IPC from the Browser to Renderer with a single ACK callback which is sent after all Documents finish dispatching  prerenderingchange. This has some nice properties: there are just two sync points, and there's no invasive "is_prerender" in all IPCs that send a BIB. Until the Browser gets the ACK it keeps all BIBs in loose MCC mode. The disadvantage is we lose some precision: Documents that are not prerendering still have APIs deferred until all Documents finish, but that shouldn't be a big deal in practice. Also, in loose mode we run kCancel/kUnexpected interfaces, so Documents that are prerendering can start using APIs that are supposed to cancel prerendering, which is not great either. 

If we could defer instead of run kCancel/kUnexpected in loose mode, that could be more viable, but there are other challenges there.

Stepping back, probably the sequence I wrote in the previous mail is not a big deal in practiceーwe only lose some correctness in a twisted IPC race condition case. I think we could be fine always applying MCC based on the browser's understanding of the prerendering state, even for renderer-initiated BIB receivers, at least for M93 (i.e., sometimes a Document that thinks it's prerendering gets a non-controlled BIB, and it later tells the browser it DidActivate, and the browser says "Ah you thought you were being prerendered, but you weren't, I'll just ignore this message").

[Alexander Timin]

Big +1 to this suggestion – this aligns with my thinking very well.

Re: per-document precision: I actually think that it's a feature, not a bug — documents in the same RenderView (generally) have sync-script access between them, so the exact boundaries between them are fuzzy, so overall I think for the purposes of the global state (like `is_prerendering`) we should documents belonging to same agent as an atomic unit.

 

If we could defer instead of run kCancel/kUnexpected in loose mode, that could be more viable, but there are other challenges there. 

 

Yep, +1 to looking into deferring them in loose mode until we receive an ACK.

[Matt Falkenhagen]

Thanks, this approach looks promising. I summarized this discussion in a document and created a CL with this approach.

2021年7月10日(土) 0:44 Alexander Timin <alt...@chromium.org>: