Closed Bug 825205 Opened 12 years ago Closed 12 years ago

WebGL index validation fails when a large attribute buffer is bound.


(Core :: Graphics: CanvasWebGL, defect)

Not set



Tracking Status
firefox18 + affected
firefox19 + verified
firefox20 + verified


(Reporter: jk, Assigned: bjacob)


(Blocks 1 open bug, )


(Whiteboard: [games:p1])


(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.22 (KHTML, like Gecko) Chrome/25.0.1364.5 Safari/537.22

Steps to reproduce:

Opened this document:

Actual results:

Validation fails for the larger of the two attribute buffers.

Tested on:
Firefox 18.0 beta OSX
Firefox nightly 20.0a1 (2012-12-28) OSX

Expected results:

Validation should succeed regardless of attribute buffer size, since the index buffer uses only the first 3 elements.

WebGL conformance suite pull request:

Original bug report:
Confirmed on win32 nightly -- this definitely fails.  This is likely the same problem that's causing a few other similar fails that we've seen.
Component: Untriaged → Canvas: WebGL
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Whiteboard: [games:p1]
Not a regression, so no need to track for release. CC'ing Martin to get this on his radar though (w/r/t games).
Er, it is a regression -- likely as a result of bug 732660 which went in for Firefox 18.  I don't think this happens for 17, I'm actually a little afraid that we're going to ship with some major regressions in WebGL as a result of this.
Yup, I'll look into it ASAP (Monday). To relativize the gravity of this though, this went unnoticed through the channels for about 3 months and passes both WebGL conformance tests and the rather intensive new compiled code test added for this, so the regression has to kick in only in certain unusual cases.
Read the testcase, comment 0 and comment 4 more carefully --- oops, seems bad, looking now.
So here's what's happening. maxAllowed is computed to be 65536, then casted to the index type which is uint16, thus becoming 0.

(gdb) bt
#0  mozilla::WebGLElementArrayCache::Validate<unsigned short> (this=0x7fffdc48be40, maxAllowed=0, 
    firstElement=0, countElements=3)
    at /hack/mozilla-inbound/content/canvas/src/WebGLElementArrayCache.cpp:479
#1  0x00007ffff2c32a76 in mozilla::WebGLElementArrayCache::Validate (this=0x7fffdc48be40, 
    type=5123, maxAllowed=65536, firstElement=0, countElements=3)
    at /hack/mozilla-inbound/content/canvas/src/WebGLElementArrayCache.cpp:531
#2  0x00007ffff2c15865 in mozilla::WebGLBuffer::Validate (this=0x7fffd1f82f20, type=5123, 
    max_allowed=65536, first=0, count=3)
    at /hack/mozilla-inbound/content/canvas/src/WebGLBuffer.h:52
#3  0x00007ffff2c1e9cd in mozilla::WebGLContext::DrawElements (this=0x7fffd0f5c800, mode=4, 
    count=3, type=5123, byteOffset=0)
    at /hack/mozilla-inbound/content/canvas/src/WebGLContextGL.cpp:1523
Attached patch fix the bugSplinter Review
Attachment #698311 - Flags: review?(vladimir)
Comment on attachment 698311 [details] [diff] [review]
fix the bug

Hmm.  T(-1) is maybe not the best, since conceptually indices could be signed.  Isn't there an integer traits thing in C++ you can use?  Or at the very least, can you check that T(-1) doesn't end up negative?

But other than that looks fine, we can finesse the impl bits separately.
Attachment #698311 - Flags: review?(vladimir) → review+
I can do this in a safe and easy way using mozilla::CheckedInt:

   if (CheckedInt<T>(x).isValid()) { /* x is in the range of type T */ }
carried forward r+.

This uses numeric_limits to get the max value for type T. The CheckedInt approach isn't exactly what we want, because we want to do the early exit for x >= T_max, not just for x > T_max.

My SSH key has been disabled as a result of the recent burglary in the Toronto office. If you'd like this to land ASAP, maybe you can land it quicker than me (depending on how long it takes me to re-learn how to re-gen a key pair).
Attachment #698340 - Flags: review+
My SSH key is being regen'd in bug 827055.
Comment on attachment 698340 [details] [diff] [review]
fix the bug, using std::numeric_limits

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 732660
User impact if declined: certain valid, real-world WebGL pages fail to run in Firefox 18
Testing completed (on m-c, etc.): just landed on m-i
Risk to taking this patch (and alternatives if risky): Not risky. Small, simple patch; extensive unit test.
String or UUID changes made by this patch: none
Attachment #698340 - Flags: approval-mozilla-beta?
Attachment #698340 - Flags: approval-mozilla-aurora?
Nominating for inclusion in Fx18 prior to shipping or as a potential ride-along in a dot release.
Flagging this bug for QA verification. In addition to we'll want to check other WebGL sites to make sure nothing more popular has regressed (ex. MapsGL).
Keywords: verifyme
Comment on attachment 698340 [details] [diff] [review]
fix the bug, using std::numeric_limits

Beta merge is complete - please land as soon as possible so that we can go to build with FF19b1.
Attachment #698340 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

My understanding is that it's being automatically uplifted to aurora so we can drop the aurora approval request. Right?
Comment on attachment 698340 [details] [diff] [review]
fix the bug, using std::numeric_limits

[Approval Request Comment]
See above request comment, and today's release-drivers thread. This could be a good candidate for taking on the release tree and shipping as part of a 18.0.1 if there is one.
Attachment #698340 - Flags: approval-mozilla-release?
Comment on attachment 698340 [details] [diff] [review]
fix the bug, using std::numeric_limits

Yep you're right, no need for Aurora given it's now FF20. Leaving the nom for release in preparation for the possibility of an 18.0.1.
Attachment #698340 - Flags: approval-mozilla-aurora?
Even though I bet this isn't showing up much in 18, it's going to start affecting partnerships and demo development that we have in flight for MWC/GDC.  Would be great to get this in to 18.0.1 so that we don't have any issues with those.
[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: some WebGL content fails to render. Vlad says it affects partners i.e. games we partner with
Testing completed (on m-c, etc.): been on m-c and aurora for a while
Risk to taking this patch (and alternatives if risky): no risk at all
Attachment #703441 - Flags: approval-mozilla-release?
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #24)
> Even though I bet this isn't showing up much in 18, it's going to start
> affecting partnerships and demo development

Oh, is it? All I had heard about was the crypt demo which ended up being a different bug.
Confirmed the issue on Firefox 18.0.1 using testcase from comment 1 on Windows 7 x64 and Mac OS X 10.7.5.

  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
  Build ID: 20130116073211

"Test even larger attribute buffer
FAIL context.drawElements(context.TRIANGLES, 3, context.UNSIGNED_SHORT, 0) expected:   NO_ERROR. Was INVALID_OPERATION."

Verified the same testcase for Firefox 19.0 beta 2 and there where no fails on Windows 7 x64 and Mac OS X 10.7.5.

  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
  Build ID: 20130116072953

Also on Firefox 19.0, as per comment 19, I did some exploratory on other WebGL sites from and MapsGL from and no issues were found.
This is verified in 19.
Still showing as an unresolved issue on the release notes.
I confirm WebGL index validation works as expected on FF 20b6:

Verification done on Windows 7 x64 and Mac OS 10.8:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
MacOS 10.8:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0

If anyone can still reproduce this issue please reopen it, but based on my verification done it's fixed!
Keywords: verifyme
Attachment #698340 - Flags: approval-mozilla-release?
Attachment #703441 - Flags: approval-mozilla-release?
Blocks: gecko-games
You need to log in before you can comment on or make changes to this bug.