这是indexloc提供的服务，不要输入任何密码
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Oct 5, 2024
Merged

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

merged 11 commits into from
Oct 5, 2024

Conversation

@blu25
Copy link
Collaborator

@blu25 blu25 commented Aug 19, 2024 •
edited by pr-preview bot
Loading

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

domfarolino commented Aug 26, 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?

@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

domfarolino commented Sep 19, 2024

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.

domfarolino
domfarolino reviewed Sep 19, 2024
View reviewed changes
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

domfarolino
domfarolino reviewed Sep 21, 2024
View reviewed changes
domfarolino
domfarolino reviewed Oct 2, 2024
View reviewed changes
domfarolino
domfarolino reviewed Oct 4, 2024
View reviewed changes
@blu25 blu25 mentioned this pull request Oct 4, 2024
Open
@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
@blu25 @github-actions
Fix navigation from FencedFrameConfig constructor not installing a fe…
03370d4 
…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

Reviewers

@domfarolino domfarolino domfarolino approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@blu25 @domfarolino