Closed Bug 968425 Opened 6 years ago Closed 6 years ago

getShaderInfoLog has a null-term at the end of the string we return to JS

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(1 file, 3 obsolete files)

We should just return the string without the null-terminator.
Attached patch fix-info-log-length (obsolete) — Splinter Review
Attachment #8371000 - Flags: review?(bjacob)
Comment on attachment 8371000 [details] [diff] [review]
fix-info-log-length

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3234,3 @@
>                  shader->SetTranslationFailure(info);
>              } else {
>                  shader->SetTranslationFailure(NS_LITERAL_CSTRING("Internal error: failed to get shader info log"));

So now, in the case of a successful compilation with empty infolog, we have len==0 so we're hitting this SetTranslationFailure.

I think that you should keep the existing behavior in the 0-length case:remove the above MOZ_ASSERT, and have this if() be based on lenWithNull.
Attachment #8371000 - Flags: review?(bjacob) → review-
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 8371000 [details] [diff] [review]
> fix-info-log-length
> 
> Review of attachment 8371000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +3234,3 @@
> >                  shader->SetTranslationFailure(info);
> >              } else {
> >                  shader->SetTranslationFailure(NS_LITERAL_CSTRING("Internal error: failed to get shader info log"));
> 
> So now, in the case of a successful compilation with empty infolog, we have
> len==0 so we're hitting this SetTranslationFailure.
> 
> I think that you should keep the existing behavior in the 0-length
> case:remove the above MOZ_ASSERT, and have this if() be based on lenWithNull.

Oh, ok. I wasn't entirely clear on how that worked.
It sounds like lenWithNull is:
* 0 on internal-error.
* 1 on compile-success. (empty-string)
* >1 on compile-fail.

Thanks, will fix.
Attached patch patch (obsolete) — Splinter Review
Attachment #8371000 - Attachment is obsolete: true
Attachment #8371823 - Flags: review?(bjacob)
Attached patch fix-info-log-length (obsolete) — Splinter Review
I hate whitespace so much.
Attachment #8371823 - Attachment is obsolete: true
Attachment #8371823 - Flags: review?(bjacob)
Attachment #8371826 - Flags: review?(bjacob)
Comment on attachment 8371826 [details] [diff] [review]
fix-info-log-length

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3235,1 @@
>                  info.SetLength(len);

I still don't understand why you need _two_ SetLength calls here!

In my understanding, if you make the first SetLength call with 'len', it will take care to allocate room for one more byte for the terminating zero, and you then don't need the second SetLength call. What am I missing?
Attachment #8371826 - Flags: review?(bjacob) → review-
Now with less SetLength
Attachment #8371826 - Attachment is obsolete: true
Attachment #8371872 - Flags: review?(bjacob)
Comment on attachment 8371872 [details] [diff] [review]
fix-info-log-length

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3234,1 @@
>                  ShGetInfoLog(compiler, info.BeginWriting());

Have you checked that BeginWriting() doesn't want to be paired with some kind of EndWriting() ?
Attachment #8371872 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #8)
> Have you checked that BeginWriting() doesn't want to be paired with some
> kind of EndWriting() ?

In the string APIs, BeginWriting means "Give me the pointer I would use to write to the buffer from the beginning".  EndWriting is "give me the pointer to the end of the writable string".  They're basically c++ iterators, but they're just char*'s.
We should take an ANGLE update as well, because we still have bugs in the error log that were fixed in upstream ANGLE... in some cases if it needs to say "vector of size N", for N it'll include a literal \x04 byte instead of the ascii character "4".
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #10)
> We should take an ANGLE update as well, because we still have bugs in the
> error log that were fixed in upstream ANGLE... in some cases if it needs to
> say "vector of size N", for N it'll include a literal \x04 byte instead of
> the ascii character "4".

Alright, I'll put this on my todo.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bbe8c54029e2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.