Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spec] Fix navigation from the default constructor not installing a fenced frame config. #183

Merged
merged 11 commits into from
Oct 5, 2024

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Aug 19, 2024

The existing spec results in a fenced frame navigating directly to the url stored in a FencedFrameConfig IDL object when navigating to a config built with the FencedFrameConfig(url) default constructor. This results in a fenced frame config and fenced frame config instance not being created for the fenced frame, cutting off the internal document from window.fence.

This PR fixes that in the following ways:

  • Modifies the FencedFrameConfig(url) constructor to also create a fenced frame config and add it to the navigable's fenced frame config mapping.
  • Unconditionally passes in the FencedFrameConfig's urn when navigating the internal document, ensuring that the navigation will find the associated fenced frame config, and instantiate a fenced frame config instance as expected.
  • Removes the url field from the FencedFrameConfig in favor of using the mapped url in the fenced frame config/fenced frame config instance.
  • Adds a method to store a new FencedFrameConfig directly in the finalized mapping without first putting it in the pending mapping.

This PR also adds a default fenced frame effective sandboxing flags constant, which is needed when building the fenced frame config in the FencedFrameConfing(url) constructor.

See issue: #178


Preview | Diff

@blu25 blu25 changed the title [Spec] Fix navigation from the default constructor doesn't install a fenced frame config. [Spec] Fix navigation from the default constructor not installing a fenced frame config. Aug 19, 2024
@blu25 blu25 requested a review from domfarolino August 26, 2024 15:03
@domfarolino
Copy link
Collaborator

I assumed the changes described here would match the implementation but that doesn't seem to be the case in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc;l=518-535;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa, or the constructor which doesn't store or create an underlying config in default mode, right?

@blu25
Copy link
Collaborator Author

blu25 commented Aug 27, 2024

I assumed the changes described here would match the implementation but that doesn't seem to be the case in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc;l=518-535;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa, or the constructor which doesn't store or create an underlying config in default mode, right?

There's no underlying config being added to the urn/config map, but the navigation request is creating a new fenced frame properties object that gets installed in the resulting navigated document. So we do need a fenced frame config object to exist and be accessible by the inner document by the time navigation ends. I do agree that this should be done without touching the pending/finalized mapping, since those aren't necessary.

I think the way to have this match the implementation is to have the navigation algorithm directly create a new fenced frame config and put it into the target fenced frame config in the source snapshot params for embedder-initiated navigations. That will ensure that the config object is in place for this navigation type, and we can keep the mapping untouched.

@domfarolino
Copy link
Collaborator

I think the way to have this match the implementation is to have the navigation algorithm directly create a new fenced frame config and put it into the target fenced frame config in the source snapshot params for embedder-initiated navigations. That will ensure that the config object is in place for this navigation type, and we can keep the mapping untouched.

Yep this is exactly what I was thinking too.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM with small nits

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domfarolino domfarolino merged commit 223f0fe into master Oct 5, 2024
2 checks passed
@domfarolino domfarolino deleted the liam-default-config-navigate branch October 5, 2024 19:51
github-actions bot added a commit that referenced this pull request Oct 5, 2024
…nced frame config. (#183)

SHA: 223f0fe
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants