Preserve lazy loading behavior when an image is side-loaded, take two [chromium/src : main]
Stefan Zager (Gerrit)
Stefan Zager added 4 comments![]( "Open in Gerrit")
if (new_image && new_image->IsLazyloadPossiblyDeferred()) {Stefan ZagerCan you help me understand this code? When is `IsLazyloadPossiblyDeferred()` ever true here? I ask because right above, we manually set:
```
image_request_behavior = FetchParameters::ImageRequestBehavior::kNone;
```... and pass it into `loader.Load(...)`. `StyleImageLoader::Load()` immediately delegates to `CSSImageValue::CacheImage()` which just passes the `kNone` image_request_behavior` into `CSSImageValue::PrepareFetch()`, which I guess *never* [sets the image to be lazy loaded](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_image_value.cc;l=78-81;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a)—at least I cannot follow a path where this would ever result in a lazy loaded CSS image (I've gotta be missing something, right?).
Nevertheless, I can't see how we'd enter this block. But assuming that's possible, what are we trying to do here? It seems like we're trying to say: if synchronously after we tried to fetch this lazy loaded image, it's `IsLoaded()` is true, then that means it must've been already fetched by some other means, and we just need to finish the loading from our client's perspective (i.e., call `LoadDeferredImage()`). Is that right?
Indeed, it seems that lazy load behavior for CSS images was disabled entirely by:
https://chromium-review.googlesource.com/c/chromium/src/+/3316090
I've removed my diff from this section, added a CHECK to verify it, and added a TODO to clean up this dead code.
To<StyleFetchedImage>(new_image)->LoadDeferredImage(Stefan ZagerHow do we know that `new_image` is a `StyleFetchedImage` specifically? Sorry, I'm just really not familiar with `element_style_resources` so this might be a dumb question.
Because `property == CSSPropertyID::kBackgroundImage` in the `case` statement above, which means that this condition will be `true`:
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Here is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
delay_until_image_notify_finished_ = nullptr;Stefan ZagerI wonder if we should just prevent the creation of this object in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=771-772;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a specifically for lazyload-deferred images, since I don't think they should ever block the load event, right? In fact, today lazy loaded images don't block the load event when they eventually load, so why is this actually necessary? Is the edge case for lazy loaded images that were loaded out of band before the document load event is fired—then `ImageChanged()` is called because the original `<img>` and the side-loaded one share the same underlying resource / observer chain, and then we accidentally end up creating a `delay_until_image_notify_finished_` object for these images?
Yes, I assume that's how `delay_until_image_notify_finished_` gets vivified. I don't know this code well enough to be confident that modifying `ImageLoader::ImageChanged` is the right thing to do. so I opted instead for a more localized.
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
delay_until_image_notify_finished_ = nullptr;Stefan ZagerI wonder if we should just prevent the creation of this object in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=771-772;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a specifically for lazyload-deferred images, since I don't think they should ever block the load event, right? In fact, today lazy loaded images don't block the load event when they eventually load, so why is this actually necessary? Is the edge case for lazy loaded images that were loaded out of band before the document load event is fired—then `ImageChanged()` is called because the original `<img>` and the side-loaded one share the same underlying resource / observer chain, and then we accidentally end up creating a `delay_until_image_notify_finished_` object for these images?
Yes, I assume that's how `delay_until_image_notify_finished_` gets vivified. I don't know this code well enough to be confident that modifying `ImageLoader::ImageChanged` is the right thing to do. so I opted instead for a more localized.
I went ahead and made the change you suggested, to not populate `delay_until_image_notify_finished_` for a lazy-loaded image.
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 2 comments![]( "Open in Gerrit")
if (new_image && new_image->IsLazyloadPossiblyDeferred()) {Stefan ZagerCan you help me understand this code? When is `IsLazyloadPossiblyDeferred()` ever true here? I ask because right above, we manually set:
```
image_request_behavior = FetchParameters::ImageRequestBehavior::kNone;
```... and pass it into `loader.Load(...)`. `StyleImageLoader::Load()` immediately delegates to `CSSImageValue::CacheImage()` which just passes the `kNone` image_request_behavior` into `CSSImageValue::PrepareFetch()`, which I guess *never* [sets the image to be lazy loaded](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_image_value.cc;l=78-81;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a)—at least I cannot follow a path where this would ever result in a lazy loaded CSS image (I've gotta be missing something, right?).
Nevertheless, I can't see how we'd enter this block. But assuming that's possible, what are we trying to do here? It seems like we're trying to say: if synchronously after we tried to fetch this lazy loaded image, it's `IsLoaded()` is true, then that means it must've been already fetched by some other means, and we just need to finish the loading from our client's perspective (i.e., call `LoadDeferredImage()`). Is that right?
Indeed, it seems that lazy load behavior for CSS images was disabled entirely by:
https://chromium-review.googlesource.com/c/chromium/src/+/3316090
I've removed my diff from this section, added a CHECK to verify it, and added a TODO to clean up this dead code.
Great, thanks a lot!
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Here is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Prior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Related details
- Mason Freed
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Dominic FarolinoHere is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Prior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Sorry, I explained that incorrectly. This code path *does* fire the load event immediately, both before and after this CL.
Because `IsLoaded()` is true, `NotifyFinished()` will be called synchronously from `DoUpdateElement()` in this call stack, via the call to `AddObserver()` ([stack](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=619)). Prior to this patch, `lazy_image_load_state_` would still be `kDeferred` at that time, but `ImageNotifyFinished()` didn't have an early return clause, so we would proceed through the rest of that method and fire the `load` event. With this patch, `lazy_image_load_state_` will be `kFullImage`, so we skip the early-return clause and fire the `load` event just as before.
The early-return clause in `ImageNotifyFinished()` handles the case where it's called while `lazy_image_load_state_` is still `kDeferred`, which is the case for a `loading="lazy"` image that is being monitored for viewport intersection. In that case, we now early-return. When the `LazyLoadImageObserver` signals that the image should be loaded, it updates `lazy_load_image_state_` [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=1015), which ensures that a subsequent call to `ImageNotifyFinished()` will not early-return.
Related details
- Dominic Farolino
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Dominic FarolinoHere is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Stefan ZagerPrior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Sorry, I explained that incorrectly. This code path *does* fire the load event immediately, both before and after this CL.
Because `IsLoaded()` is true, `NotifyFinished()` will be called synchronously from `DoUpdateElement()` in this call stack, via the call to `AddObserver()` ([stack](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=619)). Prior to this patch, `lazy_image_load_state_` would still be `kDeferred` at that time, but `ImageNotifyFinished()` didn't have an early return clause, so we would proceed through the rest of that method and fire the `load` event. With this patch, `lazy_image_load_state_` will be `kFullImage`, so we skip the early-return clause and fire the `load` event just as before.
The early-return clause in `ImageNotifyFinished()` handles the case where it's called while `lazy_image_load_state_` is still `kDeferred`, which is the case for a `loading="lazy"` image that is being monitored for viewport intersection. In that case, we now early-return. When the `LazyLoadImageObserver` signals that the image should be loaded, it updates `lazy_load_image_state_` [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=1015), which ensures that a subsequent call to `ImageNotifyFinished()` will not early-return.
Here's the [call stack](https://screenshot.googleplex.com/6hzv5shQREaNW8f) for `ImageNotifyFinished()` being called when the image resource finishes loading, without going through `DoUpdateElement()`.