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 the ACHL cache and ACHL/ACH processing #3774

Open
wants to merge 19 commits into
base: master
from

Conversation

4 participants
@yoavweiss
Copy link
Collaborator

commented Jun 26, 2018

This PR replaces whatwg/fetch#729 in resolving whatwg/fetch#726, which as discussed should move to HTML. Rough design sketch is also available.

This initial PR still doesn't resolve everything that was previously discussed (e.g. doesn't clear the ACHL cache along with cookies just yet), but I'd love to get an early opinion on it, to see if this is the right direction.


/browsing-the-web.html ( diff )
/dom.html ( diff )
/images.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/offline.html ( diff )
/references.html ( diff )
/semantics.html ( diff )
/webappapis.html ( diff )
/window-object.html ( diff )
/workers.html ( diff )

@domenic
Copy link
Member

left a comment

Lots of editorial things, and one big model confusion.

Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
source Outdated
<li><dfn data-x="concept-accept-ch-lifetime-cache-max-age"
data-definition-for="concept-accept-ch-lifetime-cache">max-age</dfn> (a number of seconds)</li>
<li><dfn data-x="concept-accept-ch-lifetime-cache-client-hints-list"
data-definition-for="concept-accept-ch-lifetime-cache">client-hints-list</dfn></li>

This comment has been minimized.

Copy link
@domenic

domenic Jun 26, 2018

Member

"(a list of X)" would be good

This comment has been minimized.

Copy link
@yoavweiss

yoavweiss Jun 26, 2018

Author Collaborator

I thought it should match (or eventually replace) https://fetch.spec.whatwg.org/#client-hints-list

This comment has been minimized.

Copy link
@domenic

domenic Jun 26, 2018

Member

Oh interesting. Then it should probably be "(a client hints list)" which links to that definition. Although, that definition might be best changed to a set, not a list.

Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
source Outdated
settings object a secure context?</span> on the <code>Window</code> object's
environment settings object.</p></li>

<li><p>If <var>response</var>'s

This comment has been minimized.

Copy link
@domenic

domenic Jun 26, 2018

Member

This model seems kind of broken.

What is a settings object's clients hints list? It's obtained by the algorithm "retrieve client-hints list", which creates a new list each time it's called. So, appending to it has no effect.

You seem to have mixed together two models which contradict each other:

  • Client hints list is figured out from preexisting data ("retrieve client-hints list" figures out some subset of the Accept-CH lifetime cache that applies)
  • Client hints list gets modified directly during Document creation

This comment has been minimized.

Copy link
@yoavweiss

yoavweiss Jun 27, 2018

Author Collaborator

So the model I had in mind is:

  • Accept-CH adds client hints to the current env settings object's client-hints list.
  • After that, an Accept-CH-lifetime header stores entries in the ACHL cache, based on the current env settings object.
  • When an env settings object is created, it's initial client-hints list is populated from the cache.

"retrieve client hints list" indeed creates a new list, but then populates it with of all cached hints from the cache (as there could be multiple entries which may correspond the current origin, with different hints on each.)

Does that make sense?

@yoavweiss

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2018

@domenic - thanks for reviewing! :) I addressed most of your comments, and left TODOs for the rest, which I intend to tackle a bit later (but before landing)

@domenic

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

(Continuing this discussion away from now-collapsed commit comments:)

So the model I had in mind is:

  • Accept-CH adds client hints to the current env settings object's client-hints list.
  • After that, an Accept-CH-lifetime header stores entries in the ACHL cache, based on the current env settings object.
  • When an env settings object is created, it's initial client-hints list is populated from the cache.

"retrieve client hints list" indeed creates a new list, but then populates it with of all cached hints from the cache (as there could be multiple entries which may correspond the current origin, with different hints on each.)

Does that make sense?

It could make sense, but as written doesn't really work. Every time you say "some settings object's client-hints set", you run the algorithm specified in https://whatpr.org/html/3774/window-object.html#script-settings-for-window-objects , which re-creates a new list from scratch. So any additions you make to it (via Accept-CH) are only to that list, and are never seen again by any future callers of "some settings object's client-hints set".

If you want a single list that's created once, and then stored and returned later, you need to explicitly do that.

Relatedly, it seems a bit strange that you intend to create one client hints set per environment settings object (<-> Window), whereas all the other similar stuff like origin, HTTPS state, and referrer policy are associated with the Document. The main difference here is how you treat the transition from initial about:blank to a new Document within the same window. As written right now, if I add <meta> elements to the initial about:blank, then navigate to https://example.com/, the client hints that were set by those <meta> elements in about:blank will still apply to https://example.com/. That doesn't seem right to me?

