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

Use the newly exposed methods to initialize feature policy. #4772

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

dtapuska
Copy link
Contributor

@dtapuska dtapuska commented Jul 12, 2019

Instead of delegating to the feature policy spec to initialize feature
policy after the document is created initialize feature policy first
and pass it into the constructor of the document.


/browsers.html ( diff )
/browsing-the-web.html ( diff )
/infrastructure.html ( diff )

@dtapuska
Copy link
Contributor Author

This requires w3c/webappsec-permissions-policy#324 to land first though.

@clelland PTAL

Instead of delegating to the feature policy spec to initialize feature
policy after the document is created initialize feature policy first
and pass it into the constructor of the document.
@dtapuska
Copy link
Contributor Author

@annevk WDYT?

@clelland
Copy link
Contributor

This looks reasonable; I can't see any obvious issues with this ordering. (And I just merged w3c/webappsec-permissions-policy#324, so we can call the algorithms this way)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM, seems like just an editorial change. Will give @annevk another day or so if he wants but I think it's pretty simple.

@dtapuska
Copy link
Contributor Author

I'd like both spec editors to be aware of this pattern I'm introducing. Constructing things before the Document is created. The current COEP proposal and CSP also suffer from this same problem that feature policy had, those will need to be fixed at some point.

dtapuska added a commit to dtapuska/webappsec-feature-policy that referenced this pull request Jul 16, 2019
The HTML spec was updated in whatwg/html#4772

Now the deprecated algorithms can be removed.
@annevk
Copy link
Member

annevk commented Jul 17, 2019

So the reason the browsing context is passed in is to look at ancestor browsing contexts?

I don't quite like that this means we'd have to potentially do cross-process messaging thrice. Once to start the navigation and then twice more to get additional data. I think Ideally "navigate" is passed all the state it needs at navigation time.

Another thing with this setup is that it means that Feature Policy (or whatever we end up calling it) ends up being strictly less powerful than COOP, which can cause the browsing context to be replaced. This is probably okay for now, but perhaps worth revisiting at some point.

(Another concern I have is that there's no mutual agreement on much of Feature Policy at this time, so this goes against the WHATWG Working Mode a bit, but I'm not going to block this on that.)

@dtapuska
Copy link
Contributor Author

This change is definitely purely editorial and the issue currently exists in the current spec. Yes the browser context determines inheritence. I believe the fact how cross-process messaging is done is an implementation detail. In Chrome we have the feature policy on the frame tree, so it is mirrored into every process so the query of inheritence is not a cross-process call.

@annevk
Copy link
Member

annevk commented Jul 17, 2019

The main problem I have with the model is this timeline:

  1. You navigate the child.
  2. Some time later but before the child is navigated you change state in the parent.
  3. The child finishes up navigation, does the changed state since the act of navigation take effect? I think it would be bad if it did as it's highly non-deterministic. (I realize that in the specific case of Feature Policy this might not matter (yet), but you asked about the general setup and I replied to that.)

If you want to land this change and track the problem separately, that's okay with me, but I don't think we should ignore it.

@clelland
Copy link
Contributor

@annevk, would you prefer to see a model where initiating the navigation bundles up all of the state at that time that will be needed to eventually create the new document?

You'd mentioned something along those lines in w3c/webappsec-permissions-policy#256, and it's probably worth fixing at some point.

As long as we can define exactly which statements trigger navigation, and when that gets snapshotted (on "process the iframe attributes", or at the end of the current JS function, or whether we queue a microtask), then we should be able to get rid of the race.

@annevk
Copy link
Member

annevk commented Jul 18, 2019

Yeah, I think we are there (at least planning-wise) so we can start doing this.

@annevk
Copy link
Member

annevk commented Jul 18, 2019

Happy to see this land with my newline removal.

@domenic domenic merged commit 2a397f6 into whatwg:master Jul 18, 2019
clelland pushed a commit to w3c/webappsec-permissions-policy that referenced this pull request Jul 18, 2019
The HTML spec was updated in whatwg/html#4772

Now the deprecated algorithms can be removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants