Bug 52390 – Must strip comments from WebGL shaders before enforcing character set

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The checks added in https://bugs.webkit.org/show_bug.cgi?id=50929 which verify that WebGL shaders must only contain characters from the ESSL character set are too strict. We are rejecting shaders which contain quotes in comments, which is causing much user content to break. We can allow these shaders to work by stripping comments before handing the shaders down to the OpenGL implementation. When doing so we need to be careful to preserve line numbers.

[Kenneth Russell]

As part of this fix, the changes from https://bugs.webkit.org/show_bug.cgi?id=52396 are going to be reverted. They are allowing invalid characters to be passed to entry points which should not allow them under any circumstances (bindAttribLocation, getAttribLocation, and getUniformLocation).

[Kenneth Russell]

Created attachment 79051 [details]
Patch

[David Levin]

Comment on attachment 79051 [details]
Patch

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

A few small nits below to consider. Thanks!

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:153
> +        void process(UChar character);

Remove "character" as it adds no information.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:250
> +                if (peek(temp)) {

Might be nice to collapse this with the previous if
if (c == '/' && peek(temp)) {

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:290
> +                if (peek(temp) && temp == '/') {

Would be nice to combine these if's (like before).

[Zhenyao Mo]

I see " ' ` are added back to the invalid charset now that comments are stripped first.  However, won't something like that fail?

#ERROR "..."

Gregg mentioned that the above is actually in the GLSL conformance test, so it should be legal.

[Kenneth Russell]

(In reply to comment #3)
> (From update of attachment 79051 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79051&action=review
> 
> A few small nits below to consider. Thanks!
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:153
> > +        void process(UChar character);
> 
> Remove "character" as it adds no information.

Done.

> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:250
> > +                if (peek(temp)) {
> 
> Might be nice to collapse this with the previous if
> if (c == '/' && peek(temp)) {

Done.

> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:290
> > +                if (peek(temp) && temp == '/') {
> 
> Would be nice to combine these if's (like before).

Done.

Will address all of these in the landed patch.

[Kenneth Russell]

(In reply to comment #4)
> I see " ' ` are added back to the invalid charset now that comments are stripped first.  However, won't something like that fail?
> 
> #ERROR "..."
> 
> Gregg mentioned that the above is actually in the GLSL conformance test, so it should be legal.

It probably will. Feel free to file a separate bug about that issue. I think it's more important at this point to get back to a state where obviously invalid characters are rejected from APIs like getUniformLocation.

[Kenneth Russell]

Committed r76063: <http://trac.webkit.org/changeset/76063>

[Eric Seidel (no email)]

webkit-patch failure-reason seems to think that this caused a canvas test to start failing on leopard?

SUCCESS: Build 26796 (r76065) was the first to show failures: set([u'canvas/philip/tests/2d.text.draw.align.center.html'])

[Kenneth Russell]

(In reply to comment #8)
> webkit-patch failure-reason seems to think that this caused a canvas test to start failing on leopard?
> 
> SUCCESS: Build 26796 (r76065) was the first to show failures: set([u'canvas/philip/tests/2d.text.draw.align.center.html'])

As far as I have seen that test has been failing at least intermittently for a while. I don't think there's any way this patch could have caused it to start failing.