Last Comment Bug 825205 - WebGL index validation fails when a large attribute buffer is bound.
: WebGL index validation fails when a large attribute buffer is bound.
Status: VERIFIED FIXED
[games:p1]
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
http://jareiko.github.com/webgl-confo...
Depends on:
Blocks: gecko-games
  Show dependency treegraph
 
Reported: 2012-12-28 05:39 PST by jk
Modified: 2013-11-06 14:13 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
affected
+
verified
+
verified


Attachments
expand compiled code test to cover this case (1.02 KB, patch)
2013-01-05 10:05 PST, Benoit Jacob [:bjacob] (mostly away)
vladimir: review+
Details | Diff | Splinter Review
fix the bug (4.21 KB, patch)
2013-01-05 10:06 PST, Benoit Jacob [:bjacob] (mostly away)
vladimir: review+
Details | Diff | Splinter Review
fix the bug, using std::numeric_limits (4.70 KB, patch)
2013-01-05 14:01 PST, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch for mozilla-release (5.00 KB, patch)
2013-01-17 11:43 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review

Description jk 2012-12-28 05:39:48 PST
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
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2012-12-28 07:59:02 PST
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.
Comment 3 Alex Keybl [:akeybl] 2013-01-04 16:02:12 PST
Not a regression, so no need to track for release. CC'ing Martin to get this on his radar though (w/r/t games).
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2013-01-04 18:32:52 PST
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.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 07:02:52 PST
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.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 07:10:37 PST
Read the testcase, comment 0 and comment 4 more carefully --- oops, seems bad, looking now.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 08:44:36 PST
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 8 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 10:05:46 PST
Created attachment 698310 [details] [diff] [review]
expand compiled code test to cover this case
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 10:06:18 PST
Created attachment 698311 [details] [diff] [review]
fix the bug
Comment 10 Vladimir Vukicevic [:vlad] [:vladv] 2013-01-05 13:22:59 PST
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.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 13:47:41 PST
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 */ }
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 14:01:52 PST
Created attachment 698340 [details] [diff] [review]
fix the bug, using std::numeric_limits

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).
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 14:58:53 PST
My SSH key is being regen'd in bug 827055.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 15:16:52 PST
https://tbpl.mozilla.org/?tree=Try&rev=e2b1ce627d60
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2013-01-05 18:10:55 PST
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
Comment 18 juan becerra [:juanb] 2013-01-07 10:43:58 PST
Nominating for inclusion in Fx18 prior to shipping or as a potential ride-along in a dot release.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-07 11:19:52 PST
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 20 Alex Keybl [:akeybl] 2013-01-07 13:59:43 PST
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.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2013-01-07 14:05:53 PST
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 22 Benoit Jacob [:bjacob] (mostly away) 2013-01-07 14:07:01 PST
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 23 Alex Keybl [:akeybl] 2013-01-07 16:20:58 PST
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.
Comment 24 Vladimir Vukicevic [:vlad] [:vladv] 2013-01-15 10:51:59 PST
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.
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2013-01-17 11:43:15 PST
Created attachment 703441 [details] [diff] [review]
patch for mozilla-release

[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
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2013-01-17 11:43:59 PST
(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.
Comment 27 Virgil Dicu [:virgil] [QA] 2013-01-23 07:46:42 PST
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.
Comment 28 Virgil Dicu [:virgil] [QA] 2013-01-31 23:41:46 PST
This is verified in 19.
Comment 29 Ryan 2013-02-18 21:06:30 PST
Still showing as an unresolved issue on the release notes.
Comment 30 Mihai Morar, (:MihaiMorar) 2013-03-25 05:35:41 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.