675625 - Crash in WebGL conformance test glsl-conformance.html on NVIDIA cards (Win and Linux) [@ libnvidia-glcore.so.270.41.06@0x4d8861] [@ libnvidia-glcore.so.270.41.19@0x7973ce ]

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Firefox crashes when performing WebGL conformance tests on Ubuntu 11.04. Other OSs don't show this problem. Issue is also reproducible on latest nightly.

Reproducible:always

Steps to reproduce:
 1. Go to https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/webgl-conformance-tests.html
 2. Run tests

Actual results:
 - browser crashes

Expected results:
 - browser does not crash

Crash reports:
bp-a7fe3239-0f60-4453-90b5-25fb62110801 08/01/201105:43 PM
bp-0d0607c3-f517-45b2-8227-e3b9e2110801 08/01/201105:27 PM
bp-68c6fb9a-fc4d-47ed-8623-4716d2110801 08/01/201105:08 PM
bp-e03262cc-d481-4122-9070-849722110801 08/01/201105:04 PM

Comment 1

[juan becerra [:juanb]]

George, does this happen if you toggle hardware acceleration? Does it happen in different hardware?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

I confirm this on debian testing, x86-64, NVIDIA 270.41.19.

Investigating.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Here, only test in the test suite that's crashing is:

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/glsl-conformance.html

The crash occurs while linking a shader program.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Here's a stack obtained in a debug build with MOZ_GL_DEBUG:

(gdb) bt
#0  0x00007f34a611a1cd in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#1  0x00007f34a611a040 in __sleep (seconds=<value optimized out>)
    at ../sysdeps/unix/sysv/linux/sleep.c:138
#2  0x00007f34a3057405 in ah_crap_handler (signum=11)
    at /home/bjacob/mozilla-central/toolkit/xre/nsSigHandlers.cpp:121
#3  0x00007f34a305c958 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fffd43d0b70, 
    context=0x7fffd43d0a40) at /home/bjacob/build/firefox/toolkit/profile/nsProfileLock.cpp:226
#4  <signal handler called>
#5  0x00007f3486d2d3ce in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#6  0x00007f3486d2f961 in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#7  0x00007f3486d21129 in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#8  0x00007f3486c2246d in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#9  0x00007f3486c26ff8 in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#10 0x00007f3486c27525 in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#11 0x00007f348726fa1d in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#12 0x00007f3487278121 in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#13 0x00007f3487264710 in ?? () from /usr/lib/libnvidia-glcore.so.270.41.19
#14 0x00007f34a37c8b9a in mozilla::gl::GLContext::fLinkProgram (this=0x23115a0, program=19)
    at ../../../dist/include/GLContext.h:1683
#15 0x00007f34a37d707e in mozilla::WebGLContext::LinkProgram (this=0xa368a0, pobj=0x47415b0)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp:2891
#16 0x00007f34a402caac in nsIDOMWebGLRenderingContext_LinkProgram (cx=0xdb65f0, argc=1, 
    vp=0x7f348f3ff088) at /home/bjacob/build/firefox/js/src/xpconnect/src/dom_quickstubs.cpp:32124
#17 0x00007f34843612ed in ?? ()
#18 0x00007f348ddb8461 in ?? ()
#19 0x0000000000000014 in ?? ()
#20 0x00007fffd43d3880 in ?? ()
#21 0x0000000000000000 in ?? ()

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Please confirm that the crash also happens in Chromium.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Minimal testcase

Here's a pretty minimal testcase. Reload the page until it crashes. Here it typically crashes at the latest on the second or third attempt.

This testcases uses a uniform with an identifier 254 characters long. 254 is the minimum to get a crash. With 253 characters, there's no crash.

This is unfortunate as the WebGL spec allows identifiers up to 256 characters long.

The good news is that ANGLE already has code to enforce this kind of restrictions so the patch (only for linux/NVIDIA systems, as it breaks the spec) should be easy.

Comment 7

[Kenneth Russell]

Could the long identifier mapping support, which zmo recently added to ANGLE, fix this problem?

Comment 8

[George Carstoiu]

I can confirm issue is also reproducing in Chromium 12.0.742.112 (Developer Build 90304) Ubuntu 11.04

Regression range:
Last good nightly: 2011-05-24
First bad nightly: 2011-05-25

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=456c915b3caf&tochange=836aa9658341

Comment 9

[Piers Daniell]

I reproduced this in Windows and triaged the issue. Filed an internal nvbug 858355.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Release drivers: do you want to take into Firefox 6 a patch for this bug? It would consist in rejecting WebGL shader with identifiers longer than 253 characters. Currently identifiers longer than 256 characters are rejected.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #8)
> I can confirm issue is also reproducing in Chromium 12.0.742.112 (Developer
> Build 90304) Ubuntu 11.04
> 
> Regression range:
> Last good nightly: 2011-05-24
> First bad nightly: 2011-05-25
> 
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=456c915b3caf&tochange=836aa9658341

This is surprising. This is now known to be a NVIDIA bug. This regression range contains the ANGLE update to r653, and I don't see how this can be relevant to this bug.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #7)
> Could the long identifier mapping support, which zmo recently added to
> ANGLE, fix this problem?

I have no idea. Do you have a link? If it's about mapping WebGL identifiers to shortened identifiers then yes it would fix it.

