Bug 49350 – bufferData/bufferSubData should not crash with null data input

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

Currently it will crash in command buffer port

[Zhenyao Mo]

Created attachment 73557 [details]
patch

[Zhenyao Mo]

Test synced with khronos

[Kenneth Russell]

Comment on attachment 73557 [details]
patch

Unfortunately the spec is badly written in this area, in particular for bufferData. I don't think that making the call a no-op is the right thing to do. I've just committed an update to the spec requiring that INVALID_VALUE is raised if bufferData is called with a null ArrayBuffer or ArrayBufferView. Could you please update the code and test? Thanks.

[Zhenyao Mo]

Created attachment 73660 [details]
revised patch: responding to spec change

[Darin Adler]

Comment on attachment 73660 [details]
revised patch: responding to spec change

You added four different places that return INVALID_VALUE. But you only have two test cases that expect INVALID_VALUE. That means that at least two of the code paths are untested. Could you fill out the tests?

[Zhenyao Mo]

I don't think we can, because when we pass in null, there is no way to tell it is ArrayBuffer null or ArrayBufferView null.

[Kenneth Russell]

Comment on attachment 73660 [details]
revised patch: responding to spec change

View in context: https://bugs.webkit.org/attachment.cgi?id=73660&action=review

> WebCore/html/canvas/WebGLRenderingContext.cpp:475
> +    if (!data) {
> +        m_context->synthesizeGLError(GraphicsContext3D::INVALID_VALUE);
> +        return;
> +    }

The bufferSubData variants shouldn't generate INVALID_VALUE, only the bufferData variants. The bufferSubData variants can safely return early.

> WebCore/html/canvas/WebGLRenderingContext.cpp:498
> +    if (!data) {
> +        m_context->synthesizeGLError(GraphicsContext3D::INVALID_VALUE);
> +        return;
> +    }

See above.

> LayoutTests/fast/canvas/webgl/buffer-data-array-buffer.html:26
> +glErrorShouldBe(gl, gl.INVALID_OPERATION);

Unfortunately per discussion on the public_webgl list I do not think we can mandate this behavior in the Khronos conformance suite. The overloading behavior in Web IDL is ambiguous in this case (http://dev.w3.org/2006/webapi/WebIDL/Overview.html#call) and Firefox does something different than WebKit. You could require that the GL error is either INVALID_OPERATION or NO_ERROR (and, implicitly, not a crash).

[Zhenyao Mo]

Created attachment 73673 [details]
revised patch

[Kenneth Russell]

Comment on attachment 73673 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73673&action=review

Looks fine; could you please add one comment to the test while landing?

> LayoutTests/fast/canvas/webgl/buffer-data-array-buffer.html:32
> +gl.bufferData(gl.ARRAY_BUFFER, null, gl.STATIC_DRAW);
> +gl.getError();

Could you add a comment like "this should not crash, but the selection of the overload is ambiguous per Web IDL"?

[Zhenyao Mo]

(In reply to comment #9)
> (From update of attachment 73673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73673&action=review
> 
> Looks fine; could you please add one comment to the test while landing?
> 
> > LayoutTests/fast/canvas/webgl/buffer-data-array-buffer.html:32
> > +gl.bufferData(gl.ARRAY_BUFFER, null, gl.STATIC_DRAW);
> > +gl.getError();
> 
> Could you add a comment like "this should not crash, but the selection of the overload is ambiguous per Web IDL"?

Will do.

[Zhenyao Mo]

Committed r71861: <http://trac.webkit.org/changeset/71861>