[CSS Shaders] Reject vertex shaders with custom attributes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Max Vujovic]

An author shouldn't be able to define custom vertex attributes because there is no way to set them from CSS.

For example, defining built-in attributes is ok:
attribute vec4 a_position;
attribute vec2 a_texCoord;

But defining custom attributes should be an error:
attribute float my_attribute;

Also, we have to be careful with attached vs. detached meshes. "a_triangleCoord" is available only in detached meshes.

[Max Vujovic]

Created attachment 170183 [details]
Patch

[Max Vujovic]

Comment on attachment 170183 [details]
Patch

In this patch, I still need to figure out how to get the meshType in CustomFilterValidatedProgram, since a_triangleCoord is a built-in attribute in detached meshes but not in attached meshes.

[Max Vujovic]

Created attachment 170762 [details]
Patch

Now the patch uses the mesh type to check if a_triangleCoord is a valid attribute.

[Early Warning System Bot]

Comment on attachment 170762 [details]
Patch

Attachment 170762 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14559802

[Alexandru Chiculita]

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

Looks good.

> LayoutTests/css3/filters/resources/invalid-a-triangle-coord-with-attached-mesh.vs:9
> +attribute vec3 a_triangleCoord;
> +
> +attribute vec4 a_position;
> +
> +uniform mat4 u_projectionMatrix;

Remove the empty lines.

> LayoutTests/css3/filters/resources/invalid-custom-attribute.vs:9
> +attribute float my_attribute;
> +
> +attribute vec4 a_position;
> +
> +uniform mat4 u_projectionMatrix;

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-3805
> -		A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */ = {isa = PBXBuildFile; fileRef = A24A3A9C162499FF00522745 /* CustomFilterConstants.h */; };

I think you can revert the xcode changes.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:18251
> -				A24A3A9C162499FF00522745 /* CustomFilterConstants.h */,
> +				A2B2AE7B16375EE500CFA50B /* CustomFilterConstants.h */,

ditto

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22694
> -				A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */,
> +				A2B2AE7C16375EE500CFA50B /* CustomFilterConstants.h in Headers */,

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:63
>      // StyleCustomFilterProgram has the only implementation for the following method. That means, it casts to StyleCustomFilterProgram
> -    // withouth checking the type. If you add another implementation, also add a mechanism to check for the correct type.
> +    // without checking the type. If you add another implementation, also add a mechanism to check for the correct type.

I think this comment is not needed anymore.

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

Pass the mesh type as a parameter to keep this static.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:80
> +            if (symbol.name == "a_triangleCoord" && m_programInfo.meshType() != MeshTypeDetached) {

Why use the != operator instead of == ? We just have two modes at this moment. 

nit: Put the fastest comparison first. It will avoid making the strcmp when not needed.

[Peter Beverloo (cr-android ews)]

Comment on attachment 170762 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 170762 [details]
Patch

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

[Max Vujovic]

Created attachment 170979 [details]
Patch

Trying to fix Qt, Chromium EWS build errors.

[Early Warning System Bot]

Comment on attachment 170979 [details]
Patch

Attachment 170979 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14611143

[Max Vujovic]

Created attachment 170995 [details]
Patch

Qt EWS build fix attempt.

[Early Warning System Bot]

Comment on attachment 170995 [details]
Patch

Attachment 170995 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14617118

[Max Vujovic]

Created attachment 171023 [details]
Patch

Trying qt-wk2 again.

[Early Warning System Bot]

Comment on attachment 171023 [details]
Patch

Attachment 171023 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14614172

[Max Vujovic]

Created attachment 171029 [details]
Patch

Trying qt-wk2 again.

[Max Vujovic]

Created attachment 171046 [details]
Patch

Updated ChangeLog descriptions.

[Max Vujovic]

Thanks for the review, Alex!

(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=170762&action=review
> 
> Looks good.
> 
> > LayoutTests/css3/filters/resources/invalid-a-triangle-coord-with-attached-mesh.vs:9
> > +attribute vec3 a_triangleCoord;
> > +
> > +attribute vec4 a_position;
> > +
> > +uniform mat4 u_projectionMatrix;
> 
> Remove the empty lines.

Done. It looks better without the empty lines. I've also removed the empty lines from the other shaders referenced by the test file.

> 
> > LayoutTests/css3/filters/resources/invalid-custom-attribute.vs:9
> > +attribute float my_attribute;
> > +
> > +attribute vec4 a_position;
> > +
> > +uniform mat4 u_projectionMatrix;
> 
> Ditto.

Done.

> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-3805
> > -		A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */ = {isa = PBXBuildFile; fileRef = A24A3A9C162499FF00522745 /* CustomFilterConstants.h */; };
> 
> I think you can revert the xcode changes.

I need this to fixing an incorrect path to CustomFilterConstants.h in the Xcode project. The project thinks the file is in platform/graphics, not platform/graphics/filters. I need to fix the reference so that StyleResolver.cpp can find CustomFilterConstants.h. 

> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:18251
> > -				A24A3A9C162499FF00522745 /* CustomFilterConstants.h */,
> > +				A2B2AE7B16375EE500CFA50B /* CustomFilterConstants.h */,
> 
> ditto
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22694
> > -				A24A3A9D162499FF00522745 /* CustomFilterConstants.h in Headers */,
> > +				A2B2AE7C16375EE500CFA50B /* CustomFilterConstants.h in Headers */,
> 
> ditto
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:63
> >      // StyleCustomFilterProgram has the only implementation for the following method. That means, it casts to StyleCustomFilterProgram
> > -    // withouth checking the type. If you add another implementation, also add a mechanism to check for the correct type.
> > +    // without checking the type. If you add another implementation, also add a mechanism to check for the correct type.
> 
> I think this comment is not needed anymore.

Removed. You're right.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:66
> > +bool CustomFilterValidatedProgram::validateSymbols(const Vector<ANGLEShaderSymbol>& symbols)
> 
> Pass the mesh type as a parameter to keep this static.

Done.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:80
> > +            if (symbol.name == "a_triangleCoord" && m_programInfo.meshType() != MeshTypeDetached) {
> 
> Why use the != operator instead of == ? We just have two modes at this moment. 

Done. I was using a whitelist approach (only allow a_triangleCoord in detached meshes) vs. a blacklist approach (don't allow a_triangleCoord in attached meshes). However, since we only have two mesh types right now, it's easier to read with the blacklist approach.

> 
> nit: Put the fastest comparison first. It will avoid making the strcmp when not needed.

Done. I was assuming most of the time people will be using attached meshes. If that's true, the original arrangement is faster. However, I can't back that up with any data, so I'll put the faster comparison first :)

[Max Vujovic]

Comment on attachment 171046 [details]
Patch

Setting cq-. I need to rebase the patch.

[Max Vujovic]

Created attachment 171303 [details]
Patch

Rebased patch.

[Max Vujovic]

Created attachment 171306 [details]
Patch

Rebased patch v2. I had a merge issue in the previous one.

[WebKit Review Bot]

Comment on attachment 171306 [details]
Patch

Clearing flags on attachment: 171306

Committed r132903: <http://trac.webkit.org/changeset/132903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Vujovic]

Thanks for the review, Dean!