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

Consider setting background attributionsrc request priority to low #809

Open
yoavweiss opened this issue May 17, 2023 · 13 comments · May be fixed by #812
Open

Consider setting background attributionsrc request priority to low #809

yoavweiss opened this issue May 17, 2023 · 13 comments · May be fixed by #812
Assignees
Labels
compat issue that may affect web compatibility

Comments

@yoavweiss
Copy link
Contributor

In Make a background attributionsrc request, we're defining a request, but it doesn't seem like we're setting its priority to low, nor setting it to a "keepalive" one. What makes it a background request? Any guardrails against such requests competing on bandwidth with more critical ones?

@apasel422 apasel422 self-assigned this May 17, 2023
@apasel422
Copy link
Collaborator

It's true that the term background doesn't have much meaning here, but we do set keepalive true on these requests.

@apasel422 apasel422 assigned yoavweiss and unassigned apasel422 May 17, 2023
@yoavweiss
Copy link
Contributor Author

You're right, I missed the keepalive bit. Might also be worthwhile to set their priority to low.

@apasel422 apasel422 changed the title What makes the request a "background" one in "make a background attributionsrc request"? Consider setting background attributionsrc request priority to low May 17, 2023
@apasel422
Copy link
Collaborator

Might also be worthwhile to set their priority to low.

What would the consequences of that be? The Fetch spec seems to propagate "priority" and "internal priority" but doesn't explain how they're used.

@yoavweiss
Copy link
Contributor Author

They are used internally by the browser to set an implementation defined priority both in internal queues as well as on the network (eventually translated to H3 priorities)

@apasel422
Copy link
Collaborator

Thanks. Perhaps the Fetch spec could have some kind of non-normative guidance on when to use each value.

@csharrison
Copy link
Collaborator

@yoavweiss do you expect setting the priority to low could impact things like network loss or dropped requests? I want to understand the consequences a bit deeper.

@yoavweiss
Copy link
Contributor Author

^^ @pmeenan

I wouldn't expect it to result in dropped requests other than cases where the reports would be currently sent right before the sender is killed. Depending on the implementation, the sender could be either the renderer or the browser process.

The tradeoff here is that if the requests are not marked as low priority, they could get in the way of requests for user visible content.

Another question that impacts priority, CSP, etc is - what should be these request's destination?

@apasel422
Copy link
Collaborator

Another question that impacts priority, CSP, etc is - what should be these request's destination?

Based on https://fetch.spec.whatwg.org/#destination-table the empty string seems most relevant.

@csharrison
Copy link
Collaborator

csharrison commented May 17, 2023

Another question that impacts priority, CSP, etc is - what should be these request's destination?

I'm not sure how that should be chosen but maybe following the ping attribute makes sense which uses the empty string: https://html.spec.whatwg.org/multipage/links.html#hyperlink-auditing

edit: oops race condition with @apasel422 :)

@csharrison
Copy link
Collaborator

I wouldn't expect it to result in dropped requests other than cases where the reports would be currently sent right before the sender is killed. Depending on the implementation, the sender could be either the renderer or the browser process.

Ack, this may impact us depending on the implementation given that it isn't unusual for these background requests to be made as the context is being navigated away (e.g. the user clicks on an ad). Before making this change I'd want to double check the impact here.

@dmdabbs
Copy link
Contributor

dmdabbs commented May 17, 2023

Regarding priority I've noticed that the 'background' fetch is always High priority.
And the order in which Chrome fetches (judging by DevTools Network) depends on how the DOM element is added.

As inline markup, the 'foreground' is always first.
<img src="https://www.google.com/gen_204?ara=foreground" attributionsrc="https://www.google.com/gen_204?ara=background">

As injected markup the 'background' is first, again judging by Network listing.

document.getElementById('somediv').innerHTML = '<img src="https://www.google.com/gen_204?ara=foreground" attributionsrc="https://www.google.com/gen_204?ara=background">'

image

This may vary by element type, but I didn't compare supported elements.

@csharrison csharrison added the compat issue that may affect web compatibility label Jun 26, 2023
@yoavweiss
Copy link
Contributor Author

I don't think the compat label is required here, as this is something we can easily change after shipping, and have no reason to believe would result in site breakage.

@csharrison
Copy link
Collaborator

I don't think the compat label is required here, as this is something we can easily change after shipping, and have no reason to believe would result in site breakage.

I added this out of an abundance of caution because we would want to roll this out carefully and monitor potential loss in the APIs. I agree the impact is probably minimal so it might be overkill, but it's worth being a bit more careful IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat issue that may affect web compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants