Bug 52390 - Must strip comments from WebGL shaders before enforcing character set
Summary: Must strip comments from WebGL shaders before enforcing character set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on: 50929 52396
Blocks: 52833
  Show dependency treegraph
 
Reported: 2011-01-13 12:09 PST by Kenneth Russell
Modified: 2011-01-20 12:45 PST (History)
7 users (show)

See Also:


Attachments
Patch (17.33 KB, patch)
2011-01-14 20:28 PST, Kenneth Russell
levin: review+
kbr: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2011-01-13 12:09:50 PST
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.
Comment 1 Kenneth Russell 2011-01-14 19:39:13 PST
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).
Comment 2 Kenneth Russell 2011-01-14 20:28:29 PST
Created attachment 79051 [details]
Patch
Comment 3 David Levin 2011-01-16 01:08:52 PST
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).
Comment 4 Zhenyao Mo 2011-01-18 10:05:12 PST
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.
Comment 5 Kenneth Russell 2011-01-18 13:31:52 PST
(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.
Comment 6 Kenneth Russell 2011-01-18 13:35:26 PST
(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.
Comment 7 Kenneth Russell 2011-01-18 14:12:11 PST
Committed r76063: <http://trac.webkit.org/changeset/76063>
Comment 8 Eric Seidel (no email) 2011-01-18 16:48:53 PST
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'])
Comment 9 Kenneth Russell 2011-01-18 16:50:43 PST
(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.