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.
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).
Created attachment 79051 [details] Patch
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).
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.
(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.
(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.
Committed r76063: <http://trac.webkit.org/changeset/76063>
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'])
(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.