1143994 - Fix some -Wunreachable-code and -Wswitch warnings in imagelib

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: imagelib-warnings.patch

> image/decoders/nsGIFDecoder2.cpp:1121:5 [-Wswitch-enum] enumeration value 'gif_image_body' not explicitly handled in switch

The default case would handle the gif_image_body enum value (as a decoding error). Since gif_image_body is the only unhandled enum value, we might as well handle it explicitly and assert the default case is unreachable.

> image/src/FrameAnimator.cpp:405:5 [-Wcovered-switch-default] default label in switch which covers all enumeration values
> image/src/FrameAnimator.cpp:495:13 [-Wswitch-enum] enumeration values 'NOT_SPECIFIED' and 'KEEP' not explicitly handled in switch

We can explicitly handle the NOT_SPECIFIED and KEEP enum values and assert the default case is unreachable.

> image/decoders/nsGIFDecoder2.cpp:1164:2 [-Wunreachable-code-return] 'return' will never be executed
> image/decoders/nsJPEGDecoder.cpp:396:11 [-Wunreachable-code-break] 'break' will never be executed

Comment 1

[Chris Peterson [:cpeterson]]

Comment on attachment 8578415 [details] [diff] [review]
imagelib-warnings.patch

Review ping?

Also, the gif_image_body case was not handled explicitly (and would fall into the "unreachable" default case). It seems like we might need to do something with gif_image_body instead of posting an error.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8578415 [details] [diff] [review]
imagelib-warnings.patch

Review of attachment 8578415 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review!

Thanks for looking into these issues! The patch looks good. r+ with a couple of tweaks below.

::: image/decoders/nsGIFDecoder2.cpp
@@ +1114,5 @@
>  
> +    case gif_image_body:
> +      MOZ_ASSERT_UNREACHABLE("gif_image_body");
> +      PostDataError();
> +      return;

I actually think we should just remove gif_image_body from the enum. It's never referenced anywhere.

@@ +1120,4 @@
>      // We shouldn't ever get here.
>      default:
> +      MOZ_ASSERT_UNREACHABLE("Unexpected mGIFStruct.state");
> +      PostDataError();

This is actually a PostDecoderError() situation; we could only get here due to a bug in the decoder.

Comment 3

[Seth Fowler [:seth] [:s2h]]

(In reply to Chris Peterson [:cpeterson] from comment #1)
> Also, the gif_image_body case was not handled explicitly (and would fall
> into the "unreachable" default case). It seems like we might need to do
> something with gif_image_body instead of posting an error.

Nah, I think it's just unused.

Comment 4

[Chris Peterson [:cpeterson]]

Thanks! I removed gif_image_body and changed PostDataError() to PostDecoderError(NS_ERROR_UNEXPECTED).

https://hg.mozilla.org/integration/mozilla-inbound/rev/75c26288fc67

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75c26288fc67