Last Comment Bug 746740 - Should GetProgramInfoLog throw if fGetProgramiv returns a -1?
: Should GetProgramInfoLog throw if fGetProgramiv returns a -1?
Status: RESOLVED FIXED
webgl-conformance
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-18 14:29 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2012-05-31 05:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (799 bytes, patch)
2012-04-18 16:37 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
jgilbert: feedback-
Details | Diff | Review
patch (910 bytes, patch)
2012-05-30 14:08 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-18 14:29:33 PDT
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 Jeff Gilbert [:jgilbert] 2012-04-18 16:34:22 PDT
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 Jeff Gilbert [:jgilbert] 2012-04-18 16:37:56 PDT
Created attachment 616347 [details] [diff] [review]
Patch
Comment 3 Cedric Vivier [:cedricv] 2012-04-19 05:09:28 PDT
(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 [:bz] (Out June 25-July 6) 2012-04-19 08:02:19 PDT
> 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) 2012-04-24 13:25:50 PDT
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 Jeff Gilbert [:jgilbert] 2012-04-25 01:35:02 PDT
(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) 2012-05-30 05:37:46 PDT
Jeff, can you close the loop on this one? Trying to reduce the list of open conformance bugs.
Comment 8 Jeff Gilbert [:jgilbert] 2012-05-30 13:59:17 PDT
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 Jeff Gilbert [:jgilbert] 2012-05-30 14:08:08 PDT
Created attachment 628468 [details] [diff] [review]
patch
Comment 11 Ed Morley [:emorley] 2012-05-31 05:57:04 PDT
https://hg.mozilla.org/mozilla-central/rev/869482fb63a0

Note You need to log in before you can comment on or make changes to this bug.