826613 - Enable FAIL_ON_WARNINGS on MSVC in image/

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 1

[Joe Drew (not getting mail)]

Comment on attachment 697812 [details] [diff] [review]
patch

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

::: image/src/RasterImage.cpp
@@ +368,2 @@
>    mDecodeRequest(this),
> +#pragma warning(pop)

Oh man, this is sort of sucky. Can you file a followup bug to clean this up so we don't need to use this in the initializer list?

::: media/libjpeg/jmorecfg.h
@@ +194,5 @@
>   * by just saying "FAR *" where such a pointer is needed.  In a few places
>   * explicit coding is needed; see uses of the NEED_FAR_POINTERS symbol.
>   */
>  
> +#undef FAR

This part should be submitted to libjpeg-turbo upstream too.

Comment 2

[DRC]

Given that NEED_FAR_POINTERS should never be defined when building libjpeg-turbo (far pointers are only useful with 16-bit x86 code, which we don't support), it's probably a moot point, but I think it would be slightly more correct to do this:

#ifdef NEED_FAR_POINTERS
#ifndef FAR
#define FAR  far
#endif
#else
#undef FAR
#define FAR
#endif

It may be a fruitless exercise, but whenever I patch a general issue in the libjpeg portion of the code, I try to make it so that the patch could be used by libjpeg as well, if so desired.

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #1)
> ::: image/src/RasterImage.cpp
> @@ +368,2 @@
> >    mDecodeRequest(this),
> > +#pragma warning(pop)
> 
> Oh man, this is sort of sucky. Can you file a followup bug to clean this up
> so we don't need to use this in the initializer list?

That became even more uglier to silence gcc/clang :( Filed bug 826959.

> ::: media/libjpeg/jmorecfg.h
> @@ +194,5 @@
> >   * by just saying "FAR *" where such a pointer is needed.  In a few places
> >   * explicit coding is needed; see uses of the NEED_FAR_POINTERS symbol.
> >   */
> >  
> > +#undef FAR
> 
> This part should be submitted to libjpeg-turbo upstream too.

Submitted. https://sourceforge.net/p/libjpeg-turbo/patches/38/

(In reply to DRC from comment #2)
> Given that NEED_FAR_POINTERS should never be defined when building
> libjpeg-turbo (far pointers are only useful with 16-bit x86 code, which we
> don't support), it's probably a moot point, but I think it would be slightly
> more correct to do this:
> 
> #ifdef NEED_FAR_POINTERS
> #ifndef FAR
> #define FAR  far
> #endif
> #else
> #undef FAR
> #define FAR
> #endif
> 
> It may be a fruitless exercise, but whenever I patch a general issue in the
> libjpeg portion of the code, I try to make it so that the patch could be
> used by libjpeg as well, if so desired.

Makes sense. Landed with this change.
https://hg.mozilla.org/integration/mozilla-inbound/rev/80418f0fc73d

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 697812 [details] [diff] [review]
patch

Thanks for fixing this and getting us towards a More Warning-Free World!

One minor gripe / thing to watch out for, though: I think we need to be **very** wary of hacking around build warnings by blindly adding static_casts, especially when there are no bounds-checks nearby to enforce that the cast is valid.  Otherwise, we may be just telling the compiler to shut up when it's actually helpfully warning us about a legitimate bug.  (or a potential future bug)

I'm looking at this chunk in particular:

>diff --git a/content/svg/content/src/SVGPreserveAspectRatio.h b/content/svg/content/src/SVGPreserveAspectRatio.h
>--- a/content/svg/content/src/SVGPreserveAspectRatio.h
>+++ b/content/svg/content/src/SVGPreserveAspectRatio.h
>@@ -33,18 +33,18 @@ static const unsigned short SVG_MEETORSL
> namespace mozilla {
> class SVGAnimatedPreserveAspectRatio;
> 
> class SVGPreserveAspectRatio
> {
>   friend class SVGAnimatedPreserveAspectRatio;
> public:
>   SVGPreserveAspectRatio(uint16_t aAlign, uint16_t aMeetOrSlice, bool aDefer = false)
>-    : mAlign(aAlign)
>-    , mMeetOrSlice(aMeetOrSlice)
>+    : mAlign(static_cast<uint8_t>(aAlign))
>+    , mMeetOrSlice(static_cast<uint8_t>(aMeetOrSlice))

As in bug 825949 comment 10, it's silly to have a function take a variable of one type, and then *immediately* cast it to another type without any bounds-checking whatsoever.  It's just asking to be abused by a caller who accidentally passes in a value that's out-of-range-for-the-secretly-casted-to type, and then ends up triggering mysterious / subtly buggy behavior.

(In this specific case, I believe we do actually bounds-check these values at some level, so I think we're OK -- but we should do the static_cast to uint8_t at that point, not at this arbitrary later point that's far separated from that bounds-check.)

Could you file a followup on that, and CC me?

Comment 5

[Daniel Holbert [:dholbert]]

(I suppose I could file the followup, too -- but I already did that for bug 825949 comment 10, so I figured I'd let you do it this time around. :))

Comment 6

[Daniel Holbert [:dholbert]]

(er, sorry, copypaste fail -- I meant "bug 824247 comment 10" (not 825949) in the previous two comments)

Comment 7

[Masatoshi Kimura [:emk]]

OK, filed bug 826961.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/80418f0fc73d