812121 - DrawArrays and DrawElements don't allow drawing without using vertex attrib arrays

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Ran into this checking out someone's example which wasn't working in Firefox.

This isn't a common usecase, but there doesn't seem to be anything wrong with it. Non-array vertex attribs are used for all vertexes, effectively becoming uniforms.

When we don't have any active attrib arrays, we return -1 as the 'max length'. However, we don't check for this case, so naturally we throw a buffer-too-small error.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 3

[Kelsey Gilbert [:jgilbert]]

This really really needs a test in the conformance suite.

Comment 4

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

Then it is webgl-test-needed! (comment 3)

And webgl-conformance (comment 0)

Comment 5

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

Comment on attachment 681910 [details] [diff] [review]
patch

Review of attachment 681910 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +1428,5 @@
>          return ErrorInvalidOperation("drawArrays: overflow in first+count");
>  
> +    // A maxAllowedCount of -1 means unlimited. (No attrib arrays)
> +    // If we have any attribs at all, they're all non-array.
> +    if (maxAllowedCount >= 0) {

Just have maxAllowedCount be INT32_MAX in that case so there is no need for special-casing.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Here it is using INT32_MAX instead.

Comment 7

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

Comment on attachment 682355 [details] [diff] [review]
patch

Review of attachment 682355 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextValidate.cpp
@@ +137,5 @@
>      }
> +
> +    if (*maxAllowedCount == -1) {
> +        *maxAllowedCount = INT32_MAX;
> +    }

I'd prefer if you took out the whole -1 thing and replaced it by INT32_MAX in place.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

How about this?

Comment 9

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

Comment on attachment 683086 [details] [diff] [review]
patch

Review of attachment 683086 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, r- because this is really important:

::: content/canvas/src/WebGLContext.h
@@ +1169,5 @@
>      int32_t mGLMaxVertexUniformVectors;
>  
>      // Cache the max number of elements that can be read from bound VBOs
>      // (result of ValidateBuffers).
> +    bool mMinInUseAttribArrayLengthCached;

This new member needs to be initialized by the constructor, right?

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

We didn't seem to initialize the previous value we used for this, so it's probably buried somewhere. I added the vars to the constructor, though.

Comment 11

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

Comment on attachment 683090 [details] [diff] [review]
patch

Review of attachment 683090 [details] [diff] [review]:
-----------------------------------------------------------------

R+ provided that the following issue is not causing new signed-unsigned-comparison warnings:

::: content/canvas/src/WebGLContextValidate.cpp
@@ +68,5 @@
>   * that will be legal to be read from bound VBOs.
>   */
>  
>  bool
> +WebGLContext::ValidateBuffers(uint32_t *maxAllowedCount, const char *info)

Decide: either uint32_t everywhere (so  mMinInUseAttribArrayLengthCached becomes uint32_t, and it's UINT32_MAX) or int32_t everywhere. As we plan to allow uint32 elements, I suppose that uint32 makes sense.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Carrying forward r+.

Comment 13

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86b05603698f

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/86b05603698f

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 683115 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long-standing bug.
User impact if declined: Some basic WebGL tutorials and examples will fail to work with Firefox.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low risk. The code here is fairly simple, and we're working with CheckedInts anyways.
String or UUID changes made by this patch: None.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 683115 [details] [diff] [review]
patch

Fast tracking this low risk fix since this sounds like a bit of a WebGL black eye.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/605f41ba80a3

Comment 18

[Virgil Dicu [:virgil] [QA]]

Verified the issue with testcase from comment 1 on 2012-11-15 Nightly
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121115030705

Verified the fix for Firefox 19.0 beta 3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130123083802

The black rectangle from 2012-11-15 Nightly contains a small green rectangle in Firefox 19.0 beta 3.

Comment 19

[Virgil Dicu [:virgil] [QA]]

Verified fix for Firefox 20.0 beta 1 (20130220104816) on Windows 7 x64
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0