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: https://github.com/jareiko/WebGL/blob/bb30195dd86dc74619ae852eb6885bb967687a86/conformance-suites/1.0.1/conformance/buffers/index-validation-large-buffer.html 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: https://github.com/KhronosGroup/WebGL/pull/123 Original bug report: https://github.com/CodeArtemis/TriggerRally/issues/7

Uploaded the test to github pages: http://jareiko.github.com/webgl-conformance/conformance/buffers/index-validation-large-buffer.html

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.

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

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.

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).

My SSH key is being regen'd in bug 827055.

https://hg.mozilla.org/integration/mozilla-inbound/rev/27fead003810 https://hg.mozilla.org/integration/mozilla-inbound/rev/a30a8521c168

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

https://hg.mozilla.org/mozilla-central/rev/27fead003810 https://hg.mozilla.org/mozilla-central/rev/a30a8521c168

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 TriggerRally.com we'll want to check other WebGL sites to make sure nothing more popular has regressed (ex. MapsGL).

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.

https://hg.mozilla.org/releases/mozilla-beta/rev/bb246df21370 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.

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.

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

(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 http://www.netmagazine.com/features/20-webgl-sites-will-blow-your-mind and MapsGL from maps.google.com 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: Win7: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 (20130320062118) MacOS 10.8: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0 (20130320062118) If anyone can still reproduce this issue please reopen it, but based on my verification done it's fixed!