image.decode() interacts weirdly with the sync cases in "update the image data"

[emilio]

What is the issue with the HTML Standard?

See https://bug1910599.bmoattachments.org/attachment.cgi?id=9416887, where the behavior is different depending on whether the svg is in the list of available images. This happen per spec:

Questions:

• Per spec you should hit the second case (both promises resolve) on this wpt, but Chromium doesn't somehow?

• It seems very confusing that, depending on whether the load is sync, you might end up with a promise resolving successfully for an image that you didn't intend to request a decode for. Do we want to synchronously reject the decode promises whenever "update the image data" runs? I guess that might be a bit over-agressive?

[emilio]

cc @smaug---- @EdgarChen @zcorpan @annevk @domfarolino

[zcorpan]

decode() could run its steps sync if there is no "update the image data" currently running (maybe this should be formalized also).

[progers]

Having a decode resolve for a different image is incredibly confusing, and I support rejecting these cases. Chromium rejects decodes when the current src changes and has a special-case to do this in the synchronous cached-image "update the image data" case (see this comment and this comment).

Rejecting on current request changes is mentioned in the microtask of https://html.spec.whatwg.org/#dom-img-decode (2.2). Does that not apply here because the current request is synchronously modified? Could we add a "reject decodes" step to the https://html.spec.whatwg.org/#updating-the-image-data, maybe after 7.4.6?

Per spec you should hit the second case (both promises resolve) on this wpt, but Chromium doesn't somehow?

The difference in Chromium's behavior is that, in https://bug1910599.bmoattachments.org/attachment.cgi?id=9416887, the currentSrc is unchanged (it is still the svg from the first test), whereas the WPT has a new Image that changes currentSrc.

[emilio]

@progers how do you avoid rejecting for stuff like image-decode-with-quick-attach.html?

[emilio]

Ah, I found the blame: https://chromium-review.googlesource.com/c/chromium/src/+/1141121, so https://html.spec.whatwg.org/#reacting-to-dom-mutations:

The img or source HTML element insertion steps or HTML element removing steps count the mutation as a relevant mutation

Is not true in Chrome :(

That's a bit unfortunate?

[emilio]

That comes from https://issues.chromium.org/40585250 but I don't have access to that. But ok, that's a bit unfortunate... It's a bit weird to have some relevant mutations that don't schedule a task?

Actually, looking at Chromium's code nowadays, I think the current behavior is that insertion of <img> is only a "relevant mutation" iff inserted inside <picture>? That seems more reasonable...

cc @vmpstr @zcorpan @domfarolino

[progers]

@emilio, I'm also mapping this code to the specs for the first time, but agree with your reading of the current code. Can you summarize your current thoughts and open questions about this issue?

[emilio]

Open questions that I can think of:

• Do we want .decode() to queue a microtask unconditionally? If there's no "update the image data" scheduled, it might be less confusing to just go ahead and handle it right there. I don't have a super-strong opinion here.

• How do we want to define the interaction between .decode() and "relevant mutations" / canceled "update the image data" tasks? I think we all agree that the ideal behavior is that .decode() rejects if there's any mutation that actually changes the src you wanted to decode after the decode() call. But right now the spec doesn't track any of that, and it just doesn't work with the whole reordering of "update the image data" tasks due to cancellation... I'm adding more tests in https://phabricator.services.mozilla.com/D218570, but we probably need to change the expectation as part of fixing this issue.

I think the tricky bit that summarizes the problem could be something like:

img.srcset = "foo.png";
img.decode();
img.referrerPolicy = "no-referrer";

I think we ideally want that to resolve, but by the point we run "update the image data", we don't know that .srcset has been set before decode...

I think decode() should somehow be hooked into "update the image data" more deeply, rather than just saying it posts a microtask. I think there's a semi-obvious way to do this: dealing with decode() sync if there's no update the image data, otherwise deal as part of "update the image data". This would involve having some sort of "pending decode operations" list or something that would be dealt with in "update the image data".

That has a caveat tho, which is that it doesn't allow you to differentiate between something like:

img.srcset = "foo.png";
img.decode(); // We definitely want this to resolve

and:

img.srcset = "foo.png";
img.decode(); // I think ideally we want this to reject.
picture.appendChild(img); // Where there's a `<source>` that causes us to load `bar.png`

or:

img.srcset = "foo.png";
img.decode(); // I think ideally we want this to reject.
img.srcset = "bar.png";

So that's a bit disappointing, but I think it'd at least be better than the current spec, and somewhat reasonable behavior to explain. Plus, in general over-resolving feels not as bad as over-rejecting, which is what the spec is doing right now...

I can't come up with a better solution on the spot, but it might exist (well, with enough complexity everything is possible of course...). Thoughts on something like the above or a better proposal?

[emilio]

An (somewhat underwhelming) alternative would also be keeping the .decode() spec as is, and documenting that HTMLImageElement insertion is not a "relevant mutation" unless inside a <picture> element, but that still creates edge cases where we reject but we don't want to do so, like:

img.srcset = "foo.png";
img.decode(); // I think ideally we want this to resolve.
emptyPicture.appendChild(img); // The effective source doesn't change here, but it's a relevant mutation so we reject the decode() promise.

[progers]

Do we have similar issues with the load/error events? For example:

img.src = 'cached_image.png';
img.onload = ... ;
img.onerror = ... ;
img.src = 'broken_image.png';

Chromium only fires an error event. Gecko fires a load event followed by an error event.

Because decoding depends on loading, is there a reason for the decode api to be so disconnected from the load and error events? The spec even has an example that switches out a load for a decode.

I agree with the idea ("decode() should somehow be hooked into "update the image data" more deeply"), including the part about keeping a list of pending decode operations. Can this be done by integrating decode resolving/rejecting next to the load/error events in the spec (basically places current request changes)?