Bug 206068 – REGRESSION (r254291): [ Catalina wk2 Debug ] Flaky ASSERT on fast/images/animated-image-loop-count.html

• Component: Images
• Status: RESOLVED FIXED

[Truitt Savell]

fast/images/animated-image-loop-count.html

Description:
This test seems to have begun crashing around r254298 only on Catalina wk2 Debug. this is not reproducing locally for m right now

History:
https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fanimated-image-loop-count.html

Crash Log:

No crash log found for com.apple.WebKit.WebContent.Development:3398.

stdout:

stderr:
2   0x61b6f22eb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x61efbfb66 WebCore::ImageSource::~ImageSource()
4   0x61efbfcf5 WebCore::ImageSource::~ImageSource()
5   0x61ef213ea WTF::ThreadSafeRefCounted<WebCore::ImageSource, (WTF::DestructionThread)0>::deref() const::'lambda'()::operator()() const
6   0x61ef213ad WTF::ThreadSafeRefCounted<WebCore::ImageSource, (WTF::DestructionThread)0>::deref() const
7   0x61ef2135f WTF::Ref<WebCore::ImageSource, WTF::DumbPtrTraits<WebCore::ImageSource> >::~Ref()
8   0x61eef23f5 WTF::Ref<WebCore::ImageSource, WTF::DumbPtrTraits<WebCore::ImageSource> >::~Ref()
9   0x61efc99ec WebCore::ImageSource::startAsyncDecodingQueue()::$_1::~$_1()
10  0x61efc1d85 WebCore::ImageSource::startAsyncDecodingQueue()::$_1::~$_1()
11  0x61efcfe31 WTF::Detail::CallableWrapper<WebCore::ImageSource::startAsyncDecodingQueue()::$_1, void>::~CallableWrapper()
12  0x61efcf915 WTF::Detail::CallableWrapper<WebCore::ImageSource::startAsyncDecodingQueue()::$_1, void>::~CallableWrapper()
13  0x61efcf939 WTF::Detail::CallableWrapper<WebCore::ImageSource::startAsyncDecodingQueue()::$_1, void>::~CallableWrapper()
14  0x635b2e03f std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> >::operator()(WTF::Detail::CallableWrapperBase<void>*) const
15  0x635b2dfbf std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::reset(WTF::Detail::CallableWrapperBase<void>*)
16  0x635b2df59 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::~unique_ptr()
17  0x635b2df35 std::__1::unique_ptr<WTF::Detail::CallableWrapperBase<void>, std::__1::default_delete<WTF::Detail::CallableWrapperBase<void> > >::~unique_ptr()
18  0x635b2df15 WTF::Function<void ()>::~Function()
19  0x635b2d215 WTF::Function<void ()>::~Function()
20  0x635c46de3 WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0::~$_0()
21  0x635c46125 WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0::~$_0()
22  0x635c467f0 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void const*)::operator()(void const*) const
23  0x635c46775 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void const*)::__invoke(void const*)
24  0x7fff709a0bed _Block_release
25  0x7fff708f94de _dispatch_client_callout
26  0x7fff708fea9e _dispatch_lane_serial_drain
27  0x7fff708ff422 _dispatch_lane_invoke
28  0x7fff70908aa1 _dispatch_workloop_worker_thread
29  0x7fff70b52763 _pthread_wqthread
30  0x7fff70b525c3 start_wqthread
LEAK: 1 WebPageProxy

[Radar WebKit Bug Importer]

<rdar://problem/58480028>

[Alexey Proskuryakov]

Seems almost certain to be https://trac.webkit.org/r254291

[Chris Lord]

