Bug 98972 – [CSS Shaders] Validate types of built-in vertex attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Max Vujovic]

When an author defines a built-in attribute, WebKit should check its type.

For example, the following should be an error because a_texCoord should be a vec2, not a float:
attribute float a_texCoord;

[Max Vujovic]

Created attachment 168447 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168447 [details]
Patch

Attachment 168447 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14266384

[Peter Beverloo (cr-android ews)]

Comment on attachment 168447 [details]
Patch

Attachment 168447 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14282032

[Max Vujovic]

Created attachment 168464 [details]
Patch

Attempting Chromium EWS build fix.

[Alexandru Chiculita]

Comment on attachment 168447 [details]
Patch

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

Looks good! Just some minor comments.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:104
>      // Validate the author's samplers.
>      for (Vector<ANGLEShaderSymbol>::iterator it = symbols.begin(); it != symbols.end(); ++it) {
>          if (it->isSampler()) {

This was not in your change, but could also be moved to a separate method (maybe in a different patch). Would it make sense to merge the for loops? They are both iterating on the same symbols. Maybe a call like checkSymbol() inside the for loop would be a good way to do it.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:138
> +    BuiltInAttributes builtInAttributes;
> +    builtInAttributes.set("a_position", SH_FLOAT_VEC4);
> +    builtInAttributes.set("a_texCoord", SH_FLOAT_VEC2);
> +    builtInAttributes.set("a_meshCoord", SH_FLOAT_VEC2);
> +    builtInAttributes.set("a_triangleCoord", SH_FLOAT_VEC3);

It looks like builtInAttributes can be a static and init it only once.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:145
> +        if (builtInAttribute != builtInAttributes.end() && builtInAttribute->value != symbol->dataType) {

Do we allow other attribute names? It looks like you could add a test for that too and fix the check here.

[Max Vujovic]

Thanks for the review, Alex!

(In reply to comment #5)
> (From update of attachment 168447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168447&action=review
> 
> Looks good! Just some minor comments.
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:104
> >      // Validate the author's samplers.
> >      for (Vector<ANGLEShaderSymbol>::iterator it = symbols.begin(); it != symbols.end(); ++it) {
> >          if (it->isSampler()) {
> 

That's a good idea! I'll give it a try.

> This was not in your change, but could also be moved to a separate method (maybe in a different patch). Would it make sense to merge the for loops? They are both iterating on the same symbols. Maybe a call like checkSymbol() inside the for loop would be a good way to do it.
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:138
> > +    BuiltInAttributes builtInAttributes;
> > +    builtInAttributes.set("a_position", SH_FLOAT_VEC4);
> > +    builtInAttributes.set("a_texCoord", SH_FLOAT_VEC2);
> > +    builtInAttributes.set("a_meshCoord", SH_FLOAT_VEC2);
> > +    builtInAttributes.set("a_triangleCoord", SH_FLOAT_VEC3);
> 
> It looks like builtInAttributes can be a static and init it only once.

Sure. Then, we can easily have one loop over the symbols, calling different per-symbol validation functions.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:145
> > +        if (builtInAttribute != builtInAttributes.end() && builtInAttribute->value != symbol->dataType) {
> 
> Do we allow other attribute names? It looks like you could add a test for that too and fix the check here.

Right now we do, but we shouldn't. To make this patch a little smaller / easier to review, I'm planning on implementing that in bug 98973.

[Max Vujovic]

Created attachment 169037 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 169037 [details]
Patch

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

Looks good, Max!

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:140
> +            typedef HashMap<String, ShDataType> SymbolNameToType;
> +            static SymbolNameToType* attributeToType = 0;
> +            if (!attributeToType) {
> +                attributeToType = new SymbolNameToType;
> +                attributeToType->set("a_meshCoord", SH_FLOAT_VEC2);
> +                attributeToType->set("a_position", SH_FLOAT_VEC4);
> +                attributeToType->set("a_texCoord", SH_FLOAT_VEC2);
> +                attributeToType->set("a_triangleCoord", SH_FLOAT_VEC3);
> +            }

nit: Extract these lines to a separate static function.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:98
> +    bool validateSymbols(Vector<ANGLEShaderSymbol>& symbols) const;

nit: This could be a static function as it's not using any of the members. Also, I don't really like that we include ANGLEWebKitBridge.h just to make this call work. What about keeping validateSymbols as a static function in the cpp file only?

[Max Vujovic]

Created attachment 169246 [details]
Patch

[Max Vujovic]

Thanks for looking at this again, Alex.

(In reply to comment #8)
> (From update of attachment 169037 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169037&action=review
> 
> Looks good, Max!
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:140
> > +            typedef HashMap<String, ShDataType> SymbolNameToType;
> > +            static SymbolNameToType* attributeToType = 0;
> > +            if (!attributeToType) {
> > +                attributeToType = new SymbolNameToType;
> > +                attributeToType->set("a_meshCoord", SH_FLOAT_VEC2);
> > +                attributeToType->set("a_position", SH_FLOAT_VEC4);
> > +                attributeToType->set("a_texCoord", SH_FLOAT_VEC2);
> > +                attributeToType->set("a_triangleCoord", SH_FLOAT_VEC3);
> > +            }
> 
> nit: Extract these lines to a separate static function.

Good idea. Done.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:98
> > +    bool validateSymbols(Vector<ANGLEShaderSymbol>& symbols) const;
> 
> nit: This could be a static function as it's not using any of the members. Also, I don't really like that we include ANGLEWebKitBridge.h just to make this call work. What about keeping validateSymbols as a static function in the cpp file only?

Makes sense. Done.

On a different topic, I removed the ANGLEShaderSymbol::isArray() method from the previous patch. I realized ANGLE appends the array size in square brackets to the symbol name. So, "vec4 array[1]" and "vec4 vector" have the same size (1), but have different names, "array[1]" and "vector".

In this patch, we don't need to explicitly check that the author incorrectly defined a built-in attribute as an array because attributes can't be arrays in GLSL, so ANGLE won't validate the shader in the first place.

However, when validate built-in uniform types in bug 98974, we might have to use the uniform's "base name" (without the array size). With the base name, we can look up the uniform in a map and identify that it is one of the built-in uniforms, regardless of its array size.

[Max Vujovic]

(In reply to comment #10)
> On a different topic, I removed the ANGLEShaderSymbol::isArray() method from the previous patch. I realized ANGLE appends the array size in square brackets to the symbol name. So, "vec4 array[1]" and "vec4 vector" have the same size (1), but have different names, "array[1]" and "vector".
> 

I misspoke there. ANGLE appends "[0]" to all array names, regardless of size.

Side note- It looks like there's some new code in ANGLEWebKitBridge::getSymbolInfo, which generates separate symbol names for each array component.

Regardless, attributes can't be arrays or structs in GLSL, so we don't have to worry about this here.

[Max Vujovic]

Created attachment 169286 [details]
Patch

Removed an inaccurate comment from the previous patch about array naming in ANGLEWebKitBridge. Now, I don't touch ANGLEWebKitBridge at all.

[Dean Jackson]

Comment on attachment 169286 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:67
> +    for (Vector<ANGLEShaderSymbol>::iterator symbol = symbols.begin(); symbol != symbols.end(); ++symbol) {

In a patch of mine recently, Darin told me that we try to use array indexing rather than iterators for iterating over Vectors.

[Max Vujovic]

Thanks for the review!

(In reply to comment #13)
> (From update of attachment 169286 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169286&action=review
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:67
> > +    for (Vector<ANGLEShaderSymbol>::iterator symbol = symbols.begin(); symbol != symbols.end(); ++symbol) {
> 
> In a patch of mine recently, Darin told me that we try to use array indexing rather than iterators for iterating over Vectors.

Good to know- I'll update the patch.

[Max Vujovic]

Created attachment 169460 [details]
Patch for Landing

Replaced the vector iterator with a size_t index.

Also, added the const qualifier to ANGLEShaderSymbol::isSampler, so that I can do:
const ANGLEShaderSymbol& symbol = symbols[i];
...
if (symbol.isSampler())
...

[WebKit Review Bot]

Comment on attachment 169460 [details]
Patch for Landing

Clearing flags on attachment: 169460

Committed r131809: <http://trac.webkit.org/changeset/131809>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

Hi Max,
it looks like this patch caused some regressions in the conformance tests.
In particular it caused glsl/functions/glsl-function-atan to fail.
In addition many of the conformance/more/functions tests crash, for example bindBufferBadArgs.html.
Could you take a look?
Thanks,
Roger

[Max Vujovic]

(In reply to comment #18)
> Hi Max,
> it looks like this patch caused some regressions in the conformance tests.
> In particular it caused glsl/functions/glsl-function-atan to fail.
> In addition many of the conformance/more/functions tests crash, for example bindBufferBadArgs.html.
> Could you take a look?
> Thanks,
> Roger

Hi Roger,

It seems unlikely that this patch would cause WebGL failures. It doesn't touch WebGL code except making ANGLEShaderSymbol::isSampler a const method. I'll take a look at the failures, though. Do you have a stack trace that suggests it?

Thanks,
Max

[Roger Fong]

The stack trace seems a bit obscure and javascripty,
What I did was the following:
I reverted to the closest revision where the build was working after this patch landed.
I tested the tests and confirmed that they failed.
I then rolled out the changes made in this patch, rebuilt, and the tests all passed/didn't crash.

I'll get back to you with the actual stack trace when I get to the office.

[Max Vujovic]

(In reply to comment #20)
> The stack trace seems a bit obscure and javascripty,
> What I did was the following:
> I reverted to the closest revision where the build was working after this patch landed.
> I tested the tests and confirmed that they failed.
> I then rolled out the changes made in this patch, rebuilt, and the tests all passed/didn't crash.
> 
> I'll get back to you with the actual stack trace when I get to the office.

Ok, thanks.

I just did this:
- Reverted to the revision right before this patch (r131807), built. glsl/functions/glsl-function-atan succeeds.
- Reverted to the revision with this patch (r131809), built. glsl/functions/glsl-function-atan succeeds.
- Built latest WebKit (r132805), ran Safari. glsl/functions/glsl-function-atan fails.

Btw, I'm not getting any crashes with latest WebKit in conformance/more/functions. I'm on OS X 10.8.1 with an ATI Radeon HD 5770.

The glsl/functions/glsl-function-atan failure in latest WebKit misrenders one pixel in a different position on every refresh. Here's my test result:

PASS getError was expected value: NO_ERROR : no errors from draw
PASS getError was expected value: NO_ERROR : no errors from draw
at (15,28): ref=(123,0,0,255)  test=(122,128,0,255) 
FAIL images are different

It still looks like this change is unrelated to the glsl/functions/glsl-function-atan failure. I'll keep narrowing it down.

[Roger Fong]

Oops, I wouldn't worry about it anymore.
I think something I did didn't match my story earlier because I don't see the problem anymore putting the changes back in. 
So you're right, doesn't look like it's from this patch, my b.

[Max Vujovic]

(In reply to comment #22)
> Oops, I wouldn't worry about it anymore.
> I think something I did didn't match my story earlier because I don't see the problem anymore putting the changes back in. 
> So you're right, doesn't look like it's from this patch, my b.

No worries. In case it helps, I got up to r131826 without an atan failure.

So, the atan regression happened somewhere between r131826.. r132805. Only a thousand commits :)