Closed Bug 1329051 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Canvas: WebGL, defect, P3, minor)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: david.catuhe, Assigned: cleu)

References

()

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 1 obsolete file)

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
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(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)
Flags: needinfo?(jgilbert)
Assignee: nobody → cleu
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.
Flags: needinfo?(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
Depends on: CVE-2011-2988
Flags: needinfo?(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.
Is there a usecase that needs this large of a shader, or is this just edge-case experimentation?
Severity: normal → minor
Flags: needinfo?(david.catuhe)
Priority: -- → P3
Whiteboard: gfx-noted
I'll prepare the needed environment for it.
Flags: needinfo?(cleu)
Summary: WebGL shader crashes with "shaderSource: Soure has more than 262143 characters. (Driver workaround)" while it is working on Chrome and Edge → WebGL shader crashes with "shaderSource: Source has more than 262143 characters. (Driver workaround)" while it is working on Chrome and Edge
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.
Flags: needinfo?(cleu)
Attachment #8826063 - Flags: review?(jgilbert)
Attachment #8826063 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
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
Flags: needinfo?(cleu)
Keywords: checkin-needed
Attachment #8826063 - Attachment is obsolete: true
Flags: needinfo?(cleu)
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?
Flags: needinfo?(cbook)
indeed works great now :)
Flags: needinfo?(cbook)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6a6ad3296e
Remove Old Driver Workaround. r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/bf6a6ad3296e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(david.catuhe)
You need to log in before you can comment on or make changes to this bug.