Bug 176732 – Followup (r221805): Address comments and add more tests

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

[Said Abou-Hallawa]

See Darin's comments in https://bugs.webkit.org/show_bug.cgi?id=176016#c15. The new test ensures that multiple decode() promises can be resolved or rejected simultaneously without any issues.

[Said Abou-Hallawa]

Created attachment 320474 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/34376485>

[Darin Adler]

Comment on attachment 320474 [details]
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?

[Said Abou-Hallawa]

Created attachment 320678 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 320474 [details]
Patch

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

>> Source/WebCore/loader/ImageLoader.cpp:402
>> +        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?

Part of the Promise implementation is written in JS, please see JavaScriptCore/builtins/PromiseOperations.js. When creating a promise, createResolvingFunctions() is called to create wrappers around the user resolve and reject code blocks. The returned resolve() and reject() functions just call enqueueJob() for the user resolve and reject code blocks. So when ImageLoader::decodeError() calls DeferredPromise::reject(), we end up calling rejectPromise() in PromiseOperations.js which will schedule expecting the user reject code in the next JS execution cycle. If the user reject code has a call to image.decode(), it will not be called until ImageLoader::decodeError() finishes its execution. From looking at the source code and from debugging calling image.decode() from the reject block of another image.decode(), I can confirm that the functions of ImageLoader are not re-enterant. This case is tested by fast/images/decode-static-image-reject.html.

[WebKit Commit Bot]

Comment on attachment 320678 [details]
Patch

Clearing flags on attachment: 320678

Committed r221996: <http://trac.webkit.org/changeset/221996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.