Safely accessing an ancestor's frame last committed origin

[Daniel Cheng]

Is there any way to do this safely without caching the value? If I understand correctly, when we commit a navigation in a RFH, descendent RFHs aren't necessarily destroyed promptly—they may remain in pending deletion state for some time. If an object associated with this pending delete RFH simply calls render_frame_host->GetMainFrame()->GetLastCommittedOrigin(), this may end up returning the origin of the already-navigated main frame.

This came up in the context of https://chromium-review.googlesource.com/c/chromium/src/+/3087727: the API performs a check using the origin of the main frame.

Assuming this is a problem, any thoughts on how we can make this sort of pattern less bug-prone?

A few random (not necessarily good) ideas:

- Ship RenderDocumentHost for all cross-origin navigations. Hope that this doesn't run into the document.write() bug. Referencing the last committed URL could still be wrong… but at least the origin wouldn't be?

- Maintain a parallel tree of DocumentAssociatedData and move the last committed origin/URL to it. The tree would be maintained in such a way that the ancestor chain is always immutable. How to expose this DocumentAssociatedData to actual code that would consume it is left as an open question.

- Eagerly destroy RenderDocumentHostUserData/DocumentServiceBase objects when the current frame commits a cross-document navigation (already happens for RenderDocumentHostUserData but not for DocumentServiceBase) or *any* ancestor frame commits a cross-document navigation. However, this could have surprising side effects. A RenderFrameHost that is still valid but pending deletion would suddenly lose its RenderDocumentHostUserData. Mojo services might be unexpectedly deleted too early, breaking IPCs from that frame in the renderer.

Daniel

[Russ Hamilton]

My thought would to get rid of the guarantee that the return value from GetMainFrame is not NULL and update the entire tree when they get disconnected by the main frame.

This would require changing or at least auditing every place that uses GetMainFrame, but any place this would actually be affected by this change is probably currently working with wrong data anyway.

--Benjamin "Russ" Hamilton

[Kentaro Hara]

My thought would to get rid of the guarantee that the return value from GetMainFrame is not NULL and update the entire tree when they get disconnected by the main frame.
This would require changing or at least auditing every place that uses GetMainFrame, but any place this would actually be affected by this change is probably currently working with wrong data anyway.

I agree but do we need this after we have RenderDocumentHost? RenderDocumentHost switches the main RFH every navigation. Then rfh->GetMainFrame() of the descendant (pending-deletion) RFHs return the main RFH before the navigation, not the main RFH after the navigation. Will it be sufficient?

Overall my understanding is that shipping RenderDocumentHost will fix the problem.

- Ship RenderDocumentHost for all cross-origin navigations. Hope that this doesn't run into the document.write() bug. Referencing the last committed URL could still be wrong… but at least the origin wouldn't be?

With RenderDocumentHost, would you help me understand why the last committed URL could be wrong?

- Eagerly destroy RenderDocumentHostUserData/DocumentServiceBase objects when the current frame commits a cross-document navigation (already happens for RenderDocumentHostUserData but not for DocumentServiceBase) or *any* ancestor frame commits a cross-document navigation. However, this could have surprising side effects. A RenderFrameHost that is still valid but pending deletion would suddenly lose its RenderDocumentHostUserData. Mojo services might be unexpectedly deleted too early, breaking IPCs from that frame in the renderer.

Would you help me understand how eagerly destroying RenderDocumentHostUserData/DocumentServiceBase helps prevent the descendant RFHs from accessing rfh->GetMainFrame()?

--
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/CAAG-DU2d5eZWr-%2BQzb%2BiBz9LNEwVRxRNzgsDhg1%3DhvUMdt5ycQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Daniel Cheng]

On Sun, 12 Sept 2021 at 18:41, Kentaro Hara <har...@chromium.org> wrote:

My thought would to get rid of the guarantee that the return value from GetMainFrame is not NULL and update the entire tree when they get disconnected by the main frame.
This would require changing or at least auditing every place that uses GetMainFrame, but any place this would actually be affected by this change is probably currently working with wrong data anyway.

I agree but do we need this after we have RenderDocumentHost? RenderDocumentHost switches the main RFH every navigation. Then rfh->GetMainFrame() of the descendant (pending-deletion) RFHs return the main RFH before the navigation, not the main RFH after the navigation. Will it be sufficient?

Overall my understanding is that shipping RenderDocumentHost will fix the problem.

- Ship RenderDocumentHost for all cross-origin navigations. Hope that this doesn't run into the document.write() bug. Referencing the last committed URL could still be wrong… but at least the origin wouldn't be?

With RenderDocumentHost, would you help me understand why the last committed URL could be wrong?

I'm not completely up-to-date on RenderDocumentHost, but I was hoping that it would be easier to ship "swap RFH on cross-origin cross-document navigations" rather than "swap RFH on all cross-document navigations". I'm not sure if this is the case though: if we ship full RenderDocumentHost, then last committed URL will be correct. But if we only ship it for cross-origin cross-document navigations, then it could still be wrong.

 

- Eagerly destroy RenderDocumentHostUserData/DocumentServiceBase objects when the current frame commits a cross-document navigation (already happens for RenderDocumentHostUserData but not for DocumentServiceBase) or *any* ancestor frame commits a cross-document navigation. However, this could have surprising side effects. A RenderFrameHost that is still valid but pending deletion would suddenly lose its RenderDocumentHostUserData. Mojo services might be unexpectedly deleted too early, breaking IPCs from that frame in the renderer.

Would you help me understand how eagerly destroying RenderDocumentHostUserData/DocumentServiceBase helps prevent the descendant RFHs from accessing rfh->GetMainFrame()?

When we commit a navigation in a RFH, we eagerly clear the RenderDocumentHostUserData here: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=10322;drc=5dd5f982afa4cc7152097106bf755eeacb9bc327

This happens before we set the last committed origin, so the RenderDocumentHostUserData will never see the wrong origin in that case.

However, let's say we have A(B), and B has a RenderDocumentHostUserData attached. When A commits a navigation, we will destroy the RenderDocumentHostUserData for A: however, we will *not* clear it for B. The RenderDocumentHostUserData will be destroyed later, when RFH B is deleted after it completes unloading. In that time period, B's RenderDocumentHostUserData will incorrectly see the already-updated origin on A.

Daniel

[Alexander Timin]

Big +1 from my side to shipping the RenderDocumentHost (maybe its cross-origin version to start with) being the right solution here — let's do it as soon as we can :). A few additional random thoughts below:

- I don't think that making RenderFrameHost::GetMainFrame returning nullptr is a good solution — it will require a significant amount of work to add a lot of null checks to the codebase — and we'll have to remove them after shipping RenderDocument, which doesn't sound ideal.

- Maintaining a parallel tree of Documents is possible, but seems to be a roundabout way to get to the RenderDocument. I think we can do this if we hit a major roadblock to RenderDocument, but I'd really like us to start with making a push towards RenderDocument.

- Making the most important individual callsites more robust is an option as well: they could explicitly check for RenderFrameHost::IsActive / GetLifecycleState to detect RFHs whose ancestors are not current in their frames.

Many places also use RFH::GetMainFrame to check if they are same-origin with the main frame or not. We could introduce a dedicated helper for this (e.g. RenderFrameHost::IsSameOriginWithPrimaryDocument), which will always return false to non-active RFHs.

This might nicely align with MPArch / Fenced Frames work, where we want to rethink what "same-origin with main frame" works for fenced frames anyway.

- (this idea is too exciting even for me and I'm pretty sure that it's unreasonable) 

We already destroy child RenderFrameHosts before committing a new document in the parent RFH if there are no unload (or similar) handlers: when renderer is committing navigation, it detaches the frame (e.g. RemoteFrameHost::Detach), which means that browser processes

RenderFrameHostImpl::DeleteRenderFrame before processing DidCommitNavigation and if there are no unload handlers DeleteRenderFrame causes RFH to be deleted in the same task. If there is an unload handler, the child RFH will outlive the document in the parent RFH.

There are two main reasons why we'd want to keep child RFH alive there — to ensure that the renderer process isn't destroyed before unload has a chance to run and to ensure that the (associated) IPCs sent from unload handler (e.g. ones modifying window.name) are processed.

I believe that we can achieve the same effect without keeping RFHI alive — delaying process destruction is easy, and for processing IPCs (associated, as non-associated already don't have ordering guarantees) we can unbind LocalFrameHost interface and bind it again to a new smaller content-internal class. 

  

 

  

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

[Kentaro Hara]

There are two main reasons why we'd want to keep child RFH alive there — to ensure that the renderer process isn't destroyed before unload has a chance to run and to ensure that the (associated) IPCs sent from unload handler (e.g. ones modifying window.name) are processed.
I believe that we can achieve the same effect without keeping RFHI alive — delaying process destruction is easy, and for processing IPCs (associated, as non-associated already don't have ordering guarantees) we can unbind LocalFrameHost interface and bind it again to a new smaller content-internal class. 

Once we have RenderDocument and the MPArch fix is done (which is anyway needed), is there any reason we want to remove pending deletion RFHs?

--

Kentaro Hara, Tokyo

[Alexander Timin]

On Wed, 15 Sept 2021 at 02:38, Kentaro Hara <har...@chromium.org> wrote:

There are two main reasons why we'd want to keep child RFH alive there — to ensure that the renderer process isn't destroyed before unload has a chance to run and to ensure that the (associated) IPCs sent from unload handler (e.g. ones modifying window.name) are processed.
I believe that we can achieve the same effect without keeping RFHI alive — delaying process destruction is easy, and for processing IPCs (associated, as non-associated already don't have ordering guarantees) we can unbind LocalFrameHost interface and bind it again to a new smaller content-internal class. 

Once we have RenderDocument and the MPArch fix is done (which is anyway needed), is there any reason we want to remove pending deletion RFHs?

Nope, RenderDocument is the comprehensive (and the best) answer here, with everything else being needed only if we don't have the full RenderDocument.