551771 - GLBoolean args to WebGL calls always seen as true when called in a loop

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[00003b]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a3pre) Gecko/20100310 Minefield/3.7a3pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a3pre) Gecko/20100310 Minefield/3.7a3pre (.NET CLR 3.5.30729)

when called in a loop, vertexAttribPointer always normalizes the data, colorMask enables all channels.
when called outside loop, both work as expected

Reproducible: Always

Steps to Reproduce:
1. view test case
2. toggle checkboxes
Actual Results:  
with loop not checked, red-black gradient drawn when normalize is checked, purple-red when normalize unchecked

with loop checked, yellow-green gradient with normalize checked or unchecked

Expected Results:  
should always show the red-black gradient with normalize checked, and purple-red with it unchecked
loop checkbox should have no visible effect

Comment 1

[00003b]

Attachment: test case

Comment 2

[00003b]

with loop checked, correct results are shown for short periods of time during various operations: loading pages in other tabs, opening menus/dialogs (particularly for the first time), etc.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This sounds like a failure in the quickstubs for these methods; will look into it, thanks!

Comment 4

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

Short version: the bug is only present when using the TraceMonkey JIT.

Long version:

We don't seem to have custom quickstubs for the VertexAttribPointer function. It's not defined in CustomQS_WebGL.h. Instead, it is defined in dom_quickstubs.cpp which is looking auto-generated. Its code is (note that 'normalized' is arg3):

static JSBool
nsICanvasRenderingContextWebGL_VertexAttribPointer(JSContext *cx, uintN argc, jsval *vp)
{
    XPC_QS_ASSERT_CONTEXT_OK(cx);
    JSObject *obj = JS_THIS_OBJECT(cx, vp);
    if (!obj)
        return JS_FALSE;
    nsICanvasRenderingContextWebGL *self;
    xpc_qsSelfRef selfref;
    if (!xpc_qsUnwrapThis(cx, obj, nsnull, &self, &selfref.ptr, &vp[1], nsnull))
        return JS_FALSE;
    if (argc < 6)
        return xpc_qsThrow(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
    jsval *argv = JS_ARGV(cx, vp);
    uint32 arg0;
    if (!JS_ValueToECMAUint32(cx, argv[0], &arg0))
        return JS_FALSE;
    int32 arg1;
    if (!JS_ValueToECMAInt32(cx, argv[1], &arg1))
        return JS_FALSE;
    uint32 arg2;
    if (!JS_ValueToECMAUint32(cx, argv[2], &arg2))
        return JS_FALSE;
    uint32 arg3_u32;
    if (!JS_ValueToECMAUint32(cx, argv[3], &arg3_u32))
        return JS_FALSE;
    uint8 arg3 = (uint8) arg3_u32;
    uint32 arg4;
    if (!JS_ValueToECMAUint32(cx, argv[4], &arg4))
        return JS_FALSE;
    uint32 arg5;
    if (!JS_ValueToECMAUint32(cx, argv[5], &arg5))
        return JS_FALSE;
    nsresult rv;
    rv = self->VertexAttribPointer(arg0, arg1, arg2, arg3, arg4, arg5);
    if (NS_FAILED(rv))
        return xpc_qsThrowMethodFailed(cx, rv, vp);
    *vp = JSVAL_VOID;
    return JS_TRUE;
}

Coming back to the bug report, since it's only happening when this function is called in a loop, TraceMonkey was a natural suspect, and indeed, if you go to about:config and set

    javascript.options.jit.content

to false, the bug goes away.

So... is it that we found a bug in TraceMonkey, or is it that TraceMonkey exposes a bug in our code??

Comment 5

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

See, only only special thing about GLboolean parameters is they get casted from uint32 to uint8. No idea how this can give a bug that only appears with TraceMonkey!!

Comment 6

[Boris Zbarsky [:bzbarsky]]

When called from trace, the function that's called is nsICanvasRenderingContextWebGL_VertexAttribPointer_tn, not the one in comment 4.

And if you look at that code, it has |jsval _arg3| in the function decl but:

      PRUint8 arg3 = (PRUint8) _arg3;

arg3 is the normalize arg, declared as GLboolean in the idl.

Looks like we have |typedef octet GLboolean| in that same idl.  That cast to PRUint8 comes from the 'octet' case in traceableArgumentConversionTemplates in qsgen.py.  That was added in bug 538258.

The declaration of the argument as jsval comes from the fact that neither traceTypeMap nor traceReturnTypeMap has an 'octet' case, as far as I can tell.

So what happens is that you tell tracemonkey you want a jsval (because the traceable native typemap that qsgen.py generates here uses 'jsval' for 'octet'), and it hands you one.  |false| in JS becomes JSVAL_FALSE, which is a nonzero number.  This is then just straight-cast to an integer, so you get a nonzero integer.  Then you get normalization.

Now the good news is that once jsval becomes a struct in the near future I would think that cast would fail to compile...

In any case, qsgen.py needs to be fixed to either generate some other typemap for 'octet' or to fix the conversion behavior.

Comment 7

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

Thank you for exploring a lot more than I could have!

Fun that you found a different function, I got mine from GDB by setting a breakpoint in the VertexAttribPointer function in WebGLContextGL.cpp:2454.

Anyway we find something very similar, a uint32 to uint8 conversion.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I got mine from GDB by setting a breakpoint in the VertexAttribPointer function
> in WebGLContextGL.cpp:2454

On which iteration of the loop?  The fastnative will be called on the iterations that aren't on trace yet; the traceable native will be called on trace.

> Anyway we find something very similar, a uint32 to uint8 conversion.

Not sure what you mean.

Comment 9

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

> On which iteration of the loop?

oh OK, I hit my breakpoint upon the first loading of the test page, so not from the loop. So I guess it is irrelevant...

> Not sure what you mean.

The fact that you're asking means that I was talking nonsense about stuff that I don't know :-) Sorry!

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So the thing to do would be to keep iterating through in gdb until the normalize parameter becomes incorrect (becomes false when it should be true, in this case) and then look at the stack trace and walk back up to see what went wrong.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Actually, looking at the tn:

nsICanvasRenderingContextWebGL_VertexAttribPointer_tn(JSContext *cx, JSObject *obj, uint32 _arg0, int32 _arg1, uint32 _arg2, jsval _arg3, uint32 _arg4, uint32 _arg5)

...

    PRUint8 arg3 = (PRUint8) _arg3;

Note that _arg3 is declared as a *jsval* in the prototype.  I bet it's being passed in as a jsval, too.  Though then I'd be surprised that normalize could ever be 0, because JSVAL_FALSE is (0<<3)|6=6 and JSVAL_TRUE is (1<<3)|6=13, and it looks like the WebGL code passes that normalize flag straight into gl... so it's getting values "6" and "13", and who knows what's happening there.  (That latter bit is a bug, we should ensure that boolean values are really coerced to GL_TRUE or GL_FALSE.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

vlad, you did read comment 6, right?  ;)

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No, I didn't; managed to scroll right by it.  But now I did via my email!  Ok, so maybe benoit should punt on this and I can talk to jorendroff to figure out who should own it (might be me).

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix, v0

The fix for this is pretty straightforward, though there are actually two fixes here.  Either one would be sufficient to fix this bug, but they're both correct and will prevent any problems with octet in future quickstubs.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/c6009202e1e1

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v1, for Macs

Well, the v0 patch would have worked fine, except for a GLboolean conflict on OSX.  Using the GL types is silly anyway, so this is the same as the previous patch, except it also renames all of the GL type names to WebGL* in the idl and the implementation to avoid conflicts with OSX.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

v1 checked in.