Bug 126775 - [WebGL] Correct uniform input validation for texture sampler uniforms
Summary: [WebGL] Correct uniform input validation for texture sampler uniforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 122610
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-10 13:23 PST by Brent Fulgham
Modified: 2014-01-11 19:57 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.96 KB, patch)
2014-01-10 15:00 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2014-01-11 19:36 PST, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-01-10 13:23:02 PST
I noticed that https://sketchfab.com/show/d7d5a375d4bf4f91a522a1148dbd3f6a does not render properly in the nightly from 1-9-2013.

JavaScript console indicates:

WebGL: INVALID_VALUE: uniform1: invalid texture unit.
Comment 1 Brent Fulgham 2014-01-10 13:24:22 PST
(In reply to comment #0)
> WebGL: INVALID_VALUE: uniform1: invalid texture unit.

Typo: I meant to say:

WebGL: INVALID_VALUE: uniform1i: invalid texture unit.
Comment 2 Radar WebKit Bug Importer 2014-01-10 13:25:41 PST
<rdar://problem/15795320>
Comment 3 Brent Fulgham 2014-01-10 14:05:32 PST
This stopped working in http://trac.webkit.org/changeset/157271.

However, it looks like the new code is doing the right thing, and preventing us from walking past the end of the available texture units.
Comment 4 Brent Fulgham 2014-01-10 15:00:55 PST
Created attachment 220893 [details]
Patch
Comment 5 Brent Fulgham 2014-01-10 15:01:54 PST
This was a bug in the way the Int32Array type was being accessed. The code was attempting to cast the Int32Array* to a GC3DInt*, which resulted in garbage.

Also added a test case to help avoid this in the future.
Comment 6 Dean Jackson 2014-01-10 15:11:46 PST
Comment on attachment 220893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220893&action=review

r+, but please copy into a new test.

> Source/WebCore/ChangeLog:8
> +        Revised webgl/1.0.2/resources/webgl_test_files/conformance/uniforms/uniform-samplers-test.html

We shouldn't do that. We should try to keep the webgl/1.0.2 files in sync with Khronos. Instead, copy the test into fast/canvas/webgl and make the changes there.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4205
> +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (Int32Array* Variation)");

Should we keep this in here? It might be really noisy.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4225
> +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (GC3Dint* Variation)");

Ditto.
Comment 7 Brent Fulgham 2014-01-10 15:21:03 PST
(In reply to comment #6)
> (From update of attachment 220893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220893&action=review
> 
> r+, but please copy into a new test.

OK!

> > Source/WebCore/ChangeLog:8
> > +        Revised webgl/1.0.2/resources/webgl_test_files/conformance/uniforms/uniform-samplers-test.html
> 
> We shouldn't do that. We should try to keep the webgl/1.0.2 files in sync with Khronos. Instead, copy the test into fast/canvas/webgl and make the changes there.

Done.
 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4205
> > +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (Int32Array* Variation)");
> 
> Should we keep this in here? It might be really noisy.

Removed.
 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4225
> > +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (GC3Dint* Variation)");
> 
> Ditto.
Removed.
Comment 8 Brent Fulgham 2014-01-10 15:24:48 PST
Committed r161684: <http://trac.webkit.org/changeset/161684>
Comment 9 Darin Adler 2014-01-11 15:21:37 PST
This patch seems to have added a test with no test results: fast/canvas/webgl/uniform-samplers-test.html

Also, the test seems to be failing on my computer.
Comment 10 Brent Fulgham 2014-01-11 16:11:59 PST
(In reply to comment #9)
> This patch seems to have added a test with no test results: fast/canvas/webgl/uniform-samplers-test.html
> 
> Also, the test seems to be failing on my computer.

Fixing now.
Comment 11 Brent Fulgham 2014-01-11 19:35:21 PST
Reopened to add missing test.
Comment 12 Brent Fulgham 2014-01-11 19:36:15 PST
Created attachment 220951 [details]
Patch
Comment 13 Brent Fulgham 2014-01-11 19:57:57 PST
Committed r161794: <http://trac.webkit.org/changeset/161794>