Bug 42907 – texture functions should gen INVALID_OPERATION if no texture is bound

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Gregg Tavares]

As per the spec section 5.13.8, if no texture is bound all texture related functions should generate INVALID_OPERATION

eg

gl.bindTexture(gl.TEXTURE_2D, null)
gl.texImage2D(gl.TEXTURE_2D, ...)    <-  should generate INVALID_OPERATION

This conformance test has been updated to show the issue

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/null-object-behaviour.html

[Zhenyao Mo]

Created attachment 63796 [details]
patch

[Zhenyao Mo]

forget to mention: tests are synced with khronos.

[Kenneth Russell]

Comment on attachment 63796 [details]
patch

The code changes look OK but there is an issue with the test:

LayoutTests/fast/canvas/webgl/null-object-behaviour.html:44
 +  shouldGenerateGLError(context, context.NO_ERROR, "context.bindTexture(context.TEXTURE_2D, undefined)");
I think it's a bug in the WebKit WebGL bindings that these accept "undefined" as argument. They should raise TypeError. Only null is a valid non-WebGLTexture argument. If this part of the test is removed then thumbs up from me.

[Zhenyao Mo]

(In reply to comment #3)
> (From update of attachment 63796 [details])
> The code changes look OK but there is an issue with the test:
> 
> LayoutTests/fast/canvas/webgl/null-object-behaviour.html:44
>  +  shouldGenerateGLError(context, context.NO_ERROR, "context.bindTexture(context.TEXTURE_2D, undefined)");
> I think it's a bug in the WebKit WebGL bindings that these accept "undefined" as argument. They should raise TypeError. Only null is a valid non-WebGLTexture argument. If this part of the test is removed then thumbs up from me.

I'll fix the test on khronos and in the patch.

[Gregg Tavares]

If undefined is supposed to generate type error then please change the test to check for type error, don't delete the test.

I ran into tons of no fun bugs in my own code because I was passing undefined unintentionally, misspellings of properties, here and there and WebGL was not complaining about it.  I wasn't sure if it was supposed to complain or not so I ended up wrapping the entire API with a wrapper that checks that no args I pass are undefined.

If those are supposed to reject undefined then we should test that they do.

Is this only for parameters that take null or also for parameters that take number or boolean as well?

[Zhenyao Mo]

(In reply to comment #5)
> If undefined is supposed to generate type error then please change the test to check for type error, don't delete the test.

I will use null instead.

> 
> I ran into tons of no fun bugs in my own code because I was passing undefined unintentionally, misspellings of properties, here and there and WebGL was not complaining about it.  I wasn't sure if it was supposed to complain or not so I ended up wrapping the entire API with a wrapper that checks that no args I pass are undefined.

I think it should so I am planning a patch for it.  However, we need to run through the WebKit folks first because it effects more than just WebGL.

> 
> If those are supposed to reject undefined then we should test that they do.

Yes, "more/functions" in khronos conformance tests have a lot of tests for illegal input parameters.

> 
> Is this only for parameters that take null or also for parameters that take number or boolean as well?

The part of the js binding code I am touching will only check for parameters that take null.

[Zhenyao Mo]

(In reply to comment #5)
> If undefined is supposed to generate type error then please change the test to check for type error, don't delete the test.

After another a look at the test, I realized it's a whole section of tests against "undefined" input.

I'll leave them out in the LayoutTests here and add them back with "shouldThrow" once we have the js binding patch in.

For now LayoutTests and khronos conformance tests will not be the same, but I see no way around it.

> 
> I ran into tons of no fun bugs in my own code because I was passing undefined unintentionally, misspellings of properties, here and there and WebGL was not complaining about it.  I wasn't sure if it was supposed to complain or not so I ended up wrapping the entire API with a wrapper that checks that no args I pass are undefined.
> 
> If those are supposed to reject undefined then we should test that they do.
> 
> Is this only for parameters that take null or also for parameters that take number or boolean as well?

[Zhenyao Mo]

after more thinking, I feel we should check in the patch as it is.  The current test has a nice pattern that tests against 0, undefined, null.  We shouldn't break the pattern by removing part of the undefined section (there are already a few undefined in the test).

We should check them in, and later when we fix the js binding, we change all the 0 and undefined tests to shouldThrow.

[Kenneth Russell]

(In reply to comment #8)
> after more thinking, I feel we should check in the patch as it is.  The current test has a nice pattern that tests against 0, undefined, null.  We shouldn't break the pattern by removing part of the undefined section (there are already a few undefined in the test).
> 
> We should check them in, and later when we fix the js binding, we change all the 0 and undefined tests to shouldThrow.

OK. As we've discussed separately, I was wrong that the tests which pass "undefined" should raise TypeError. Those should convert to null. Patch looks fine as is.

[Dimitri Glazkov (Google)]

Comment on attachment 63796 [details]
patch

ok.

[Zhenyao Mo]

Committed r65089: <http://trac.webkit.org/changeset/65089>