[webkit-reviews] review granted: [Bug 176732] Followup (r221805): Address comments and add more tests : [Attachment 320474] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 176732: Followup (r221805): Address comments and add more tests
https://bugs.webkit.org/show_bug.cgi?id=176732

Attachment 320474: Patch

https://bugs.webkit.org/attachment.cgi?id=320474&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 320474
  --> https://bugs.webkit.org/attachment.cgi?id=320474
Patch

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

Thanks. This addresses most of my comments.

> Source/WebCore/loader/ImageLoader.cpp:402
>      for (auto& promise : m_decodingPromises)
> -	   promise->reject(Exception { EncodingError, WTFMove(message) });
> +	   promise->reject(Exception { EncodingError, message });

I asked this question in the previous patch, and I did not see an answer
explaining why this is OK, nor a code change if it’s not OK:

Is there something that guarantees we will not reenter this function while
rejecting the promises? Can rejecting a promise run arbitrary JavaScript code?
What prevents modification of m_decodingPromises while iterating it?

> Source/WebCore/platform/graphics/BitmapImage.cpp:544
> +    for (auto& decodingCallback : *m_decodingCallbacks)
>	   decodingCallback();

Same question as above:

Is there something that guarantees we will not reenter this function while
making the callbacks? Can a callback run arbitrary JavaScript code? What
prevents modification of m_decodingCallbacks while iterating it?

> Source/WebCore/platform/graphics/BitmapImage.cpp:545
> +    m_decodingCallbacks->clear();

I suggest deleting the Vector instead. Like this:

    m_decodingCallbacks = nullptr;

Unless perhaps there is some advantage to not deallocating it that I am
missing?