Bug 45769 – drawElements with count=0 and offset = 0 should not generate GL error or crash

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

[Zhenyao Mo]

gman@google found that drawElements(count=0, offset=0) will generate GL error or crash in some cases.

For example, on khronos webgl demo google/particles, if we change numParticles: 20 to 0 in demo.js, browser will crash.

Also, khronos webgl conformance test conformance/draw-elements-out-of-bounds.html wil generate an error.

[Zhenyao Mo]

Created attachment 67587 [details]
patch

The test is in sync with khronos.

Also, tested the particles demo with numParticles=0, it no longer crashes.

[Kenneth Russell]

Comment on attachment 67587 [details]
patch

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

Basically looks OK but I think one cleanup is desired.

> WebCore/html/canvas/WebGLRenderingContext.cpp:789
> +            else if (m_boundElementArrayBuffer->elementArrayBuffer()) {
I think it would be cleaner and safer to hoist this check and the other one below to the top of this function and return early; i.e., "if (!m_boundElementArrayBuffer->elementArrayBuffer()) return false;" and then let the precise validation handle it.

[Zhenyao Mo]

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

[Kenneth Russell]

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

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

Sorry for the long delay getting back to this review. I'm marking it r+ since I think the logic is correct and this fix has been held up long enough, but ideally we would add one more test simulating vertex attribute 0 and calling drawElements with count==0. If you decide to add this test now, feel free to add it upon commit, or otherwise please file a bug about adding the test.

> WebCore/html/canvas/WebGLRenderingContext.cpp:799
> +            numElements /= sizeof(unsigned short);

This can cause numElements to go to 0 if the buffer only has one element. However, I don't think that's a problem since it will only cause false to be returned.

> WebCore/html/canvas/WebGLRenderingContext.cpp:830
> +        return true;

This clause seems to be used when simulating vertex attribute 0. We should really add a test for that.

[Zhenyao Mo]

(In reply to comment #4)
> (From update of attachment 67604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67604&action=review
> 
> Sorry for the long delay getting back to this review. I'm marking it r+ since I think the logic is correct and this fix has been held up long enough, but ideally we would add one more test simulating vertex attribute 0 and calling drawElements with count==0. If you decide to add this test now, feel free to add it upon commit, or otherwise please file a bug about adding the test.
> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:799
> > +            numElements /= sizeof(unsigned short);
> 
> This can cause numElements to go to 0 if the buffer only has one element. However, I don't think that's a problem since it will only cause false to be returned.
> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:830
> > +        return true;
> 
> This clause seems to be used when simulating vertex attribute 0. We should really add a test for that.

[Zhenyao Mo]

(In reply to comment #4)
> (From update of attachment 67604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67604&action=review
> 
> Sorry for the long delay getting back to this review. I'm marking it r+ since I think the logic is correct and this fix has been held up long enough, but ideally we would add one more test simulating vertex attribute 0 and calling drawElements with count==0. If you decide to add this test now, feel free to add it upon commit, or otherwise please file a bug about adding the test.

I'll file a bug on this.

> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:799
> > +            numElements /= sizeof(unsigned short);
> 
> This can cause numElements to go to 0 if the buffer only has one element. However, I don't think that's a problem since it will only cause false to be returned.
> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:830
> > +        return true;
> 
> This clause seems to be used when simulating vertex attribute 0. We should really add a test for that.

[Zhenyao Mo]

Committed r68401: <http://trac.webkit.org/changeset/68401>

[Eric Seidel (no email)]

fast/canvas/webgl/draw-elements-out-of-bounds.html is crashing on Snow Leopard.

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r68740%20(18485)/fast/canvas/webgl/draw-elements-out-of-bounds-stderr.txt

Assertion failed: (0 && "InitThread(): Process hasn't been initalised."), function InitThread, file /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release/build/ANGLE/src/compiler/InitializeDll.cpp, line 71.

[Eric Seidel (no email)]

The crashing may have started after I turned off some canvas tests as part of this rollout:
http://trac.webkit.org/changeset/68737