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

Use the keepalive flag when fetching images #1655

Open
domenic opened this issue Aug 11, 2016 · 13 comments
Open

Use the keepalive flag when fetching images #1655

domenic opened this issue Aug 11, 2016 · 13 comments
Labels
integration Better coordination across standards needed topic: fetch topic: img

Comments

@domenic
Copy link
Member

domenic commented Aug 11, 2016

https://fetch.spec.whatwg.org/#keepalive-flag indicates the flag should be set for images, but it is not.

Maybe wait until #1643 is done.

@zcorpan
Copy link
Member

zcorpan commented Aug 12, 2016

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4372
The image fetch is canceled in Chromium/Gecko/WebKit according to devtools.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4373
The image fetch appears to not be canceled in Chromium/Gecko/WebKit.

How this is implemented I'm not sure.

@domenic
Copy link
Member Author

domenic commented Aug 12, 2016

Wat :(

@annevk annevk changed the title Use the keep-alive flag when fetching images Use the keepalive flag when fetching images Oct 25, 2016
@annevk annevk removed their assignment Jul 18, 2017
@domenic
Copy link
Member Author

domenic commented Mar 29, 2019

For the record, this was originally https://www.w3.org/Bugs/Public/show_bug.cgi?id=24597, although there isn't too much more detail there.

@yoavweiss
Copy link
Contributor

sigbjornf says this may be relevant

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/ImageLoader.cpp?q=ImageLoader::updatedHasPendingEvent&sq=package:chromium&l=519&dr=CSs

That code is no longer relevant, but I suspect its current-day equivalent is here

It seems like images are getting set to be kept alive only when the element is updated during a page dismissal event. My guess is that it's something that was added before sendBeacon() and fetch keepalive flag as a way to achieve the same impact.

@yoavweiss
Copy link
Contributor

Overall, this seems to me like something we should try to deprecate and remove, if we can.

@domenic
Copy link
Member Author

domenic commented Mar 30, 2019

I think deprecation and removal here is not likely to break pages, but is likely to cause many, many analytics providers or bespoke shutdown routines on sites to work incorrectly, and thus make page operators sad. It'd be cool if you want to try, though.

@yoavweiss
Copy link
Contributor

yoavweiss commented Apr 1, 2019

Did some archeology and it seems like this was added to WebKit back in 2009 for IE compat. This was before sendBeacon and fetch() keepalive were used to provide the same functionality. (Relevant discussion)

Agree that any removal attempt will need to collect data on current usage and evaluate the level of sadness that will be caused by it. If we conclude that removal is not feasible, we probably need to specify that behavior and add tests for it.

@emmettbutler
Copy link

@yoavweiss please correct me if I'm wrong, but it sounds like setting img.src in a beforeunload handler is a valid way to track analytics events on page unload, though perhaps not the modern preferred method. See this SO question for more specificity in what I'm asking.

Within a beforeunload handler, are img.src and sendBeacon functionally equivalent?

@igrigorik
Copy link
Member

Do we have WPT tests for this (img) behavior? I wouldn't be surprised to see this failing in a various browsers, and generally agree with Yoav that this is not something we should encourage and ideally deprecate.

@zcorpan zcorpan removed their assignment Apr 2, 2019
@zcorpan
Copy link
Member

zcorpan commented Apr 2, 2019

I don't think there are wpt for this. There's live dom viewer demos in a comment above.

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 7, 2021
As part of the discussion in [1], we wondered if it makes sense
to try to remove the special treatment that image loads have when added
as part of a dismissal event. That special treatment results in
developer confusion, as demonstrated in the related bug.

This CL adds a use counter for image loads in dismissal events,
so that we can make a better decision on that front - either standardize
the behavior, or remove it.

[1] whatwg/html#1655

Change-Id: I55935bef66b6d8b0f2f89936ab36b03fb926c5a0
Bug: 1264540
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3318037
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#949158}
@zcorpan
Copy link
Member

zcorpan commented Jan 10, 2022

@ericlaw1979 said in https://twitter.com/ericlaw/status/1480594796267937795 that CSP is affected by at-unload image requests as well (in Chromium, at least?).

Did you know: <img> requests as the page unloads are converted to a beacon-like mode and are thus controlled by the |connect-src| CSP instead?

@yoavweiss
Copy link
Contributor

Yup, relevant Chromium code

I'm less confident about this, but it seems like WebKit's code is loading the image similarly to other PING-type requests, so it's likely to impose the same CSP restrictions.

@annevk annevk added integration Better coordination across standards needed topic: fetch labels Jan 12, 2022
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
As part of the discussion in [1], we wondered if it makes sense
to try to remove the special treatment that image loads have when added
as part of a dismissal event. That special treatment results in
developer confusion, as demonstrated in the related bug.

This CL adds a use counter for image loads in dismissal events,
so that we can make a better decision on that front - either standardize
the behavior, or remove it.

[1] whatwg/html#1655

Change-Id: I55935bef66b6d8b0f2f89936ab36b03fb926c5a0
Bug: 1264540
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3318037
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#949158}
NOKEYCHECK=True
GitOrigin-RevId: ada3076b3f392fe25088f91e9e8048599777b0dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Better coordination across standards needed topic: fetch topic: img
Development

No branches or pull requests

6 participants