whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.12k stars 333 forks source link

Deferred fetching feedback #1715

Open annevk opened 1 year ago

annevk commented 1 year ago

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

noamr commented 1 year ago

"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 commented 1 year ago

... Fixed everything else.

noamr commented 1 year ago

Regarding activationTimeout:

Suggesting dueAfter or activationDelay but open to other suggestions.

annevk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

[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 commented 1 year ago

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 commented 1 year ago

"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 commented 1 year ago

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 https://github.com/WICG/pending-beacon/issues/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 https://github.com/WICG/pending-beacon/issues/3#issuecomment-1286397825

mingyc commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

noamr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mingyc commented 1 year ago

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

We use background sync permission https://github.com/WICG/pending-beacon/issues/3#issuecomment-1306174273 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 https://github.com/WICG/pending-beacon/issues/77