1572360 - img.decode() regression on macOS

Status: NEW

(Core :: Graphics: ImageLib, defect, P2)

Description

[Karl Dubost💡 :karlcow]

1. Go to https://eyen.fr/
2. Note how images are missing

The images are visible in Safari.

<div class="carousel-item active">
  <img class="" src="/images/placeholder.png" data-src="/images/en/screenshots/screenshot4.png" data-srcset="/images/en/screenshots/[e-mail] 2x" width="1336" height="948">
</div>

with css

.carousel-item {
	position: relative;
	display: none;
	-webkit-box-align: center;
	-ms-flex-align: center;
	align-items: center;
	width: 100%;
	transition: -webkit-transform 0.6s ease;
	transition: transform 0.6s ease;
	transition: transform 0.6s ease, -webkit-transform 0.6s ease;
	-webkit-backface-visibility: hidden;
	backface-visibility: hidden;
	-webkit-perspective: 1000px;
	perspective: 1000px;
}
.carousel-fade .carousel-item {
	-webkit-transition: opacity .6s ease-in-out;
	transition: opacity .6s ease-in-out;
}
.carousel-item.active, .carousel-item-next, .carousel-item-prev {
	display: block;
}

The console shows the message:

EncodingError: Invalid image request.

Thomas found out that

1. It works for me on Linux, so odds are that it's Retina-related.
2. mozregression gives this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1501794

see https://webcompat.com/issues/36948#issuecomment-518046459 for the script involved in decoding the image.

Comment 1

[Timothy Nikkel (:tnikkel)]

We reject because between asking for the decode() and actually running the microtask to request the decode the src of the img changes from https://eyen.fr/images/en/screenshots/screenshot1.png to https://eyen.fr/images/en/screenshots/[e-mail]. Per spec we are supposed to reject when this happens. But I haven't determined if we are changing the uri at the right time so it's still possible that it's a gecko bug. This explains why you need a retina screen, because you need to img src to change to the hi res version.

Comment 2

[Timothy Nikkel (:tnikkel)]

It looks like the image element is created, then we queue a microtask to resolve the src set and load (this changes the uri of the image), then the script calls decode() on the image, then the microtask runs, then the microtask to actually request the decode runs, and by this time the uri has changed as a result of the srcset.

Comment 3

[Timothy Nikkel (:tnikkel)]

Yeah, this is a Gecko bug. We need to check the request generation when the microtask for the decode runs, not when we enqueue the decode microtask.

Comment 4

[Timothy Nikkel (:tnikkel)]

Now I see why it is designed that way. The image loading algorithm from the spec uses a microtask. But for setting img.src Gecko just does it synchronously without a microtask, for srcset and other things it correctly uses a microtask. So the current behaviour was designed to allow img.src = something, img.decode() to work, but that means that img.decode on imgs with srcset might not work.

If we adjust img.decode to only check for mutations to the img in the microtask (instead of when we queue the microtask) that fixes this bug but regresses tests that do img.decode then set img.src and expect the promise to be rejected.

Comment 5

[Karl Dubost💡 :karlcow]

The Chrome implementation
https://codereview.chromium.org/2769823002

Comment 6

[Timothy Nikkel (:tnikkel)]

We can probably fix this by tracking which calls are sync and which are microtask in our img element implementation, keeping two generation counters in image loading content, and then checking each generation counter at different points in time depending if the call comes from a sync or microtask.

Comment 7

[Timothy Nikkel (:tnikkel)]

I think bug 1076583 would fix this.