Bug 201243

Created attachment 377499 [details] Patch

<rdar://problem/51351552>

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

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

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.

Created attachment 377598 [details] Patch

Created attachment 377603 [details] Patch

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.

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)); });

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

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

Created attachment 377810 [details] Patch

(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.

Comment on attachment 377810 [details] Patch Clearing flags on attachment: 377810 Committed r249367: <https://trac.webkit.org/changeset/249367>

All reviewed patches have been landed. Closing bug.

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

> > 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.

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.

> 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.

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.

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.

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