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

Deferred fetching feedback #1715

Open
14 tasks
annevk opened this issue Oct 2, 2023 · 21 comments
Open
14 tasks

Deferred fetching feedback #1715

annevk opened this issue Oct 2, 2023 · 21 comments

Comments

@annevk
Copy link
Member

annevk commented Oct 2, 2023

Feedback on #1647 to keep the overall PR in terms of comments and such somewhat smaller in size.

  • Put the new method under the existing "Fetch method" heading, but rename it to "Fetch methods". "Fetch" in this heading is not a reference to fetch(), but to the fact that the method can fetch. So "FetchLater method" doesn't make a whole lot of sense in that light.
  • FetchLaterResult needs to be [Exposed].
  • User agent is without hyphen, see Infra.
  • "Not allowed in this context" is woefully vague. Can we do better or even agree on a model here?
  • If the exceptions are meant to be DOMException that should follow the style for them.
  • "This may throw an exception." we generally don't state this.
  • "If request’s URL is not a potentially trustworthy url," This is a Mixed Content consideration and should be caught by the network layer, not the API.
  • "exists then" missing comma.
  • "DOMHighResTimeStamp then" missing comma.
  • "0 then" missing comma.
  • I would put an <hr> before the deferred fetch record definition.
  • Why are some request body checks made inside fetchLater() and some inside "request a deferred fetch"?
  • "The user agent may process deferred fetches at any given moment" This seems wrong? I suppose it's only when the browser expects that the fetch groups will die? (Unfortunately fetch groups are not well-defined...)
  • "process deferred fetches for fetchGroup." needs uppercase P at the start.
@annevk annevk mentioned this issue Oct 2, 2023
4 tasks
@noamr
Copy link
Contributor

noamr commented Oct 3, 2023

"If request’s URL is not a potentially trustworthy url," This is a Mixed Content consideration and should be caught by the network layer, not the API.

How do I do that synchronously? I basically want to allow only HTTPS+localhost-ish and throw immediately if that's not the case.

@noamr
Copy link
Contributor

noamr commented Oct 3, 2023

... Fixed everything else.

@noamr
Copy link
Contributor

noamr commented Oct 3, 2023

Regarding activationTimeout:

  • Usually timeouts mean "no earlier than" (e.g. setTimeout), while here we mean "no later than", which is more of a deadline.
  • Deadline sounds a bit rigid, because here the browser can still delay further than the deadline.

Suggesting dueAfter or activationDelay but open to other suggestions.

@annevk
Copy link
Member Author

annevk commented Oct 4, 2023

How about activationBefore? Nicely complements fetchLater.

Let me also copy in @domenic as the person having last touched timers in HTML and likely having relevant opinions.

@noamr
Copy link
Contributor

noamr commented Oct 6, 2023

How about activationBefore? Nicely complements fetchLater.

Actually it would be activateAfter rather than before.
"Fetch later, but activate after X ms" sounds like the right story-ish.

@noamr
Copy link
Contributor

noamr commented Oct 6, 2023

[copying from Matrix]

The reason why I like activateAfter:

The fetch could be activated after the deadline or before, but for different reasons. The timeout duration is like a preference. The UA would activate before the timeout in case of termination or some such, and after the activation timeout for throttling/batching.

The word "timeout" is maybe confusing because it's the opposite of the abort signal's timeout
So activateAfter means the same as activationTimeout but it's clearer that it's "a timeout after which activation should take place" rather than "A timeout after which activation expires" or something like that.

/cc @smaug---- @mingyc @fergald

@domenic
Copy link
Member

domenic commented Oct 12, 2023

I was asked to provide feedback but since I have been purposefully not paying attention to the giant thread #1647, I don't have much to provide. For example I have no idea what "activation" or "activate" concept is being discussed.

I'm happy to leave this to my colleagues, as I don't think the value I can provide here is outweighed by the amount of time it would take me to get up to speed on this API.

@noamr
Copy link
Contributor

noamr commented Oct 24, 2023

"Not allowed in this context" is woefully vague. Can we do better or even agree on a model here?"

I think this is the main remaining issue. @mingyc what is the model for this in chromium? Do we use the backgorund-fetch permission?

@mingyc
Copy link

mingyc commented Oct 25, 2023

If the user agent has determined that deferred fetching is not allowed in this context, then throw a NotAllowedError .

what is the model for this in chromium? Do we use the backgorund-fetch permission?

I thought we should use permission policy as described in WICG/pending-beacon#77. The proposal is by default it should be enabled. It is currently not implemented in chromium though.

