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
<rdar://problem/58480028>
Seems almost certain to be https://trac.webkit.org/r254291
(In reply to Alexey Proskuryakov from comment #2) > Seems almost certain to be https://trac.webkit.org/r254291 Almost certainly, looking into it.
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(?)
Created attachment 387509 [details] Patch
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.
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.
Created attachment 387633 [details] Patch
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).
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.
(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).
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?
(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.
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.
Comment on attachment 387633 [details] Patch Clearing flags on attachment: 387633 Committed r254692: <https://trac.webkit.org/changeset/254692>
All reviewed patches have been landed. Closing bug.
*** Bug 206326 has been marked as a duplicate of this bug. ***