Bug 38700 – drawElements/drawArrays should validate input parameters according to GLES2 spec

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

[Kenneth Russell]

Gregg Tavares points out that WebGLRenderingContext::drawElements currently generates an INVALID_OPERATION error if the UNSIGNED_INT type is passed in, because the index validation code catches the invalid enum but always causes INVALID_OPERATION to be generated. Per the OpenGL ES 2.0 spec, INVALID_ENUM should be generated in this case.

[Zhenyao Mo]

Several cases should return INVALID_VALUE instead of INVALID_OPERATION.

[Zhenyao Mo]

Created attachment 58315 [details]
patch

[Kenneth Russell]

Comment on attachment 58315 [details]
patch

Mostly looks good. A few issues.

WebCore/html/canvas/WebGLRenderingContext.cpp:824
 +          if (!validateDrawMode(mode))
From recent emails on the public-webgl mailing list it sounds like they may want to strictly specify the order in which errors are returned if the inputs would generate multiple errors. For example, INVALID_ENUM would be preferred over INVALID_VALUE. Because of this, even though it may mean doing some redundant validation, I think we should remove the isGLES2Compliant() check here and always validate the draw mode.


LayoutTests/fast/canvas/webgl/resources/webgl-test.js:110
 +              testFailed(evalStr + " expected: " + getGLErrorAsString(ctx, glError) + ". Was " + getGLErrorAsString(ctx, err) + ".");
Can you say here "expected GL error:" and ". Generated: " for clarity?


LayoutTests/fast/canvas/webgl/resources/webgl-test.js:112
 +              testPassed(evalStr + " was expected value: " + getGLErrorAsString(ctx, glError) + ".");
Can you say here " generated expected GL error: " for clarity?


LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html:44
 +  shouldGenerateGLError(context, context.INVALID_ENUM, "context.drawElements(0x0009, 3, context.UNSIGNED_INT, 0)"); // GL_POLYGON
You should use UNSIGNED_BYTE here to make sure you're producing the error for the first enum error and not the second.

[Zhenyao Mo]

Created attachment 58421 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 58421 [details]
revised patch: responding to kbr's review

Looks good.

[Dimitri Glazkov (Google)]

Comment on attachment 58421 [details]
revised patch: responding to kbr's review

ok.

[WebKit Commit Bot]

Comment on attachment 58421 [details]
revised patch: responding to kbr's review

Clearing flags on attachment: 58421

Committed r61017: <http://trac.webkit.org/changeset/61017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.