| Summary: | REGRESSION (r254291): [ Catalina wk2 Debug ] Flaky ASSERT on fast/images/animated-image-loop-count.html | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Truitt Savell <tsavell> | ||||||
| Component: | Images | Assignee: | Chris Lord <clord> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, cdumez, clord, commit-queue, dino, eric.carlson, sabouhallawa, simon.fraser, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=205850 https://bugs.webkit.org/show_bug.cgi?id=200734 https://bugs.webkit.org/show_bug.cgi?id=206326 |
||||||||
| Attachments: |
|
||||||||
|
Description
Truitt Savell
2020-01-10 08:45:32 PST
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. *** |