Bug 201243 – HTMLImageElement::decode() should return a resolved promise for decoding non bitmap images

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Created attachment 377496 [details]
test case

Open the attached test case. In this test case, decode() is called for an SVG data url image.

Result: an alert messages says 'decode() promise was rejected'.
Expected: an alert messages says 'decode() promise was resolved'.

The specs: https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode says "If decoding does not need to be performed for this image (for example because it is a vector graphic), resolve promise with undefined."

[Said Abou-Hallawa]

Created attachment 377499 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/51351552>

[EWS Watchlist]

Comment on attachment 377499 [details]
Patch

Attachment 377499 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12978447

New failing tests:
fast/images/decode-non-bitmap-image-resolve.html

[EWS Watchlist]

Created attachment 377534 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[youenn fablet]

Comment on attachment 377499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377499&action=review

> Source/WebCore/loader/ImageLoader.cpp:388
>      m_decodingPromises.append(WTFMove(promise));

It seems slightly inefficient to append the promise to m_decodingPromises and then call rejectDecodePromises.
We could try to append the promise only in case there is no error and m_imageComplete is false.

> Source/WebCore/loader/ImageLoader.cpp:408
> +    for (auto& promise : m_decodingPromises)

I usually prefer to move the vector in a local variable and then iterate on the local vector, just in case calling some methods might change m_decodingPromises while iterating over it.
I do not think this can happen here but that might not hurt to do that since we changing the code. Ditto for rejectDecodePromises.