I'd thus suggest a framework that more closely follows referrer policy or HTTPS state:

  • Documents and WorkerGlobalScope instances get a client hints set, initially empty. (This is the only time we ever create client hints sets.)
  • The environment settings object algorithms return that set. (The purpose of this is to provide a small indirection so that fetch only ever has to worry about clients = environment settings objects, not about Documents vs. WorkerGlobalScopes vs. ...)

Then I think this would work:

  • When creating a document or workerglobalscope, "retrieve the client hints set" for the correct origin, and then loop over the result and append them to the document/WGS's client hints set.
  • Accept-CH adds client hints to the Document/WGS's client hints set. (This now works, because running the "get client hints set" algorithm doesn't return a fresh set each time anymore, so mutating it has the desired effect.)

Thoughts?

@yoavweiss

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2018

@domenic makes perfect sense! Working on changing the model. Will be ready for re-review tomorrow.

@yoavweiss

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2018

@domenic technically, it's tomorrow for me :D I think I fixed all the comments. Can you PTAL?

I still need to update the corresponding Fetch PR, to rename its client hints list to client hints set

source Outdated
@@ -97614,6 +97877,14 @@ interface <dfn>SharedWorkerGlobalScope</dfn> : <span>WorkerGlobalScope</span> {

</dd>

<dt>The <span data-x="concept-settings-object-client-hints-set">client-hints set</span></dt>

This comment has been minimized.

Copy link
@yoavweiss

yoavweiss Jun 28, 2018

Author Collaborator

@domenic - I'm not sure this is what you meant by proxying the WGS's client-hints set through the settings object's client-hints set. Would that work?

@domenic

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

OK, I did a pass through for a final review that was mostly editorial. Please check my changes and let me know what you think.

I'd still prefer to hold off on this until @annevk has time to look at it from a higher conceptual/architectural. But, I think it's pretty solid at the level that I can understand.

@domenic domenic dismissed their stale review Jul 2, 2018

All good now

@yoavweiss
Copy link
Collaborator Author

left a comment

@domenic - Thanks for the fix-ups!

source Outdated
given <var>origin</var>, <var>max-age</var>, and <var>client-hints set</var>,
add a new entry with the following fields to the Accept-CH-Lifetime cache:
<p>To <dfn>add a new Accept-CH-Lifetime cache entry</dfn> to the <span
data-x="concept-accept-ch-lifetime-cache">Accept-CH-Lifetime cache</span>, given

This comment has been minimized.

Copy link
@yoavweiss

yoavweiss Jul 3, 2018

Author Collaborator

Oops, changed the name here, but missed changing the link and the callers. Thanks for fixing!

@yoavweiss

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2018

@domenic - Thanks for reviewing and fixing up. Your changes look good from my perspective. Waiting for @annevk to land the PRs makes sense.

@annevk
Copy link
Member

left a comment

Looks good overall, I think, but lots of tiny issues remain as far as I can tell.

Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
source Outdated

<ol>
<li><p>If the <code>meta</code> element is not a child of a <code>head</code> element, then
return.</p></li>

This comment has been minimized.

Copy link
@annevk

annevk Aug 13, 2018

Member

Shouldn't this be more strict so it only works for head element that is the first element child of the root element? What do implementations do here? Do we test the difference?

This comment has been minimized.

Copy link
@yoavweiss

yoavweiss Aug 23, 2018

Author Collaborator

Copied that piece from CSP. Maybe worth while to coalesce that logic into a single place. I don't think current tests make sure this (or CSP) only work for a head element that's the first child of root

source Outdated
<code data-x="attr-meta-content">content</code> attribute's value.</p></li>

<li><p>Parse <var>acceptCHValue</var> according to the `<code>Accept-CH</code>` header parsing
rules, as a <span data-x="concept-http-field-name">field-name</span>. Append each parsed

This comment has been minimized.

Copy link
@annevk

annevk Aug 13, 2018

Member

Isn't it a field value?

This comment has been minimized.

Copy link
@yoavweiss

yoavweiss Aug 23, 2018

Author Collaborator

It's defined as field-name at https://httpwg.org/http-extensions/client-hints.html#accept-ch
That makes sense as the values accepted are request header names

This comment has been minimized.

Copy link
@annevk

annevk Jan 22, 2019

Member

So it's defined as a sequence of field-names, but that doesn't seem conveyed here? In particular the # is significant.

Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated
Show resolved Hide resolved source Outdated

@yoavweiss yoavweiss force-pushed the yoavweiss:ACHL_cache2 branch from 7bd134e to 44b682a Mar 15, 2019

yoavweiss added some commits Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.