Mechanisms for determining if a FrameTree, FrameTreeNode, and RenderFrameHostImpl are fenced

[Dominic Farolino]

Hey everyone, I wanted to publicly summarize a discussion that I had with shivanisha@ earlier today that may impact the work that will be done on MPArch long-tail audit for fenced frames.

Right now there are four ways of determining whether or not various brower-side objects are associated with a fenced frame:

1. FrameTreeNode::IsFencedFrame() (being renamed to IsFencedFrameRoot() in this CL)
1. ShadowDOM implementation: This will only return true for the FrameTreeNode corresponding with the internal <iframe> backing the <fencedframe> element
2. MPArch implementation: This will only return true for the root FrameTreeNode of the inner FrameTree (not the outer delegate dummy child FrameTreeNode) (literally just delegates to IsMainFrame() and FrameTree::type())
2. FrameTreeNode::IsInFencedFrameTree()
1. ShadowDOM implementation: Returns true for all FrameTreeNodes under-and-including the internal <iframe> backing the <fencedframe> element.
2. MPArch implementation: Returns true for all FrameTreeNodes in the inner FrameTree (literally just delegates to FrameTree::type())
3. RenderFrameHost(Impl)::HostedByFencedFrame()
1. Just delegates directly to FrameTreeNode::IsFencedFrame(). I'm renaming this method to IsFencedFrameRoot() just like the FTN method for clarity.
2. This method is needed to expose something to the public API for consumers that are dealing directly with the RenderFrameHost interface
4. FrameTree::IsFencedFrameTree() // FrameTree::type()
1. This is only used for unit tests right now before the MPArch impl lands. I'm removing it in CL 3057656 in favor of having people just call type() directly and check there

This begs the question: when working on the long-tail for fenced frames, which one(s) should we use? There are some significant differences between them, notably that FrameTree::type() will only return kFencedFrame for the MPArch implementation, whereas the RFHI/FTN methods will indicate fenced-frame-ness even in the ShadowDOM Origin Trial implementation.

The MPArch long-tail spreadsheet is called the MPArch long-tail spreadsheet for a reason, so I don't think it makes sense to address each row for both the MPArch version of fenced frames as well as the ShadowDOM Origin Trial implementation. With that said, presumably many long-tail fixes will be made by determining whether or not we're in a fenced frame FrameTree, and then implementing some behavior difference etc. If these fixes attempt to determine whether or not we're in a fenced frame's FrameTree by checking the RFHI/FTN methods, then these changes will be live for both the MPArch implementation as well as the ShadowDOM implementation, even though only the MPArch implementation will be tested.

That is not very great, since having lots of untested code paths is never a good thing, which is why I'm thinking a good policy to have for writing/reviewing long-tail fenced frame fixes would be: "Stay away from the FrameTreeNode methods, and always prefer FrameTree::type() when trying to determine if you're associated with a fenced frame FrameTree, and not a kPrimary one".

