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

Define top level origin of a document #4966

Merged
merged 37 commits into from
Mar 11, 2020
Merged

Define top level origin of a document #4966

merged 37 commits into from
Mar 11, 2020

Conversation

shivanigithub
Copy link
Contributor

@shivanigithub shivanigithub commented Oct 3, 2019

Adds the definition of the top level origin of a resource request.
This is based on feedback from whatwg/fetch#943


💥 Error: write EPROTO 139802362353536:error:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol version:../deps/openssl/openssl/ssl/s3_pkt.c:1494:SSL alert number 70

139802362353536:error:1409E0E5:SSL routines:ssl3_write_bytes:ssl handshake failure:../deps/openssl/openssl/ssl/s3_pkt.c:659:
💥 ###

PR Preview failed to build. (Last tried on Mar 11, 2020, 8:55 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 HTML Diff Service - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request.

🔗 Related URL

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@annevk
Copy link
Member

annevk commented Oct 7, 2019

I don't really know what your timezone is, but it might help if you had a quick chat with @domenic about this. I work Berlin office hours, if that helps.

In short, what's needed here is to define new state on document, analogous to something like "CSP list". And then Fetch and others get to inspect that to make decisions.

@shivanigithub
Copy link
Contributor Author

I am in Eastern Standard Time. Looks like Domenic is OOO.
Do you mean adding something in this section:
https://html.spec.whatwg.org/multipage/dom.html#the-document-object? If yes, while making the change in this section, does the current thinking in this PR make sense to you: dividing the scenarios as top-frame navigation resource and subframe navigation resource while computing the top-level origin?

@annevk
Copy link
Member

annevk commented Oct 7, 2019

I think we need to set the field when creating a document (note that there's a couple places where this happens). And yes, I suspect for now we will need to look at ancestors rather than ancestors passing something down.

@shivanigithub shivanigithub changed the title Define top level origin of a resource request Define top level origin of a document Oct 10, 2019
@shivanigithub
Copy link
Contributor Author

The latest patch has the top level origin computation in the navigation section now. I kept it right after "determining the origin" as they both are origin states on the document and "determining the top level origin" is dependent on the value returned from "determining the origin". PTAL, thanks!

@domenic
Copy link
Member

domenic commented Oct 15, 2019

I'm back from vacation and trying to help, but I'm not sure I really understand the intent here. I can give minor feedback on this PR, e.g. I'll note that it seems to operate on browsing contexts which are not the same as documents, but I'm not sure what the goal is here.

Perhaps as a step back, @annevk, I'm unsure why you're insisting that this be "state" on the document, instead of an algorithm similar to the one defined here. Currently this one takes a browsing context, but if it got redefined to operate on documents, it seems like it would work reasonably?

I'm also unsure how this is helpful for whatwg/fetch#943. Fetches are generally associated with clients, not with documents (or browsing contexts). For example, what is the desired behavior in a worker (i.e. where the client of the fetch is a worker environment settings object)? It seems like you'd really want to go from client -> HTTP cache, which might again require different techniques. Perhaps even different techniques depending on the type of worker!

@annevk
Copy link
Member

annevk commented Oct 15, 2019

It could work reasonably if there's a way to get to a parent document without involving a browsing context as otherwise you might get the wrong top-level document for a bfcache document. (I don't think we have that infrastructure at the moment, but we should add it.)

@shivanigithub
Copy link
Contributor Author

Had a discussion with @domenic and here are the main points discussed. I am working on creating the next patch based on this.

@shivanigithub
Copy link
Contributor Author

Added the environment settings and document level changes in the latest patch. PTAL, thanks!

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.

Sorry for the delay; I'll try to be more responsive going forward.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@shivanigithub
Copy link
Contributor Author

Thanks for the feedback.
This got a little delayed but I should be able to get to these this week.

@shivanigithub
Copy link
Contributor Author

Sorry for the delay. Addressed the feedback now. Thanks!

@domenic
Copy link
Member

domenic commented Nov 20, 2019

I fixed some formatting nits; please take a look at the latest commit and be sure to pull it down before adding more.

It seems like the main work left here is that you need to ensure all settings objects have a top-level origin. This includes updating the other call site for "set up a window environment settings object", i.e. making sure new popup windows have a top-level origin set for themselves, and also updating workers.

From what I understand we might not yet know the right answer for workers. That's OK. In that case, we should still have https://html.spec.whatwg.org/#set-up-a-worker-environment-settings-object add a "The top-level origin" entry, but its contents can be a <p class="XXX"> explaining that we are still figuring out what this should be.

@annevk
Copy link
Member

annevk commented Nov 21, 2019

I'm curious what you both think about building this on top of #5091's container document. With that we could define a document's parent document and a document's top-level document on top of that and those would provide a bfcache-safe access to the top-level origin (the origin of a document's top-level document).

The main wrinkle is that if you were to perform same origin-domain comparisons with this origin you'd be in a trouble (as mutations to the origin are generally ignored, but observed by that comparison). But I don't think we have use cases for that and we might also wanna call out explicitly to not do that, regardless of whether we use this design or copying down the tree.

We could also use top-level document for other concepts as long as they are immutable. Policies having to be immutable makes this less attractive as a general mechanism I suspect as unfortunately we have many mutable policies. So I guess we'll at least have some copying down, at which point maybe we should try to fit everything into that scheme long term. Hmm. Not sure, would love your input.

@shivanigithub
Copy link
Contributor Author

@annevk : I'm in the reading #5091 but in the meantime have one question regarding your comment here: https://github.com/whatwg/html/pull/5091#issuecomment-554419563
In the scenario you discussed there, as per the changes in this PR, B's top-level origin will be saved as A1 and not C since B's top-level origin will be computed at the time of navigation commit for B.
Does that sound right?

@shivanigithub
Copy link
Contributor Author

Thinking a bit more about this, the eager approach seems more aligned with what we want, since we would not like the partition of a frame to change during its lifetime.
Will upload the new patch, although I will be slow since I am OOO for the next few weeks.

@domenic
Copy link
Member

domenic commented Jan 29, 2020

I wonder though whether this is intentionally allowed or something that ought to be spec'd

My suspicion is that preventing fetches from such discarded contexts is something that we should work to spec, eventually. In fact I believe we've vaguely known about it for a long time, but nobody has invested the effort to do a proper test matrix of types of fetches × different browsers × types of shutdowns × other factors in order to confidently suggest a way forward. (For example, what about sendBeacon or keepalive fetches, which are intentionally supposed to outlive a document? Or img fetches, which historically have done so and analytics vendors rely on? Maybe we could still disallow starting one of those from a detached document, as long as we allowed it to continue once started...)

However, I don't think it's reasonable to block any of this work on figuring out that long-standing problem.

Thinking a bit more about this, the eager approach seems more aligned with what we want

That works for me! It probably does help sidestep this whole mess in the best way.

My other suggestion was going to be adding some kind of Assert or <p class="XXX"> in the Fetch spec that says something like "this might sometimes be null per spec, but in implementations we suspect such fetches get shut down earlier", and maybe linking to an open bug about that whole problem. But, I think switching to the eager approach will be better.

@shivanigithub
Copy link
Contributor Author

Changed to using the eager approach as discussed. PTAL, thanks!

@shivanigithub
Copy link
Contributor Author

Friendly ping.

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.

This looks great. I pushed some minor nit fixes, but this looks really solid to me.

@annevk, can we trouble you for another look?

@shivanigithub
Copy link
Contributor Author

@annevk : friendly ping.

@shivanigithub
Copy link
Contributor Author

Thanks Domenic!

@shivanigithub
Copy link
Contributor Author

@annevk : friendly ping.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks @shivanigithub, this setup seems good. There's a couple of minor issues left however.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@shivanigithub
Copy link
Contributor Author

Addressed feedback. PTAL, thanks!

source Show resolved Hide resolved
@@ -84115,8 +84131,14 @@ interface <dfn>Location</dfn> { // but see also <a href="#the-location-interface
</ul>
</li>

<li><p>Let <var>topLevelOrigin</var> be <var>origin</var> if <var>browsingContext</var> is
<span>top-level browsing context</span>, otherwise let <var>topLevelOrigin</var> be
<var>browsingContext</var>'s <span>top-level browsing context</span>'s
Copy link
Member

Choose a reason for hiding this comment

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

Same here, it seems using the container document would be sufficient and require less going up the tree (though it might end up with more words I suppose).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A question on how does "browsing context's top-level browsing context" work? If it's such that every browsing context has a reference to its top-level browsing context then we are not really going up the tree here, but just accessing that reference? Searching other places that invoke "browsing context's top-level browsing context", I see examples like 1, 2, 3, etc.

I am not sure how container document would be better, since container document will lead to the parent and we'll have to continue to iterate till the last parent, right?

Copy link
Member

Choose a reason for hiding this comment

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

I agree; I don't see why container document would be better. Top-level browsing context is defined straightforwardly in a way that bottoms out in container document; it's a nice abstraction on top that saves us some typing. (Explicitly: TLBC uses ancestor which uses child browsing context which uses container document.) I don't see why we would avoid using that abstraction when it gives us exactly what we want.

Copy link
Member

Choose a reason for hiding this comment

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

I think grabbing the active document of the top-level browsing context is the problematic aspect here. It might well be changing due to race conditions. Container document could become null I suppose, but that seems safer than a different document.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how a race condition could be involved. This step is atomic. If you're worried about things spontaneously changing during a single step, both approaches seem equally problematic.

Copy link
Member

Choose a reason for hiding this comment

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

It's currently atomic because the spec is rather blind to site isolation. It also seems nicer to me if top-level origin works recursively.

@annevk
Copy link
Member

annevk commented Mar 10, 2020

I thought about it some more and I'm okay with this being merged as the active document stuff is an existing problem that needs auditing either way and the new instances are fairly isolated and also happen at clear points in time.

@domenic
Copy link
Member

domenic commented Mar 10, 2020

Thanks for being flexible :). Is your other comment outstanding, or is this good to merge? And do you have any thoughts about whether we should merge this now/ASAP, vs. waiting for whatwg/fetch#943? I'm unsure myself; it's a standalone piece of infrastructure, but it has no consumers until the Fetch PR is merged.

@annevk
Copy link
Member

annevk commented Mar 10, 2020

It's good to merge. I think it's okay to merge now. I suspect there are many more dependencies we haven't identified yet, as browsers do plenty of things differently for third parties these days, and this is the primitive you want to tell a third party from a first party.

@shivanigithub
Copy link
Contributor Author

Thanks @annevk !

@domenic domenic merged commit 916a923 into whatwg:master Mar 11, 2020
domenic added a commit to domenic/css-houdini-drafts that referenced this pull request Mar 31, 2020
Follows whatwg/html#5411. Also includes a top-level origin, introduced in whatwg/html#4966.
bfgeek pushed a commit to w3c/css-houdini-drafts that referenced this pull request Apr 6, 2020
Follows whatwg/html#5411. Also includes a top-level origin, introduced in whatwg/html#4966.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants