47 bytes, text/x-phabricator-request
|Details | Review|
This bug was filed from the Socorro interface and is report bp-6852d82b-af65-4e75-85f0-f83750181024. ============================================================= Top 10 frames of crashing thread: 0 libxul.so mozilla::image::DecoderFactory::CloneAnimationDecoder image/DecoderFactory.cpp:237 1 libxul.so mozilla::image::AnimationSurfaceProvider::Reset image/AnimationSurfaceProvider.cpp:104 2 libxul.so mozilla::image::FrameAnimator::ResetAnimation image/ISurfaceProvider.h:229 3 libxul.so mozilla::image::RasterImage::ResetAnimation image/RasterImage.cpp:903 4 libxul.so nsImageLoadingContent::MakePendingRequestCurrent dom/base/nsImageLoadingContent.cpp:1516 5 libxul.so nsImageLoadingContent::LoadImage dom/base/nsImageLoadingContent.cpp:1076 6 libxul.so nsImageLoadingContent::LoadImage dom/base/nsImageLoadingContent.cpp:916 7 libxul.so mozilla::dom::HTMLImageElement::AfterMaybeChangeAttr dom/html/HTMLImageElement.cpp:470 8 libxul.so mozilla::dom::HTMLImageElement::OnAttrSetButNotChanged dom/html/HTMLImageElement.cpp:410 9 libxul.so mozilla::dom::Element::SetAttr dom/base/Element.cpp:2576 ============================================================= There are 11 crashes (from 11 installations) in nightly 65 starting with buildid 20181023100120. In analyzing the backtrace, the regression may have been introduced by patch  to fix bug 1465619.  https://hg.mozilla.org/mozilla-central/rev?node=bd9168e1e48c
Assignee: nobody → aosmond
Priority: -- → P3
One concerning line is: https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/image/AnimationSurfaceProvider.cpp#457 Where if we encountered an error we won't recreate the decoder, but then the Reset call comes in an assumes we have one. This is a real problem, although tricky to solve and should have existed before. Maybe I made it more likely somehow?
Perhaps the best way to solve is if we need to recreate the decoder, I reset the animation state too. That way it won't matter if there was a redecode error.
In fact, upon further consideration, I think there is a decent argument to be made that if we are discarding, we should be continuously resetting the total frame count. I'm not sure how workable this is (e.g. we are at the end of the animation, and we hit a decoding error and bail early, we can't easily set the size to something lower...). But something I should look at.
Redecode errors break the state machine of FrameAnimator, since the decoder and the animation state are now out of sync. Going forward this tight coupling should be eliminated, as the decoder will produce full frames and the animator can just take the current frame without worrying about its relative position. For the moment, we should just not reset an animation if it hit a redecode error (likely due to OOM) just like how we already stop advancing the animation before the reset.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/56814f076c2b Skip recreating the decoder after redecode errors if an animated image is reset. r=tnikkel
You need to log in before you can comment on or make changes to this bug.