jk Reporter
	Description • 12 years ago

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

	jk Reporter
	Comment 1 • 12 years ago

Uploaded the test to github pages:

http://jareiko.github.com/webgl-conformance/conformance/buffers/index-validation-large-buffer.html

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 2 • 12 years ago

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.

	Alex Keybl [:akeybl]
	Comment 3 • 12 years ago

Not a regression, so no need to track for release. CC'ing Martin to get this on his radar though (w/r/t games).

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 4 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 5 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 6 • 12 years ago

Read the testcase, comment 0 and comment 4 more carefully --- oops, seems bad, looking now.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 7 • 12 years ago

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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 10 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 11 • 12 years ago

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 */ }

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 12 • 12 years ago

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

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 13 • 12 years ago

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

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 14 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e2b1ce627d60

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 15 • 12 years ago

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

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 16 • 12 years ago

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

	(no longer active)
	Comment 17 • 12 years ago

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

	juan becerra [:juanb]
	Comment 18 • 12 years ago

Nominating for inclusion in Fx18 prior to shipping or as a potential ride-along in a dot release.

	u279076
	Comment 19 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 20 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 21 • 12 years ago

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?

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 22 • 12 years ago

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.

	Alex Keybl [:akeybl]
	Comment 23 • 12 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 24 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 25 • 11 years ago

[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

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 26 • 11 years ago

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

	Virgil Dicu [:virgil] [QA]
	Comment 27 • 11 years ago

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.

	Virgil Dicu [:virgil] [QA]
	Comment 28 • 11 years ago

This is verified in 19.

	Ryan
	Comment 29 • 11 years ago

Still showing as an unresolved issue on the release notes.

	Mihai Morar, (:MihaiMorar)
	Comment 30 • 11 years ago

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!