Unrelated to this topic, but background-sync permission is used to tell if a pending deferred fetch request can still be pending after navigating away from a page (in bfached). See also WICG/pending-beacon#3 (comment)

@mingyc
Copy link

mingyc commented Oct 25, 2023

f the user agent has determined that deferred fetching is not allowed in this context, then throw a NotAllowedError .

After some offline discussions, we think it's fine to remove this check for now, as there is currently no permissions policy defined for other keepalive requests (fetch, sendBeacon()).

We we get more feedback about providing control on this, the policy should be implemented not just for FetchLater, but for other keepalive requests.

@noamr
Copy link
Contributor

noamr commented Oct 25, 2023

OK, as discussed at TPAC, sending a NotAllowedError for contexts that are cross-origin from the top-level (e.g. 3p iframes), but not requiring a particular permission. I think all the checkboxes are complete now.

@fergald
Copy link

fergald commented Oct 25, 2023

Is there some record of the TPAC discussion? I don't understand why we would ever deny a call to fetchLater when the frame can use fetch with/without keep-alive or sendBeacon. This just seems like it will push people away from using this and they will use something that is worse for users (e.g. sending immediately).

If someone wants to propose a permissions policy that controls all keep-alive-like fetches, that would be consistent. I would also want to understand why that would be desirable.

@noamr
Copy link
Contributor

noamr commented Oct 25, 2023

Is there some record of the TPAC discussion? I don't understand why we would ever deny a call to fetchLater when the frame can use fetch with/without keep-alive or sendBeacon. This just seems like it will push people away from using this and they will use something that is worse for users (e.g. sending immediately).

If someone wants to propose a permissions policy that controls all keep-alive-like fetches, that would be consistent. I would also want to understand why that would be desirable.

It's here, though I didn't participate in this discussion.

@fergald
Copy link

fergald commented Oct 25, 2023

I don't see anything about that there. Maybe it happened outside the room. Do you know who suggested it?

@noamr
Copy link
Contributor

noamr commented Oct 25, 2023

I don't see anything about that there. Maybe it happened outside the room. Do you know who suggested it?

It came up in the WHATNOT meeting last week as something that was raised in TPAC. Finding out. I'm OK with not having this restriction.

@annevk
Copy link
Member Author

annevk commented Nov 8, 2023

I thought this capability was going to be more powerful than keepalive, but maybe that is not the case? What exactly are we providing that websites cannot do today?

It was with the understanding that it was going to be more powerful that the idea came up that it perhaps should be constrained in some manner.

Also, keepalive was added in 2016, before widespread acknowledgment that we need to partition state and perhaps restrict cross-site documents in certain manners. So even if this is strictly as capable as keepalive we will need to consider if the existing keepalive non-restrictions still make sense.

@fergald
Copy link

fergald commented Nov 8, 2023

It is no more powerful in the sense that there is no new information that you can extract with this API. It is more reliable and as a result can reduce the number of requests made since now you can safely allow data to accumulate instead of eagerly shipping it home, knowing that it will be sent.

If we restrict this, devs are just going to use fetch() without keepalive and they are going send data eagerly to avoid it being lost. There are no limits at all on payload size for fetch()s. That seems strictly worse.

Is there a specific concern that you are trying to guard against?

@annevk
Copy link
Member Author

annevk commented Nov 8, 2023

Allowing fetches beyond the lifetime of the top-level navigable is a big deal and we're not willing to give unfettered access to it. Because that will impact overall system performance. Giving unfettered access to this from the least trusted documents within a website by default seemed like a non-starter and when we discussed this way-back-when with @yoavweiss we said as much. Permission policy delegation seems reasonable, but there will have to be some kind of upper limit.

@yoavweiss
Copy link
Collaborator

The TPAC discussion did not include anything on limiting this to top-level origin, and I don't remember this being discussed.

IIRC in the past we talked about restricting this behind a background fetch permission, as both have similar characteristics. We also discussed an upper limit per beacon origin, to avoid sharing a quota amongst different origins.

@annevk
Copy link
Member Author

annevk commented Nov 8, 2023

I'm not talking about TPAC, I'm talking about the discussion we had when this was still PendingBeacon.

@mingyc
Copy link

mingyc commented Nov 9, 2023

in the past we talked about restricting this behind a background fetch permission, as both have similar characteristics.

We use background sync permission WICG/pending-beacon#3 (comment) to decide whether a fetchLater can stay after navigation.

I'm talking about the discussion we had when this was still PendingBeacon.

We didn't have discussion about restricting with permission policy until moving toward fetch-based approach. There is a tracking issue with no feedback WICG/pending-beacon#77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants