Bug 201402 – REGRESSION (r249367): m_decodingPromises grows indefinitely until ImageLoader destruction

• Component: Images
• Status: RESOLVED FIXED

[Daniel Bates]

By code inspection, m_decodingPromises never shrinks, it can only get larger. There are two issues with this:

1. Memory footprint only gets larger.
2. There may be a correctness issues here: we may try to resolve or reject already resolved or rejected decoding promises.

[Radar WebKit Bug Importer]

<rdar://problem/54952907>

[Said Abou-Hallawa]

Created attachment 377964 [details]
Patch

[Daniel Bates]

Comment on attachment 377964 [details]
Patch

Thank you for looking into this so quickly and writing a test! On a tangentially relate note there might be a better solution:

1. Making the reject*() resolve*() code private member functions since they only operate on class instance data.
2. .clear() is ever so slightly less efficient compared to WTFMove()ing the m_decodingPromises into a local since .clear() calls .shrink(0) and that function has quite a few branches and more inlined function calls to restore inline capacity, etc etc. And m_decodingPromises has no inline capacity. In comparison, there is less code emitted (less branches and less inlined function calls) needed to move a Vector (just a swap()) and construct an empty Vector.
3. The only use for pendingDecodePromisesCount() is for testing. Maybe adding a ForTesting() suffix to its name would make this clear to people that they should not use it for non-testing purposes.

[Daniel Bates]

Comment on attachment 377964 [details]
Patch

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

> LayoutTests/fast/images/decode-resolve-reject-no-leak.html:2
> +<script src="../../resources/js-test.js"></script>

Should this be js-test-pre.js?

[Said Abou-Hallawa]

Created attachment 377997 [details]
Patch

[Daniel Bates]

Comment on attachment 377997 [details]
Patch

Thank you for updating the patch and adding non-static member functions. However the optimal solution would not have static member functions because :

1. There are no callers other than this class and this class passes instance data to them.
2. Compiler generates out-of-band function calls for the static and symbols are in the binary, which increase code size.


How will the static be used in the future? Could they be deferred until that future comes?

[Daniel Bates]

(In reply to Daniel Bates from comment #6)
> Comment on attachment 377997 [details]
> Patch
> 
> Thank you for updating the patch and adding non-static member functions.
> However the optimal solution would not have static member functions because :
> 
> 1. There are no callers other than this class and this class passes instance
> data to them.
> 2. Compiler generates out-of-band function calls for the static and symbols
> are in the binary, which increase code size.
> 
> 
> How will the static be used in the future? Could they be deferred until that
> future comes?

Never mind. I see why you added them to handle:

bitmapImage.decode([promises = WTFMove(m_decodingPromises)]()

[Daniel Bates]

Sorry I didn't mean to mislead you about the static functions. It would have been beneficial to have an explanation in the ChangeLog on why the static functions were needed. I like your original patch more now after discovering this fact. The beta solution is:

1. Add a static, inline, non-member function to resolve promises that takes a rvalue reference Vector.
2. Add private member function (marked inline in .cpp; read: not marked inline in header) to resolve promise function that takes rvalue reference Vector and const char* and turns around and calls (1).
3. Add a private member function (marked Inline in .cpp) to **reject*** promises that takes a const char*.
4. A remark in the ChangeLog for (1) that explains that decoding is async and need to take care to move Vector in capture in case more promises become pending between time of capture and time decode completes.

A static, non-member functions to reject promises is not necessary and should be inlined into (3).

[Daniel Bates]

(In reply to Daniel Bates from comment #8)
> Sorry I didn't mean to mislead you about the static functions. It would have
> been beneficial to have an explanation in the ChangeLog on why the static
> functions were needed. I like your original patch more now after discovering
> this fact. The beta solution is:
> 

*best

> 1. Add a static, inline, non-member function to resolve promises that takes
> a rvalue reference Vector.
> 2. Add private member function (marked inline in .cpp; read: not marked
> inline in header) to resolve promise function that takes rvalue reference
> Vector and const char* and turns around and calls (1).

^^^ Not correct, I meant:

Add private member function (marked inline in .cpp; read: not marked inline in header) to resolve promise function that takes const char* and turns around and calls (1).

(No need to take rvalue ref Vector)

[Said Abou-Hallawa]

Created attachment 378089 [details]
Patch

[youenn fablet]

Comment on attachment 378089 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:294
> +    resolvePromises(WTFMove(m_decodingPromises));

It seems simpler to just inline resolvePromises here.
If you want to keep the move promises before iterating over, it can be written as:

ASSERT(!m_decodingPromises.isEmpty());
auto promises = WTFMove(m_decodingPromises);
for (auto& promise : promises)
    promise->....

[Daniel Bates]

Comment on attachment 378089 [details]
Patch

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

Thanks for updating the patch. It doesn't quite match the description in comment 8 and comment 9, but it's really close..

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

It is error prone and inefficient to take promises by value because:

1. Doing so performs a copy of the Vector, which is inefficient.
2. Since the Vector is of RefPtr copying means ref'ing things not actually copying them, which is confusing.

It would be better to pass by rvalue reference.

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

Ditto.

[Daniel Bates]

Comment on attachment 378089 [details]
Patch

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

>> Source/WebCore/loader/ImageLoader.cpp:294
>> +    resolvePromises(WTFMove(m_decodingPromises));
> 
> It seems simpler to just inline resolvePromises here.
> If you want to keep the move promises before iterating over, it can be written as:
> 
> ASSERT(!m_decodingPromises.isEmpty());
> auto promises = WTFMove(m_decodingPromises);
> for (auto& promise : promises)
>     promise->....

This is not possible because of line 451

[youenn fablet]

> 2. Since the Vector is of RefPtr copying means ref'ing things not actually
> copying them, which is confusing.

In that specific case, it is not inefficient since all callers are providing an r-value of the vector and not a const reference.
I agree though it is better to pass by rvalue reference, no caller is calling the functions by const reference.

> It would be better to pass by rvalue reference.

Agreed.

> > ASSERT(!m_decodingPromises.isEmpty());
> > auto promises = WTFMove(m_decodingPromises);
> > for (auto& promise : promises)
> >     promise->....
> 
> This is not possible because of line 451

True!

[Daniel Bates]

r=me too!

Everything I wrote in comment 12 is optional.

[Said Abou-Hallawa]

Created attachment 378217 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378217 [details]
Patch

Clearing flags on attachment: 378217

Committed r249594: <https://trac.webkit.org/changeset/249594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.