1329051 - WebGL shader crashes with "shaderSource: Source has more than 262143 characters. (Driver workaround)" while it is working on Chrome and Edge

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[David Catuhe]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

Go there:
http://www.babylonjs-playground.com/debug.html#18TRYT#26
Check the console


Actual results:

Shader not compile, No mirror on the box


Expected results:

Shader should compile and the box should display a black mirror

Comment 1

[Michael Leu[:Lenzak](UTC+8)]

I can reproduce it on both Windows and Mac, I think the cause is here.
https://dxr.mozilla.org/mozilla-central/rev/e68cbc3b5b3d3fba4fe3e17e234713020f44e4a0/dom/canvas/WebGLShader.cpp#176

The shader demo can run without problem after removing this if statement.

But I am not sure whether it is safe to just remove it or not.

Hi Jeff, do you have any suggestion about this one?

Maybe we need more check to determine whether we need to block over-length shader code or not.

Comment 2

[Kelsey Gilbert [:jgilbert]]

(In reply to Michael Leu[:lenzak800](UTC+8) from comment #1)
> I can reproduce it on both Windows and Mac, I think the cause is here.
> https://dxr.mozilla.org/mozilla-central/rev/
> e68cbc3b5b3d3fba4fe3e17e234713020f44e4a0/dom/canvas/WebGLShader.cpp#176
> 
> The shader demo can run without problem after removing this if statement.
> 
> But I am not sure whether it is safe to just remove it or not.
> 
> Hi Jeff, do you have any suggestion about this one?
> 
> Maybe we need more check to determine whether we need to block over-length
> shader code or not.

I don't know. Please track down the original commit and find the bug. Hopefully it will tell us how we can retest. I suspect we don't need it anymore. (workaround for 10.5-era OSX, or something)

Comment 3

[Michael Leu[:Lenzak](UTC+8)]

I suspect we don't need this anymore, too.

However, I don't know what's this workaround for.
 
The bug is bug1109945, it is about re-factoring shader manipulation and it's a big patch.

But I didn't see anything about this length limit throughout discussion there.

Comment 4

[Kelsey Gilbert [:jgilbert]]

That code was just moved around.
Also tracing back, it used to be ((1<<18)-1).

Here's the genesis of this restriction:
https://hg.mozilla.org/mozilla-central/rev/aa69bb7c6bfe

This is apparently one of our oldest workarounds, originally found on Firefox 4, fixed in Nightly8: Bug 665936

Comment 5

[Kelsey Gilbert [:jgilbert]]

This was an ANGLE bug, so it's likely it was fixed at some point. We should retest.
We should be careful here though, as this was a sec-bug before.

lenzak800: Can you try removing the limit and seeing what happens, particularly on 32-bit?
This should not require Windows, being in the ANGLE shader preprocessor.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Is there a usecase that needs this large of a shader, or is this just edge-case experimentation?

Comment 7

[Michael Leu[:Lenzak](UTC+8)]

I'll prepare the needed environment for it.

Comment 8

[Michael Leu[:Lenzak](UTC+8)]

I prepared a Windows 7 32-bit computer with HD4000 gfx and tested it with the old workaround code removed.

And it works with no problem, I think we can just remove it.

Comment 9

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug1329051 - Remove old driver workaround

Comment 10

[Michael Leu[:Lenzak](UTC+8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=455cd6cb2151cde1171bbd15bab30675f172dec3

Here is try

Comment 11

[Carsten Book [:Tomcat]]

needs rebasing

patching file dom/canvas/WebGLShader.cpp
Hunk #1 FAILED at 172
1 out of 1 hunks FAILED -- saving rejects to file dom/canvas/WebGLShader.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug1329051-Remove-old-driver-workaround.patch

Comment 12

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug1329051 - Remove old driver workaround r=jgilbert

Comment 13

[Michael Leu[:Lenzak](UTC+8)]

Ht Tomcat,

I have re-applied the patch on the latest m-c.

But it seems to be the same with the original patch.

Can you post the .rej file here if there's conflict again?

Comment 14

[Carsten Book [:Tomcat]]

indeed works great now :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6a6ad3296e
Remove Old Driver Workaround. r=jgilbert

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf6a6ad3296e