Comment 13

[Kenneth Russell]

zmo's long identifier name mapping was tracked in http://code.google.com/p/angleproject/issues/detail?id=144 . There were a couple of CLs; there was a bug in the first one.

I don't think this is enabled by default yet.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #11)
> (In reply to comment #8)
> > I can confirm issue is also reproducing in Chromium 12.0.742.112 (Developer
> > Build 90304) Ubuntu 11.04
> > 
> > Regression range:
> > Last good nightly: 2011-05-24
> > First bad nightly: 2011-05-25
> > 
> > Pushlog:
> > http://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=456c915b3caf&tochange=836aa9658341
> 
> This is surprising. This is now known to be a NVIDIA bug. This regression
> range contains the ANGLE update to r653, and I don't see how this can be
> relevant to this bug.

I confirm that the crash doesn't happen in Firefox 5. Maybe the memory corruption just happens not to crash Firefox 5 out of pure luck?

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #13)
> zmo's long identifier name mapping was tracked in
> http://code.google.com/p/angleproject/issues/detail?id=144 . There were a
> couple of CLs; there was a bug in the first one.
> 
> I don't think this is enabled by default yet.

Thanks. I don't suppose that we'd want, as a fix for this issue, to rely on a new non-default feature, unless you're positive that it's well tested already (does Chrome use it yet?)

So as an interim solution until name mapping is deemed stable, I suppose my only solution is to reject identifiers longer than 253 chars.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #8)
> > > I can confirm issue is also reproducing in Chromium 12.0.742.112 (Developer
> > > Build 90304) Ubuntu 11.04
> > > 
> > > Regression range:
> > > Last good nightly: 2011-05-24
> > > First bad nightly: 2011-05-25
> > > 
> > > Pushlog:
> > > http://hg.mozilla.org/mozilla-central/
> > > pushloghtml?fromchange=456c915b3caf&tochange=836aa9658341
> > 
> > This is surprising. This is now known to be a NVIDIA bug. This regression
> > range contains the ANGLE update to r653, and I don't see how this can be
> > relevant to this bug.
> 
> I confirm that the crash doesn't happen in Firefox 5. Maybe the memory
> corruption just happens not to crash Firefox 5 out of pure luck?

As it turns out, Firefox 5 used ANGLE r550 which rejected all identifiers longer than 127 characters. This got changed to 256 characters in ANGLE r552. This explains why this regressed when we upgraded ANGLE. Given that Firefox 5 rejected identifiers longer than 127 chars, I think it's OK to reject identifiers longer than 253 chars now, i.e. the stable version of Firefox never implemented the spec which says 256 chars.

Comment 17

[Kenneth Russell]

(In reply to comment #15)
> (In reply to comment #13)
> > zmo's long identifier name mapping was tracked in
> > http://code.google.com/p/angleproject/issues/detail?id=144 . There were a
> > couple of CLs; there was a bug in the first one.
> > 
> > I don't think this is enabled by default yet.
> 
> Thanks. I don't suppose that we'd want, as a fix for this issue, to rely on
> a new non-default feature, unless you're positive that it's well tested
> already (does Chrome use it yet?)
> 
> So as an interim solution until name mapping is deemed stable, I suppose my
> only solution is to reject identifiers longer than 253 chars.

After reexamining the code, yes, Chrome has been using the long identifier mapping for some time now. See http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/service/shader_translator.cc?view=markup , in particular the use of the SH_MAP_LONG_VARIABLE_NAMES option, and the fetching of the attribute and uniform name maps after translation.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: limit identifier length to 250 chars

This patch limits WebGL shader identifiers to 250 chars. Initially I tried setting this constant to 253 but that still allowed the crash. Probably 252 would have been enough, but for good measure I set it to 250.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #17)
> After reexamining the code, yes, Chrome has been using the long identifier
> mapping for some time now. See
> http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/service/
> shader_translator.cc?view=markup , in particular the use of the
> SH_MAP_LONG_VARIABLE_NAMES option, and the fetching of the attribute and
> uniform name maps after translation.

Oh! Very good to know, thanks. I will try enabling this feature.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

OK, it seems like too big a patch to make it into Firefox 6 but we want to do that soon. I filed bug 676071 about that.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 550177 [details] [diff] [review]
limit identifier length to 250 chars

Test case please.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

A test case was already added to the WebGL conformance suite, that's why this bug was reported complaining that Firefox crashes running it.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 550177 [details] [diff] [review]
limit identifier length to 250 chars

Please approve this patch. I haven't been able to land it for now as the tree is in a very bad state at the moment and it's not clear to me if it should be closed.

Comment 24

[LegNeato]

Comment on attachment 550177 [details] [diff] [review]
limit identifier length to 250 chars

Approved for releases/mozilla-aurora and mozilla-beta. Please land asap

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Landed on central, aurora, beta.

http://hg.mozilla.org/mozilla-central/rev/7813728d68ff
http://hg.mozilla.org/releases/mozilla-aurora/rev/d28a4d6b8e50
http://hg.mozilla.org/releases/mozilla-beta/rev/8c2a57408242

Comment 26

[George Carstoiu]

 Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 - beta 5

Verified issue on all three channels (Nightly, Aurora, Beta) with the attached Minimal testcase and with the conformance test, and it is no longer reproducible.

Setting status Verified Fixed.