Crash in mozilla::image::DecoderFactory::CloneAnimationDecoder

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
critical
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: calixte, Assigned: aosmond)

Tracking

(Blocks 1 bug, {crash, regression})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 unaffected, firefox65 fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

8 months ago
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 [1] to fix bug 1465619.

[1] https://hg.mozilla.org/mozilla-central/rev?node=bd9168e1e48c
Flags: needinfo?(aosmond)
Assignee

Updated

8 months ago
Assignee: nobody → aosmond
Flags: needinfo?(aosmond)
Priority: -- → P3
Assignee

Updated

8 months ago
Flags: needinfo?(aosmond)
Assignee

Comment 1

8 months ago
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?
Assignee

Comment 2

8 months ago
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.
Assignee

Updated

8 months ago
OS: Linux → All
Hardware: Unspecified → All
Assignee

Comment 3

8 months ago
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.
Assignee

Comment 4

7 months ago
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.
Assignee

Updated

7 months ago
Status: NEW → ASSIGNED

Comment 6

7 months ago
Pushed by aosmond@gmail.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

Comment 7

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/56814f076c2b
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.