1351186 - Assertion failure "HasError() || mCurrentFrame (Should have an error or a frame)" in [@ mozilla::image::Decoder::CompleteDecode]

Status: ASSIGNED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Assertion failure: HasError() || mCurrentFrame (Should have an error or a frame), at /home/worker/workspace/build/src/image/Decoder.cpp:227

Found with mozilla-central asan debug buildID=20170327212148

==18044==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f756c16017e bp 0x7ffd9ecae250 sp 0x7ffd9ecae160 T0)
==18044==The signal is caused by a WRITE memory access.
==18044==Hint: address points to the zero page.
    #0 0x7f756c16017d in mozilla::image::Decoder::CompleteDecode() /home/worker/workspace/build/src/image/Decoder.cpp:227:5
    #1 0x7f756c152ddc in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /home/worker/workspace/build/src/image/Decoder.cpp:150:3
    #2 0x7f756c15de71 in mozilla::image::DecodedSurfaceProvider::Run() /home/worker/workspace/build/src/image/DecodedSurfaceProvider.cpp:139:34
    #3 0x7f756c19cf0e in mozilla::image::LaunchDecodingTask(mozilla::image::IDecodingTask*, mozilla::image::RasterImage*, unsigned int, bool) /home/worker/workspace/build/src/image/RasterImage.cpp:1193:32
    #4 0x7f756c195f7d in mozilla::image::RasterImage::Decode(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, mozilla::image::PlaybackType) /home/worker/workspace/build/src/image/RasterImage.cpp:1282:10
    #5 0x7f756c1954f1 in mozilla::image::RasterImage::LookupFrame(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, mozilla::image::PlaybackType) /home/worker/workspace/build/src/image/RasterImage.cpp:354:20
    #6 0x7f756c19761d in mozilla::image::RasterImage::GetFrameInternal(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, unsigned int) /home/worker/workspace/build/src/image/RasterImage.cpp:574:5
    #7 0x7f756c1973fa in mozilla::image::RasterImage::GetFrameAtSize(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, unsigned int) /home/worker/workspace/build/src/image/RasterImage.cpp:544:5
    #8 0x7f756f9d12a4 in nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent*, unsigned int, RefPtr<mozilla::gfx::DrawTarget>&) /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:7297:43
    #9 0x7f756f9d2343 in nsLayoutUtils::SurfaceFromElement(mozilla::dom::Element*, unsigned int, RefPtr<mozilla::gfx::DrawTarget>&) /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:7461:10
    #10 0x7f756dd8bab7 in mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/canvas/CanvasRenderingContext2D.cpp:4940:13
    #11 0x7f756d1d6fc7 in mozilla::dom::CanvasRenderingContext2DBinding::drawImage(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:2530:13
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Tyson Smith [:tsmith]]

Attachment: test_data.ico

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fix handling of truncated images with no frame, v1

If the image resource inside the ICO is truncated and doesn't produce a frame, it can claim decoding has completed successfully (since the truncated state is just TerminalState::SUCCESS), but we never reached the point where we create the imgFrame. This later trips up a sanity assert. I think it should be mostly harmless, but I've added a new terminal state (TerminalState::TRUNCATED) which allows us to check if we should produce an error (because we didn't get what we needed) or indicate success (because while we may not have gotten everything, we got enough to proceed).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a15c357bd91f9e95cb8f70c9e903aa9760b482fa

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fix handling of truncated images with no frame, v2

Fix broken reftests.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72cef619fdeaac9088d4f877097c757f12677957

Comment 5

[Timothy Nikkel (:tnikkel)]

Have you looked through all the code using terminal states? I feel like there was code that depended on terminal states being a binary success/failure. ie something like

  if (terminal state success) {
    return;
  }
  // handle failure

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Have you looked through all the code using terminal states? I feel like
> there was code that depended on terminal states being a binary
> success/failure. ie something like
> 
>   if (terminal state success) {
>     return;
>   }
>   // handle failure

Yes. Looking for SUCCESS and FAILURE, I found the following comparisons:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/image/decoders/nsPNGDecoder.cpp#924 -- but the only options in that code path are SUCCESS or FAILURE.

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/image/Decoder.cpp#145 -- updated by this patch.

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/image/decoders/nsICODecoder.cpp#665 -- Decoder::Decode handles the TRUNCATED case and converts it to SUCCESS or FAILURE.

Now that you mention it though, it is probably a good idea to add an assert to nsICODecoder::WriteToContainedDecoder.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Have you looked through all the code using terminal states? I feel like
> there was code that depended on terminal states being a binary
> success/failure. ie something like
> 
>   if (terminal state success) {
>     return;
>   }
>   // handle failure

I should also note that for the other code paths that look at LexerResult, they only care if it is a TerminalState, but not the specific reason for state, such as:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/image/DecodedSurfaceProvider.cpp#144

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This is still reproducible on trunk with or without Stylo enabled. Any updates on the review here?

Comment 9

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Timothy, is this still on your review radar? Not sure how applicable the patch even is at this point.