746740 - Should GetProgramInfoLog throw if fGetProgramiv returns a -1?

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

2684     GLint k = -1;
2685     gl->fGetProgramiv(progname, LOCAL_GL_INFO_LOG_LENGTH, &k);
2686     if (k == -1)
2687         return NS_ERROR_FAILURE; // XXX GL error? shouldn't happen!

Comment 1

[Kelsey Gilbert [:jgilbert]]

GL_INFO_LOG_LENGTH should return the (positive) length or 0, unless there is a GL error, in which case the contents of the param are guaranteed to not be changed.

GL_INVALID_ENUM: if pname is not an accepted value. (not relevant here)
GL_INVALID_VALUE: if program is not a value generated by OpenGL.
GL_INVALID_OPERATION: if program does not refer to a program object.

WebGL is almost certainly similar, so we should just propagate the error, which I believe is automatic. I think standard procedure is to return NS_OK and be sure we didn't touch the output.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch

Comment 3

[Cedric Vivier [:cedricv]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Created attachment 616347 [details] [diff] [review]
> Patch

Wouldn't this patch make the function return "undefined" to JavaScript?

This should be null, or preferably empty string imho (I'll open a thread on the WebGL mailing list about this).

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Wouldn't this patch make the function return "undefined" to JavaScript?

It would make the function return emptystring.  There is no way for this function to return undefined.

Comment 5

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

Comment on attachment 616347 [details] [diff] [review]
Patch

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +2683,5 @@
>  
>      GLint k = -1;
>      gl->fGetProgramiv(progname, LOCAL_GL_INFO_LOG_LENGTH, &k);
>      if (k == -1)
> +        return NS_OK; // GL error occurred, so bail before we do anything.

I would keep the ? from the comment: it is mysterious why this would ever happen.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #5)
> Comment on attachment 616347 [details] [diff] [review]
> Patch
> 
> Review of attachment 616347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +2683,5 @@
> >  
> >      GLint k = -1;
> >      gl->fGetProgramiv(progname, LOCAL_GL_INFO_LOG_LENGTH, &k);
> >      if (k == -1)
> > +        return NS_OK; // GL error occurred, so bail before we do anything.
> 
> I would keep the ? from the comment: it is mysterious why this would ever
> happen.

If we shouldn't ever get this, we should at least crash out here on debug.

Comment 7

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

Jeff, can you close the loop on this one? Trying to reduce the list of open conformance bugs.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 616347 [details] [diff] [review]
Patch

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +2683,5 @@
>  
>      GLint k = -1;
>      gl->fGetProgramiv(progname, LOCAL_GL_INFO_LOG_LENGTH, &k);
>      if (k == -1)
> +        return NS_OK; // GL error occurred, so bail before we do anything.

This needs to set retval to null with |retval.SetIsVoid(true)| before returning.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/869482fb63a0

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/869482fb63a0