(In reply to Alexey Proskuryakov from comment #2)
> Seems almost certain to be https://trac.webkit.org/r254291

Almost certainly, looking into it.

[Chris Lord]

So I'm pretty certain this is caused by the async decoding queue causing destruction to happen off the creation thread (and then hitting the assert in the destructor) - this worked before because ImageSource was constructed with the main thread set as the destruction thread.

I'm just testing a patch that moves the references back to the creation thread when exiting the async decoding queue task. I don't know if there's a more elegant way of fixing this(?)

[Chris Lord]

Created attachment 387509 [details]
Patch

[Chris Dumez]

Comment on attachment 387509 [details]
Patch

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

I think this would work fine but I have a couple of nits.

> Source/WebCore/platform/graphics/ImageSource.cpp:349
> +    decodingQueue().dispatch([protectedThis = makeRef(*this), protectedDecodingQueue = makeRef(decodingQueue()), protectedFrameRequestQueue = makeRef(frameRequestQueue()), protectedDecoder = makeRef(*m_decoder), sourceURL = sourceURL().string().isolatedCopy()] () mutable {

I do not understand why we're capturing the protectedDecodingQueue here, is does not appear to be needed. WorkQueue::dispatch() already takes care of ref'ing the WorkQueue object internally.

> Source/WebCore/platform/graphics/ImageSource.cpp:385
> +        // Ensure destruction happens on creation thread

Missing period at the end of the comment (per coding style).

> Source/WebCore/platform/graphics/ImageSource.cpp:386
> +        protectedThis->m_runLoop.dispatch([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });

I see no good reason to dispatch the protectedQueue here, I don't think it should even have been captured in the first place.

[Said Abou-Hallawa]

Comment on attachment 387509 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageSource.cpp:386
>> +        protectedThis->m_runLoop.dispatch([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });
> 
> I see no good reason to dispatch the protectedQueue here, I don't think it should even have been captured in the first place.

Why did we even change this function in r254291? The async image decoding is disabled for 2D canvas and I believe it has to be disabled for off-screen canvas. So I think this function should be retuned it its state before r254291. Maybe we can assert at its beginning ASSERT(isMainThread()) since the async decoding is only enabled for HTMLImageElement and CSS background images.

[Chris Lord]

Created attachment 387633 [details]
Patch

[Chris Lord]

Addressing the comments, I've changed both dispatches back to callOnMainThread, added a main-thread assert in the method and corrected the comment's missing full stop.

I didn't change the protectedQueue ref - at some point a reference is required to the queue being used to make sure it's the same one when updating cached frames and that seems a fine way to do it.

This function should now be the same as it was pre r254291 now, but with an added assert and explicitly moving the references back to the main thread when exiting the queue (as the object no longer has the main thread set as the destruction thread).

[Said Abou-Hallawa]

Comment on attachment 387633 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageSource.cpp:389
> +
> +        // Ensure destruction happens on creation thread.
> +        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });

Is this part still needed? I think r248703 ensures the destruction happens on the main thread.

[Chris Lord]

(In reply to Said Abou-Hallawa from comment #10)
> Comment on attachment 387633 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387633&action=review
> 
> > Source/WebCore/platform/graphics/ImageSource.cpp:389
> > +
> > +        // Ensure destruction happens on creation thread.
> > +        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });
> 
> Is this part still needed? I think r248703 ensures the destruction happens
> on the main thread.

This is needed because r254291 essentially rolls back that change so that ImageSource is usable in workers (needed for ImageBitmap).

[Chris Dumez]

Comment on attachment 387633 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageSource.cpp:377
> +            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedDecodingQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), sourceURL = sourceURL.isolatedCopy(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {

Please use WTFMove() here instead of copyRef()...

>>> Source/WebCore/platform/graphics/ImageSource.cpp:389
>>> +        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });
>> 
>> Is this part still needed? I think r248703 ensures the destruction happens on the main thread.
> 
> This is needed because r254291 essentially rolls back that change so that ImageSource is usable in workers (needed for ImageBitmap).

...and then you no longer need this callOnMainThread().

Make sense?

[Chris Lord]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 387633 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387633&action=review
> 
> > Source/WebCore/platform/graphics/ImageSource.cpp:377
> > +            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedDecodingQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), sourceURL = sourceURL.isolatedCopy(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
> 
> Please use WTFMove() here instead of copyRef()...

This call is in a loop that uses those objects, if we used WTFMove here, surely we'd just crash when the next frame is requested? (note, using copyRef here is not something I introduced, it was always like this)

> >>> Source/WebCore/platform/graphics/ImageSource.cpp:389
> >>> +        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });
> >> 
> >> Is this part still needed? I think r248703 ensures the destruction happens on the main thread.
> > 
> > This is needed because r254291 essentially rolls back that change so that ImageSource is usable in workers (needed for ImageBitmap).
> 
> ...and then you no longer need this callOnMainThread().
> 
> Make sense?

I don't think this makes sense, if I've missed something please elaborate.

[Chris Dumez]

Comment on attachment 387633 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/ImageSource.cpp:389
>>>>> +        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue), protectedDecoder = WTFMove(protectedDecoder)] () mutable { });
>>>> 
>>>> Is this part still needed? I think r248703 ensures the destruction happens on the main thread.
>>> 
>>> This is needed because r254291 essentially rolls back that change so that ImageSource is usable in workers (needed for ImageBitmap).
>> 
>> ...and then you no longer need this callOnMainThread().
>> 
>> Make sense?
> 
> I don't think this makes sense, if I've missed something please elaborate.

Now that I expand the diff, I see that you are in a while loop and therefore cannot use WTFMove(above), I thought we were in the same scope as your callOnMainThread() here. Never mind my comment then, I do agree that this new callOnMainThread is needed.

[WebKit Commit Bot]

Comment on attachment 387633 [details]
Patch

Clearing flags on attachment: 387633

Committed r254692: <https://trac.webkit.org/changeset/254692>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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