-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Image Loading: Remove last_base_element_url_
#38275
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Jan 31, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review process for this patch is being conducted in the Chromium project.
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-3311225
branch
from
January 31, 2023 18:15
f139ca7
to
a61e9b3
Compare
`blink::ImageLoader::last_base_element_url_` stores a snapshot of the Document's base URL during UpdateFromElement() [1]. However, this snapshot is not actually used to resolve the URL of the image request after the microtask checkpoint in DoUpdateFromElement(); rather, relative URL resolution always happens with the Document's live base URL mechanism [2] (more on this later). Instead, the member exists solely to detect, at image insertion time [3], if the Document's base URL has changed since the last time UpdateFromElement() was called. The rationale for this used to be to detect if a document's base URL changed before the image loading microtask (DoUpdateFromElement()) was run, so that we could protect against fetching the wrong image with a stale URL; see [4]. This was important because at the time, (before crrev.com/c/2103987) ImageLoader used to store a snapshot of the resolved image URL in the microtask state computed in UpdateFromElement(), so it was possible to wind up in DoUpdateFromElement() with an image request URL that is stale compared to the live Document's base URL (this was intentional, see crbug.com/569760 & https://codereview.chromium.org/2105283002). The solution [5] at the time was to detect, only during insertion, if the ImageLoader's last known snapshot of the Document base URL is stale, so that the image processing model can be re-entered, and the final DoUpdateFromElement() call is made with an up-to-date URL. This protects against the very specific issue of: 1. Creating an image (thus scheduling the microtask) 2. Changing the Document's base URL 3. The microtask that finally runs fetches an image with a stale URL However, I have no idea how this could protect against the original bug filed [4]; see https://chromium-review.googlesource.com/c/chromium/src/+/3311225/comments/5ab9c13f_9c553169 where there is some discussion about how the original bug could have behaved. In any case, since then, crrev.com/c/2103987 has landed which ensures that DoUpdateElement() always resolves relative URLs with the live Document's base URL, so it is impossible for DoUpdateElement() to fetch an outdated URL. This leaves us with the `last_base_element_url_` member that is used as a discriminator to re-invoke the image processing model upon insertion due to this member being stale compared to the Document's base URL. This results in Chromium sometimes fetching the same exact image twice (3-step scenario listed above), which makes this member entirely obsolete dead weight. Furthermore, the HTML Standard does not define general image insertion as a "relevant mutation" [6] that would invoke #update-the-image-data, and it certainly does not capture any base-URL-specific state in the microtask machinery. After some discussion on whatwg/html#7383, we've decided to align Chromium with the specification, which prompts this CL. Testing-wise, this CL is covered by: - image-load-reset-on-new-base.html - Created in response to [4] - svg-image-load-reset-on-new-base.html - Same as above, but the SVG variant - image-base-url.html - Added/moved in crrev.com/c/2103987 - TODO: Add a test that ensures only one request is made - This test is important because while the others ensure that the final image is correct, we've discovered that some browsers are re-fetching the image due to base URLs changing before the image microtask is run, and sometimes that second fetch is using the correct URL, and sometimes it's wrong: whatwg/html#7383 (comment) [1]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=646-647;drc=3297fb6d70a02d8c4af8db2bc9e315053744442b [2]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=724;drc=3297fb6d70a02d8c4af8db2bc9e315053744442b [3]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=297-302;drc=3297fb6d70a02d8c4af8db2bc9e315053744442b [4]: https://crbug.com/897545 [5]: https://crrev.com/c/1296506 [6]: https://html.spec.whatwg.org/multipage/images.html#relevant-mutations Bug: 569760,897545 Change-Id: If6a93c091bfaea2944489e3660adc695d04c3e72 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3311225 Reviewed-by: Fredrik Söderquist <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1099380}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-3311225
branch
from
January 31, 2023 19:09
a61e9b3
to
5628833
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
blink::ImageLoader::last_base_element_url_
stores a snapshot of theDocument's base URL during UpdateFromElement() 1. However, this
snapshot is not actually used to resolve the URL of the image request after the microtask checkpoint in DoUpdateFromElement(); rather,
relative URL resolution always happens with the Document's live base URL
mechanism 2 (more on this later).
Instead, the member exists solely to detect, at image insertion time
3, if the Document's base URL has changed since the last time
UpdateFromElement() was called. The rationale for this used to be to
detect if a document's base URL changed before the image loading
microtask (DoUpdateFromElement()) was run, so that we could protect
against fetching the wrong image with a stale URL; see 4.
This was important because at the time, (before crrev.com/c/2103987)
ImageLoader used to store a snapshot of the resolved image URL in the
microtask state computed in UpdateFromElement(), so it was possible to
wind up in DoUpdateFromElement() with an image request URL that is stale
compared to the live Document's base URL (this was intentional, see
crbug.com/569760 & https://codereview.chromium.org/2105283002). The
solution 5 at the time was to detect, only during insertion, if the
ImageLoader's last known snapshot of the Document base URL is stale, so
that the image processing model can be re-entered, and the final DoUpdateFromElement() call is made with an up-to-date URL. This protects
against the very specific issue of:
However, I have no idea how this could protect against the original bug
filed 4; see
https://chromium-review.googlesource.com/c/chromium/src/+/3311225/comments/5ab9c13f_9c553169
where there is some discussion about how the original bug could have
behaved.
In any case, since then, crrev.com/c/2103987 has landed which ensures
that DoUpdateElement() always resolves relative URLs with the live
Document's base URL, so it is impossible for DoUpdateElement() to fetch
an outdated URL. This leaves us with the
last_base_element_url_
memberthat is used as a discriminator to re-invoke the image processing model
upon insertion due to this member being stale compared to the Document's
base URL. This results in Chromium sometimes fetching the same exact
image twice (3-step scenario listed above), which makes this member
entirely obsolete dead weight.
Furthermore, the HTML Standard does not define general image insertion
as a "relevant mutation" 6 that would invoke #update-the-image-data,
and it certainly does not capture any base-URL-specific state in the
microtask machinery. After some discussion on
whatwg/html#7383, we've decided to align
Chromium with the specification, which prompts this CL.
Testing-wise, this CL is covered by:
final image is correct, we've discovered that some browsers are
re-fetching the image due to base URLs changing before the image
microtask is run, and sometimes that second fetch is using the
correct URL, and sometimes it's wrong: Images should NOT refetch upon insertion even if the document's base URL has changed whatwg/html#7383 (comment)
Bug: 569760,897545
Change-Id: If6a93c091bfaea2944489e3660adc695d04c3e72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3311225
Reviewed-by: Fredrik Söderquist <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1099380}