You might be asking "Then who is going to use the FrameTreeNode methods? What good are they?" These will continue to be used probably mostly by myself, Shivani, and perhaps FLEDGE folks. The only consumers of these methods should be objects whose fenced-frame-specific-behavior is so core to the fenced frame experience, it must be present for both the origin trial and the long-term implementation. Each of these impl paths will be tested by us specifically. But since most of the long-tail MPArch fixes are not critical to the core ads use-cases of fenced frames (beacuse we were always planning on OT'ing before the long-tail work began), we can refrain from having the long-tail fix behavior apply to the ShadowDOM OT impl, which is important because we don't want to write 2x tests for all of the long-tail sheet items.

I know that is all a bit complicated, but we're looking to create some guidance for external folks that will work on the MPArch+FencedFrames and I think this is an important one to touch on, and I'd like to hear your opinions too.

Thanks,

Dom

[Lucas Gadani]

On Fri, Aug 6, 2021 at 7:35 PM Dominic Farolino <d...@chromium.org> wrote:

Hey everyone, I wanted to publicly summarize a discussion that I had with shivanisha@ earlier today that may impact the work that will be done on MPArch long-tail audit for fenced frames.

Right now there are four ways of determining whether or not various brower-side objects are associated with a fenced frame:

1. FrameTreeNode::IsFencedFrame() (being renamed to IsFencedFrameRoot() in this CL)
1. ShadowDOM implementation: This will only return true for the FrameTreeNode corresponding with the internal <iframe> backing the <fencedframe> element
2. MPArch implementation: This will only return true for the root FrameTreeNode of the inner FrameTree (not the outer delegate dummy child FrameTreeNode) (literally just delegates to IsMainFrame() and FrameTree::type())
2. FrameTreeNode::IsInFencedFrameTree()
1. ShadowDOM implementation: Returns true for all FrameTreeNodes under-and-including the internal <iframe> backing the <fencedframe> element.
2. MPArch implementation: Returns true for all FrameTreeNodes in the inner FrameTree (literally just delegates to FrameTree::type())
3. RenderFrameHost(Impl)::HostedByFencedFrame()
1. Just delegates directly to FrameTreeNode::IsFencedFrame(). I'm renaming this method to IsFencedFrameRoot() just like the FTN method for clarity.
2. This method is needed to expose something to the public API for consumers that are dealing directly with the RenderFrameHost interface
4. FrameTree::IsFencedFrameTree() // FrameTree::type()
1. This is only used for unit tests right now before the MPArch impl lands. I'm removing it in CL 3057656 in favor of having people just call type() directly and check there

This begs the question: when working on the long-tail for fenced frames, which one(s) should we use? There are some significant differences between them, notably that FrameTree::type() will only return kFencedFrame for the MPArch implementation, whereas the RFHI/FTN methods will indicate fenced-frame-ness even in the ShadowDOM Origin Trial implementation.

From the options you listed, RenderFrameHost is the only one that's in the public API and exposed outside of content. The majority of chrome features that we are fixing in the long tail work is outside of content, and therefore they'll have to use the RFH path.

However, there are some cases where there's no RFH available to query. We'll probably have to expose something inside NavigationHandle that'll allow us to query the FrameTree type, since during the beginning of the navigation there's no assigned RFH yet. Today, we have NavigationHandle::IsInPrimaryMainFrame, and in general we are assuming that anything that's not a primary main frame is inside a prerender. If cases we need to differentiate between FF and prerenders in navigation observers or navigation throttles, we'll need to introduce something in NavigationHandle.

The MPArch long-tail spreadsheet is called the MPArch long-tail spreadsheet for a reason, so I don't think it makes sense to address each row for both the MPArch version of fenced frames as well as the ShadowDOM Origin Trial implementation. With that said, presumably many long-tail fixes will be made by determining whether or not we're in a fenced frame FrameTree, and then implementing some behavior difference etc. If these fixes attempt to determine whether or not we're in a fenced frame's FrameTree by checking the RFHI/FTN methods, then these changes will be live for both the MPArch implementation as well as the ShadowDOM implementation, even though only the MPArch implementation will be tested.

I think we have 2 options to test both Shadow DOM+MPArch code paths:

1) have a separate FYI bot that forces the Shadow DOM path. As long as we write the test helpers to add FF via renderer APIs, that should work.

2) Use a combination of virtual test suites (for wpt tests) and parametrized tests (for browser tests). This avoids the need of an extra bot (and monitoring it out of the CQ), it is fairly simple to create and maintain for wpt tests, but a bit of extra work to maintain parametrized the browser tests.

That is not very great, since having lots of untested code paths is never a good thing, which is why I'm thinking a good policy to have for writing/reviewing long-tail fenced frame fixes would be: "Stay away from the FrameTreeNode methods, and always prefer FrameTree::type() when trying to determine if you're associated with a fenced frame FrameTree, and not a kPrimary one".

For some of the reasons I mentioned above, using the content-only APIs will be restricted mostly by the simple fact that most of the features interacting with frames are implemented outside of content. I think your point is still a good point and valid, though I wouldn't emphasize it too much simply because I don't expect that FTN methods (or FT) will be used much.

You might be asking "Then who is going to use the FrameTreeNode methods? What good are they?" These will continue to be used probably mostly by myself, Shivani, and perhaps FLEDGE folks. The only consumers of these methods should be objects whose fenced-frame-specific-behavior is so core to the fenced frame experience, it must be present for both the origin trial and the long-term implementation. Each of these impl paths will be tested by us specifically. But since most of the long-tail MPArch fixes are not critical to the core ads use-cases of fenced frames (beacuse we were always planning on OT'ing before the long-tail work began), we can refrain from having the long-tail fix behavior apply to the ShadowDOM OT impl, which is important because we don't want to write 2x tests for all of the long-tail sheet items.

I know that is all a bit complicated, but we're looking to create some guidance for external folks that will work on the MPArch+FencedFrames and I think this is an important one to touch on, and I'd like to hear your opinions too.

Thanks,

Dom

--
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/CAP-uykBD_dQAys%2BHhOVWKfNo0MO-vOKDRtgwPX76%2B5fEwvVB0Q%40mail.gmail.com.

[Alexander Timin]

+1 to Lucas's point that only RenderFrameHost is widely exposed, so its method should be used in the majority of the places, complemented by something like NavigationHandle::IsInFencedFrame (or NavigationHandle::GetFrameType => {kPrimaryMainFrame, kPrerenderMainFrame, kFencedFrameRoot, kSubframe}).

A more interesting question is the ShadowDOM one. There it seems to me that the main goal is to have a non-invasive low-cost initial version, which relies on the iframe logic for correctness.

From that perspective, my main question would be in how many places we would need additional logic for the ShadowDOM milestone? My hope that the number of these places would be small and in that case we'd be better off just adding a separate bool (is_shadow_dom_fenced_frame_root_) and query it explicitly where we need.

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

[Dominic Farolino]

Apologies for the very long delay on this. Replies below.

Responding to Lucas:

From the options you listed, RenderFrameHost is the only one that's in the public API and exposed outside of content. The majority of chrome features that we are fixing in the long tail work is outside of content, and therefore they'll have to use the RFH path. 

However, there are some cases where there's no RFH available to query. We'll probably have to expose something inside NavigationHandle that'll allow us to query the FrameTree type, since during the beginning of the navigation there's no assigned RFH yet. Today, we have NavigationHandle::IsInPrimaryMainFrame, and in general we are assuming that anything that's not a primary main frame is inside a prerender. If cases we need to differentiate between FF and prerenders in navigation observers or navigation throttles, we'll need to introduce something in NavigationHandle.

I agree we may very well end up having to introduce something new on NavigationHandle; this has been on my mind as I almost needed to do this for the first fenced frames + MPArch implementation CL, and will certainly need to add something in a subsequent one. Example: RenderFrameHost::IsActive() currently returns false for main-frame prerenders and same-origin subframes, so we take advantage of this in the PageLoadMetricsWebContentsObserver as an early-return for non-primary pages, until it is adapted to work properly. This will not work for fenced frames, because they are active, and the existing NavigationHandle::IsInPrimaryMainFrame() will not work either, because we want something like IsInPrimaryFrameTree(), to catch subframes as well etc.

I think we have 2 options to test both Shadow DOM+MPArch code paths:
1) have a separate FYI bot that forces the Shadow DOM path. As long as we write the test helpers to add FF via renderer APIs, that should work.
2) Use a combination of virtual test suites (for wpt tests) and parametrized tests (for browser tests). This avoids the need of an extra bot (and monitoring it out of the CQ), it is fairly simple to create and maintain for wpt tests, but a bit of extra work to maintain parametrized the browser tests.

I am thinking we'd be doing much more of (2), and limit most of the ShadowDOM testing to non-MPArch bits in Blink (mostly ensuring solid isolation from the parent frame from script). In other words, we'd write a ton of browser tests for MPArch + long-tail, and these would almost always be independent of ShadowDOM -- they wouldn't activate the ShadowDOM path, and they wouldn't test it either, since they are mostly out-of-scope for the ShadowDOM impl's needs. Then separately there would be other "core" tests, exercising the core fenced frame flows that must work regardless of the architecture (things like navigation urn:uuid => URL conversions). These can exist as parameterized browser tests (OK because they should be limited in number) as well as WPTs.

For some of the reasons I mentioned above, using the content-only APIs will be restricted mostly by the simple fact that most of the features interacting with frames are implemented outside of content. I think your point is still a good point and valid, though I wouldn't emphasize it too much simply because I don't expect that FTN methods (or FT) will be used much.

This is a good point, thanks for bringing it up. In that case, I think a better idea to satisfy my desire of ensuring that MPArch long-tail work does trigger the ShadowDOM path, would be to not make //content/public APIs like "IsFencedFrameRoot()" delegate to the FrameTreeNode methods, and instead make them all check the underlying FrameTree::Type, so that they only return true for the MPArch version of fenced frames. In other words, every single IsFencedFrameRoot() API available would return true only for the MPArch version, except for a few obscure ones on FTN that we might want to rename to something like IsFencedFrameRootRegardlessOfArchitecture(), but obviously named much better than that.

Responding to altimin@ here:

From that perspective, my main question would be in how many places we would need additional logic for the ShadowDOM milestone? My hope that the number of these places would be small and in that case we'd be better off just adding a separate bool (is_shadow_dom_fenced_frame_root_) and query it explicitly where we need.

Thanks for raising this! I believe this is mostly covered in my reply above, but want to run it by you for clarity. I agree there shouldn't be too many places in the browser that need to know the ShadowDOM-ness of fenced frames, in which case we can delegate to something like a bool that we'd use for the core browser use cases that should care (again, like navigation urn:uuid => URL conversion). In this case, instead of actually storing a boolean, I'd like to defer to the effective_frame_policy() sitting on FrameTreeNode like we currently do-ish, or also the FrameOwnerElement type which will be kFencedFrame for the <fencedframe>'s internal <iframe>'s RFHI.

Regardless of how exactly we retrieve the bit, I think the meat of this comes down to ensuring that we don't expose truthiness in any of the IsFencedFrame[...]() //content/public APIs for the ShadowDOM version so that we can ensure they only exercise the MPArch version, which is what the long-tail is aimed at.

[Kevin McNee]

Regarding browser tests, we talked a bit about this offline, so recording here for posterity. Indeed, since the Shadow DOM path is relying on existing architecture, parameterizing the browser tests would likely be of limited value for the kinds of fixes we'd be doing for MPArch. However, since the kinds of tests we're writing for the long tail are integration tests, they would ideally be architecture agnostic. In that case, down the road, we may be able to experimentally run the tests under the Shadow DOM architecture as a sanity check, instead of committing to a maintenance burden.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAP-uykCfgq0u98L%3Dmm54%3DPHHt%3DUyiNZ5hvbVacNkDqKUY-qApA%40mail.gmail.com.