576620 - crash: getParameter stack corruption in the paths returning 4 ints

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kevin Brosnan]

Enable webgl in about:config. Load the WebGL conformance tests and let them run. Shortly the browser will crash.

Nvidia GeForce GTX 260M
Nvidia driver v257.21

Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1

Comment 1

[Kevin Brosnan]

bp-f6916010-dbca-4e20-a283-007332100702
bp-69c9c6aa-0dd5-47c8-b5d7-f54da2100702

0  	xul.dll  	mozilla::WebGLContext::GetParameter  	 content/canvas/src/WebGLContextGL.cpp:1345
1 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
2 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1789

Comment 2

[Aaron Train [:aaronmt]]

I can reproduce a test crash on OSX 10.6.4, although I have a different signature

[@ mozilla::WebGLContext::BufferData_size(unsigned int, int, unsigned int) ]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100702 Minefield/4.0b2pre

http://crash-stats.mozilla.com/report/pending/31468637-9d04-40c5-ac29-fd99e2100702

Comment 3

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

Here I am on linux x86-64. I can't reproduce any crash, but I have something reminiscent of comment 2: I seem to have an infinite loop consuming all my system's memory in BufferData_size.

Interrupting the program while it was stalling, I got this back-trace:

#0  memset () at ../sysdeps/x86_64/memset.S:1050
#1  0x00007ffff5df36c5 in mozilla::WebGLBuffer::ZeroDataIfElementArray (this=0x7fffcfb6b380)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:563
#2  0x00007ffff5de8fb5 in mozilla::WebGLContext::BufferData_size (this=0x7fffd3176e20, target=
    34963, size=-35, usage=35048)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp:356
#3  0x00007ffff659cfa1 in nsICanvasRenderingContextWebGL_BufferData (cx=0x7fffdc178c00, argc=3, 
    vp=0x7fffe94fd7b8) at ../../../../dist/include/CustomQS_WebGL.h:136
#4  0x00007ffff4d0b992 in js_Invoke (cx=0x7fffdc178c00, args=..., flags=0)
    at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:546

Comment 4

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

@ Kevin Brosnan:

So you have a crash in WebGLContextGL.cpp:1345. However in my working copy this line is not a plausible crash line, so I guess we have different parent revisions. Could you tell me what hg revision this line 1345 is referring to and/or paste the corresponding code with some context lines?

Comment 5

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

@ anybody reporting about crashes:

It would be helpful if you could tell what particular test from the test suite makes Minefield crash.

Comment 6

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

This test is the one that gives the crazy memory consumption and backtrace given in comment 3:

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

Comment 7

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

Here, all other tests listed in that page complete without crash or stalling. So let's concentrate in the test mentioned in comment 6.

Comment 8

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

@ Kevin Brosnan in comment 1:

Sorry, I didn't see your links. So your crash is in here:

1335 //
1336 // Complex values
1337 //
1338 case LOCAL_GL_DEPTH_RANGE: // 2 floats
1339 case LOCAL_GL_ALIASED_POINT_SIZE_RANGE: // 2 floats
1340 case LOCAL_GL_ALIASED_LINE_WIDTH_RANGE: // 2 floats
1341 {
1342 GLfloat fv[2] = { 0 };
1343 gl->fGetFloatv(pname, fv);
1344 wrval->SetAsArray(nsIDataType::VTYPE_FLOAT, nsnull,
1345 2, static_cast<void*>(fv));
1346 }

^ line 1345 here.

This is really strange, I don't know how it can be crashing in here. The only reason I could think of is if the wrval object was null or 'bad', but above we did:

    NS_ENSURE_TRUE(wrval, NS_ERROR_FAILURE);

So we know that wrval is not null; is there still a possibility for it to be 'bad' ?

Comment 9

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

(In reply to comment #3)
> Here I am on linux x86-64. I can't reproduce any crash, but I have something
> reminiscent of comment 2: I seem to have an infinite loop consuming all my
> system's memory in BufferData_size.
> 
> Interrupting the program while it was stalling, I got this back-trace:
> 
> #0  memset () at ../sysdeps/x86_64/memset.S:1050
> #1  0x00007ffff5df36c5 in mozilla::WebGLBuffer::ZeroDataIfElementArray
> (this=0x7fffcfb6b380)
>     at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:563
> #2  0x00007ffff5de8fb5 in mozilla::WebGLContext::BufferData_size
> (this=0x7fffd3176e20, target=
>     34963, size=-35, usage=35048)
>     at /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp:356
> #3  0x00007ffff659cfa1 in nsICanvasRenderingContextWebGL_BufferData
> (cx=0x7fffdc178c00, argc=3, 
>     vp=0x7fffe94fd7b8) at ../../../../dist/include/CustomQS_WebGL.h:136
> #4  0x00007ffff4d0b992 in js_Invoke (cx=0x7fffdc178c00, args=..., flags=0)
>     at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:546

Wow, this one is actually very easy: here BufferData_size is called with a negative size. Will patch that ASAP.

Comment 10

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

Attachment: fix bufferData validation

This fixes bufferData arguments validation:
 - reject negative sizes (fixes the above-reported infinite loop with infinite memory usage)
 - also reject invalid 'usage' enums.

Comment 11

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

Confirming that with above patch 9comment 10), this test does no longer take infinitely long and infinite memory to complete:

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

However it reports a good number of test failures that we should fix.

Comment 12

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

Attachment: fix bufferData validation

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/bf361368208f

This fixes the bufferData bug.

As for the original bug reported here, I still can't reproduce or understand it, see comment 8.

Comment 14

[Kevin Brosnan]

The test that crashes it is https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/gl-enum-tests.html however it seems to be related to the fast paced loading of several webgl items in succession as loading the test by itself does not cause the crash.

Comment 15

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

Kevin and me just investigated this. What we found is that wrval is a null pointer, and the crash occurs when we are dereferencing it.

Here is the code creating this null pointer, at the beginning of GetParameter:

    nsCOMPtr<nsIWritableVariant> wrval
        = do_CreateInstance("@mozilla.org/variant;1");
    NS_ENSURE_TRUE(wrval, NS_ERROR_FAILURE);


This prompts 2 questions:

1) Why is do_CreateInstance returning null? Need either a XPCOM guru, or to step into it with a debugger.

2) NS_ENSURE_TRUE doesn't actually ensure anything! We should do what we can to get a non-null pointer, but if after that we still get a null pointer, then we should bail out rather than continue executing this function and crashing. I guess that the most graceful thing to do is to generate a GL error (maybe OUT_OF_MEMORY) and return NS_OK.

Comment 16

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

Sorry, rather NS_ERROR_FAILURE, we probably don't want to keep executing this javascript program after such a strange error has occured.

Comment 17

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

(first of all, ignore 2 last comments, I was wrong about NS_ENSURE_TRUE).

@ Kevin: can you still reproduce this crash with mozilla-central from today?

Comment 18

[Kevin Brosnan]

Yes, http://crash-stats.mozilla.com/report/index/96965ba5-eadf-4ac6-84b8-49c422100825

Though it took longer to crash than I recall.

Comment 19

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

Good news, I can reproduce, though only in release builds, and only on Windows.

Working on it.

Comment 20

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

Attachment: Fix getParameter crash

One byte diff! but took lots of printf debugging to figure out.

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/6434c0173828

Comment 22

[Kevin Brosnan]

v. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101102 Firefox/4.0b8pre