924188 - Use MOZ_ASSERT where appropriate in WebGL code

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: patch

Comment 1

[Kelsey Gilbert [:jgilbert]]

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

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

Nits. We should really resurrect NS_NOTREACHED for MOZ_*, too.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2155,5 @@
>                  } else if (shader->ShaderType() == LOCAL_GL_FRAGMENT_SHADER) {
>                      shaderTypeName = "fragment";
>                  } else {
>                      // should have been validated earlier
> +                    MOZ_ASSERT(false);

We should really have a MOZ_ASSERT_NOTREACHED or something.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +221,5 @@
>              return true;
>      }
>  
>      NS_NOTREACHED("Invalid WebGL texture format?");
> +    MOZ_ASSERT(false);

This should be redundant.

::: content/canvas/src/WebGLTexelConversions.cpp
@@ +173,1 @@
>                            mDstStride % sizeof(DstType) == 0,

Fix the indents here.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Comment on attachment 814189 [details] [diff] [review]
> patch
> 
> Review of attachment 814189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nits. We should really resurrect NS_NOTREACHED for MOZ_*, too.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +2155,5 @@
> >                  } else if (shader->ShaderType() == LOCAL_GL_FRAGMENT_SHADER) {
> >                      shaderTypeName = "fragment";
> >                  } else {
> >                      // should have been validated earlier
> > +                    MOZ_ASSERT(false);
> 
> We should really have a MOZ_ASSERT_NOTREACHED or something.

There actually is MOZ_ASSUME_UNREACHABLE in mfbt/Assertions.h.

But it warns about how it's intentionally undefined behavior, therefore unsafe and only useful as an optimization in critical code:

 * MOZ_ASSUME_UNREACHABLE([reason]) tells the compiler that it can assume that
 * the macro call cannot be reached during execution.  This lets the compiler
 * generate better-optimized code under some circumstances, at the expense of
 * the program's behavior being undefined if control reaches the
 * MOZ_ASSUME_UNREACHABLE.
 *
 * In Gecko, you probably should not use this macro outside of performance- or
 * size-critical code, because it's unsafe.  If you don't care about code size
 * or performance, you should probably use MOZ_ASSERT or MOZ_CRASH.

So let's stick to MOZ_CRASH here.

> 
> ::: content/canvas/src/WebGLContextUtils.cpp
> @@ +221,5 @@
> >              return true;
> >      }
> >  
> >      NS_NOTREACHED("Invalid WebGL texture format?");
> > +    MOZ_ASSERT(false);
> 
> This should be redundant.

Good catch, we only want the MOZ_ASSERT(false) here for the reason given above.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: updated for landing

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1632bbff03ad

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1632bbff03ad