Should GetProgramInfoLog throw if fGetProgramiv returns a -1?

RESOLVED FIXED in mozilla15

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: jgilbert)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-conformance)

Attachments

(1 attachment, 1 obsolete attachment)

910 bytes, patch
bjacob
: review+
Details | Diff | Splinter Review
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!
Whiteboard: webgl-conformance
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 616347 [details] [diff] [review]
Patch
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attachment #616347 - Flags: review?(bjacob)
(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).
> 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 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.
Attachment #616347 - Flags: review?(bjacob) → review+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #616347 - Flags: feedback?(jgilbert)
Jeff, can you close the loop on this one? Trying to reduce the list of open conformance bugs.
(Assignee)

Comment 8

5 years ago
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.
Attachment #616347 - Flags: feedback?(jgilbert) → feedback-
(Assignee)

Comment 9

5 years ago
Created attachment 628468 [details] [diff] [review]
patch
Attachment #616347 - Attachment is obsolete: true
Attachment #628468 - Flags: review?(bjacob)
Attachment #628468 - Flags: review?(bjacob) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/869482fb63a0
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/869482fb63a0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.