User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 Build ID: 20150130030232 Steps to reproduce: 1. Go to: https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/glsl/misc/shader-varying-packing-restrictions.html Actual results: test: shaders with 15 varyings of mat2 (one past maximum) should fail FAIL [unexpected link status] shaders with 15 varyings of mat2 (one past maximum) should fail Expected results: test: shaders with 15 varyings of mat2 (one past maximum) should fail PASS shaders with 15 varyings of mat2 (one past maximum) should fail Only Firefox Nightly38.0a1 (2015-01-30) with the D3D11 backend fails the test. The following browser all pass: Chrome 42.0.2291.1 canary IE 11. Firefox Nightly38.0a1 (2015-01-30) - webgl.disable-angle = true Firefox Nightly38.0a1 (2015-01-30) - webgl.angle.try-d3d11 = false

Comment on attachment 8576271 [details] [diff] [review] 0001-Enforce-packing-restrictions-for-varyings.patch Review of attachment 8576271 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLShaderValidator.cpp @@ +224,4 @@ > for (auto itrFrag = fragList.begin(); itrFrag != fragList.end(); ++itrFrag) { > static const char prefix[] = "gl_"; > + if (StartsWith(itrFrag->name, prefix)) { > + if (itrFrag->staticUse) { Should this explicitly check for gl_FragCoord, gl_FrontFacing, and gl_PointCoord, or is that handled by itrFrag->staticUse?

(In reply to Dan Glastonbury :djg :kamidphish from comment #2) > Comment on attachment 8576271 [details] [diff] [review] > 0001-Enforce-packing-restrictions-for-varyings.patch > > Review of attachment 8576271 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGLShaderValidator.cpp > @@ +224,4 @@ > > for (auto itrFrag = fragList.begin(); itrFrag != fragList.end(); ++itrFrag) { > > static const char prefix[] = "gl_"; > > + if (StartsWith(itrFrag->name, prefix)) { > > + if (itrFrag->staticUse) { > > Should this explicitly check for gl_FragCoord, gl_FrontFacing, and > gl_PointCoord, or is that handled by itrFrag->staticUse? There's no need to check explicitly, since the compile would fail if it's a variable which shouldn't be there. We can just assume it's a valid var. I believe any gl_ vars that come back as a varying should count towards packing, not just those three per se.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d768961b5362 Actually caught by warnings-as-errors! Retroactive review from Dan.

https://hg.mozilla.org/integration/mozilla-inbound/rev/acf73f2ea7d9 We can't use std::vector::data() yet on android buildbots, so we need to switch to nsTArray here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cfb9e9de85 Another one. I checked that this compiled on my linux box.

Comment on attachment 8576412 [details] [diff] [review] hotfix-packing-restrictions.diff Review of attachment 8576412 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLShaderValidator.cpp @@ +260,5 @@ > *out_log = error; > return false; > } > > + if (staticVertUse && itrFrag->staticUse) { if above also has itrFlag->staticUse, so fold into: if (itrFlag->staticUse) { if (!definedInVertShader) { nsPrintfCString error("Varying `%s` has static-use in the frag" " shader, but is undeclared in the vert" " shader.", itrFrag->name.c_str()); *out_log = error; return false; } if (staticVertUse) { staticUseVaryingList.push_back({itrFrag->type, (int)itrFrag->elementCount()}); } } }

(In reply to Dan Glastonbury :djg :kamidphish from comment #8) > Comment on attachment 8576412 [details] [diff] [review] > hotfix-packing-restrictions.diff > > Review of attachment 8576412 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGLShaderValidator.cpp > @@ +260,5 @@ > > *out_log = error; > > return false; > > } > > > > + if (staticVertUse && itrFrag->staticUse) { > > if above also has itrFlag->staticUse, so fold into: > > if (itrFlag->staticUse) { > if (!definedInVertShader) { > nsPrintfCString error("Varying `%s` has static-use in the frag" > " shader, but is undeclared in the vert" > " shader.", itrFrag->name.c_str()); > *out_log = error; > return false; > } > > if (staticVertUse) { > staticUseVaryingList.push_back({itrFrag->type, > (int)itrFrag->elementCount()}); } > } > } That's fair.

r=kamidphish

https://hg.mozilla.org/mozilla-central/rev/509dbdfe3323 https://hg.mozilla.org/mozilla-central/rev/d768961b5362 https://hg.mozilla.org/mozilla-central/rev/acf73f2ea7d9 https://hg.mozilla.org/mozilla-central/rev/d4cfb9e9de85