> Source/WebCore/loader/ImageLoader.cpp:442
> +    bitmapImage.decode([this]() mutable {

How are we sure that 'this' will be valid when bitmapImage will call the decode callback?
The original code seems safer/clearer to me, and is probably functionally equivalent.

[Said Abou-Hallawa]

Created attachment 377598 [details]
Patch

[Said Abou-Hallawa]

Created attachment 377603 [details]
Patch

[youenn fablet]

Comment on attachment 377603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377603&action=review

> Source/WebCore/loader/ImageLoader.cpp:388
>      m_decodingPromises.append(WTFMove(promise));

I would move this line just before "if (m_imageComplete)"

> Source/WebCore/loader/ImageLoader.cpp:391
> +        rejectDecodePromises(WTFMove(m_decodingPromises), "Inactive document.");

I would write promise->reject(Exception { EncodingError, "Inactive document."_s });

> Source/WebCore/loader/ImageLoader.cpp:397
> +        rejectDecodePromises(WTFMove(m_decodingPromises), "Missing source URL.");

I would write promise->reject(Exception { EncodingError, "Missing source URL."_s });

> Source/WebCore/loader/ImageLoader.cpp:405
> +void ImageLoader::resolveDecodePromises(DecodingPromises&& promises)

Can be a free function:
static inline resolveDecodePromises(DecodingPromises&& promises);

> Source/WebCore/loader/ImageLoader.cpp:412
> +void ImageLoader::rejectDecodePromises(DecodingPromises&& promises, const char* message)

Can be a free function:
static inline rejectDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises);

> Source/WebCore/loader/ImageLoader.h:97
> +    using DecodingPromises = Vector<RefPtr<DeferredPromise>, 1>;

Do we really need 1?
Can we just use a regular Vector?

> Source/WebCore/loader/ImageLoader.h:100
> +    static void rejectDecodePromises(DecodingPromises&&, const char* message);

Please make these free functions.

[Said Abou-Hallawa]

Comment on attachment 377499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377499&action=review

>> Source/WebCore/loader/ImageLoader.cpp:388
>>      m_decodingPromises.append(WTFMove(promise));
> 
> It seems slightly inefficient to append the promise to m_decodingPromises and then call rejectDecodePromises.
> We could try to append the promise only in case there is no error and m_imageComplete is false.

I changed it the way you described then I changed my mind. Suppose you called this method and the next two conditions were evaluated to false so the promise was appended to m_decodingPromises. But no actual decoding happened because m_imageComplete is false. Then you called this function for a different promise. And then one of the next two conditions was evaluated to true because a DOM change happened between the two calls. Now we are going to reject this promise but we are going to leave the first one in m_decodingPromises.

>> Source/WebCore/loader/ImageLoader.cpp:408
>> +    for (auto& promise : m_decodingPromises)
> 
> I usually prefer to move the vector in a local variable and then iterate on the local vector, just in case calling some methods might change m_decodingPromises while iterating over it.
> I do not think this can happen here but that might not hurt to do that since we changing the code. Ditto for rejectDecodePromises.

I changed resolveDecodePromises() and rejectDecodePromises() to be static functions and I made them take rvalue reference of DecodingPromises. The caller will pass WTFMove(m_decodingPromises) so no need to copy m_decodingPromises before iterating through the promises. And no need to keep the pending promises in m_decodingPromises till all of the promises are resolved or rejected.

>> Source/WebCore/loader/ImageLoader.cpp:442
>> +    bitmapImage.decode([this]() mutable {
> 
> How are we sure that 'this' will be valid when bitmapImage will call the decode callback?
> The original code seems safer/clearer to me, and is probably functionally equivalent.

You are right. I changed this to call the status function resolveDecodePromises:

    bitmapImage.decode([promises = WTFMove(m_decodingPromises)]() mutable {
        resolveDecodePromises(WTFMove(promises));
    });

[EWS Watchlist]

Comment on attachment 377603 [details]
Patch

Attachment 377603 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12981070

New failing tests:
fast/images/decode-non-bitmap-image-resolve.html

[EWS Watchlist]

Created attachment 377613 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Said Abou-Hallawa]

Created attachment 377810 [details]
Patch

[Said Abou-Hallawa]

(In reply to Build Bot from comment #10)
> Comment on attachment 377603 [details]
> Patch
> 
> Attachment 377603 [details] did not pass win-ews (win):
> Output: https://webkit-queues.webkit.org/results/12981070
> 
> New failing tests:
> fast/images/decode-non-bitmap-image-resolve.html

The test was opening an ESP image. The ESP images are supported only on macOS. This failure was fixed by removing opening the ESP image from the test.

[WebKit Commit Bot]

Comment on attachment 377810 [details]
Patch

Clearing flags on attachment: 377810

Committed r249367: <https://trac.webkit.org/changeset/249367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

*** Bug 188347 has been marked as a duplicate of this bug. ***

[youenn fablet]

> > It seems slightly inefficient to append the promise to m_decodingPromises and then call rejectDecodePromises.
> > We could try to append the promise only in case there is no error and m_imageComplete is false.
> 
> I changed it the way you described then I changed my mind. Suppose you
> called this method and the next two conditions were evaluated to false so
> the promise was appended to m_decodingPromises. But no actual decoding
> happened because m_imageComplete is false. Then you called this function for
> a different promise. And then one of the next two conditions was evaluated
> to true because a DOM change happened between the two calls. Now we are
> going to reject this promise but we are going to leave the first one in
> m_decodingPromises.

I am not sure to get it. Let's take your example:
1. decode is called, we create a new promise P1 and add it to m_decodingPromises.
2. As part of JavaScript execution, the src attribute is now empty.
3. JavaScript execution continues and decode is called again. We create a new promise P2. We reject P1 and then P2 because the src attribute is empty.
That is, I believe, the current code behavior.

It seems that P1 should be rejected as part of step 2.
For instance, step 3 might never happen, yet P1 should get rejected.

[Daniel Bates]

Comment on attachment 377810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377810&action=review

This patch has a good bug fix in it. It also includes another bug fix (the refactoring) plus it has started a discussion about the design of ImageLoader: whether to restructure the code to conditonally append Promises to a Vector, which in itself may be an alternative fix for this bug. I did not expect to see a refactoring in the patch and I think the design discussion can be moved to another bug because:

1. The refactoring is not necessary to fix this bug as far as I can tell or a reason why the refactoring was needed is not given in the ChangeLog.
2. Because of (1) it makes it harder to see the fix for the originally reported bug.
3. Because of (1) it accidentally introduced a leak (up to lifetime of ImageLoader). See bug #201402.

The design discussion point is really good and I think it sounds like it would have eliminated the possibility of this bug from occurring if we had it from the getgo, but I haven’t thought about it other than a quick glance at the back-and-forth comments. In my opinion, that discussion seems tangential to fixing this bug and could be done in a follow up or could have been done in a bug fix landed before this bug (in which case this bug could just have served for landing a layout test or it could have been closed <— I personally, like doing the former for historical documentation purposes).

> Source/WebCore/loader/ImageLoader.cpp:290
> +static inline void resolveDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises)
> +{
> +    ASSERT(!promises.isEmpty());
> +    for (auto& promise : promises)
> +        promise->resolve();
> +}
> +
> +static inline void rejectDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises, const char* message)
> +{
> +    ASSERT(!promises.isEmpty());
> +    for (auto& promise : promises)
> +        promise->reject(Exception { EncodingError, message });
> +}

Going to switch gears here and talk about this refactoring as if it was being done in a separate bug and patch. So, read everything that follows in the context of that. I did not expect to see this refactoring because:

1. The callers of these functions only pass data controlled by the ImageLoader class. Another way to put this, in my opinion, static, non-member functions provide a benefit when they are used on data that comes from ***2 or more different*** sources: hardcoded, some other class’s instance data, global data, this class’s instance data. But we are only calling these function with one source of data: this class’s instance data. Because of this, I did not expect to see this refactoring or I would have expected an explanation in the ChangeLog what is the benefit of this refactoring and what is the disadvantages of the current design.

2. These functions do ***not*** take ownership of promises. You ***need*** to WTFMove() promises into a local for them to take ownership of it. As a result, we have a leak and maybe other behavioral issues.

[youenn fablet]

> 2. These functions do ***not*** take ownership of promises. You ***need***
> to WTFMove() promises into a local for them to take ownership of it. As a
> result, we have a leak and maybe other behavioral issues.

Oh right, we should fix this before thinking about further improvements.
I do not think there will be any behavioural issue (once a promise is rejected/resolved, it stays in that state) but this will not allow us to clear memory as soon as we could.

[Darin Adler]

Comment on attachment 377810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377810&action=review

> Source/WebCore/loader/ImageLoader.cpp:278
> +static inline void resolveDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises)

This function does not clear out this vector. But callers seem to think that it does.

> Source/WebCore/loader/ImageLoader.cpp:285
> +static inline void rejectDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises, const char* message)

Ditto.

[Said Abou-Hallawa]

Comment on attachment 377810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377810&action=review

>> Source/WebCore/loader/ImageLoader.cpp:278
>> +static inline void resolveDecodePromises(Vector<RefPtr<DeferredPromise>>&& promises)
> 
> This function does not clear out this vector. But callers seem to think that it does.

This regression was addressed in https://bugs.webkit.org/show_bug.cgi?id=201402 and it was fixed in r249594. I am sorry that I did not link the two bugs before.

[Said Abou-Hallawa]

*** Bug 198527 has been marked as a duplicate of this bug. ***