606141 - Javascript JIT engine crashes when array initialiser too large

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Lyricconch]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: 

with JIT enable, [i for(i in j)] will cause crash when j.length>=1048576.

Reproducible: Always

Steps to Reproduce:
// Crash with JIT
D:\Documents and Settings\Lymtics\My Documents>js -m -j
js> for (var str="0",i=20;i--;) void(str=str+str);
js> str.length
1048576
js> [i for(i in str)]
Assertion failure: tracecx->bailExit, at d:/Projects/mozilla-central/js/src/jstracer.cpp:7961
D:\Documents and Settings\Lymtics\My Documents>

// Exception Throw without JIT
D:\Documents and Settings\Lymtics\My Documents>js -m
js> for (var str="0",i=20;i--;) void(str=str+str);
js> [i for(i in str)]
typein:3: InternalError: array initialiser too large
js>

Also with JIT enabled, this makes firefox crash too.

Actual Results:  
crash.

Expected Results:  
an Exception throwed

Comment 1

[Boris Zbarsky [:bzbarsky]]

The issue is that js_ArrayCompPush_tn can deep bail, like so:

#1  0x001f1356 in js::DeepBail (cx=0x1800b90) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jstracer.cpp:8011
#2  0x0006a93c in js::LeaveTrace (cx=0x1800b90) at jscntxt.h:3263
#3  0x0006a94f in js_GetTopStackFrame (cx=0x1800b90) at jscntxt.h:3301
#4  0x0006b6af in PopulateReportBlame (cx=0x1800b90, report=0xbfffde3c) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jscntxt.cpp:1364
#5  0x0006c1dd in js_ReportErrorNumberVA (cx=0x1800b90, flags=0, callback=0x68f4c <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=200, charArgs=0, ap=0xbfffded0 "���\017��\n") at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jscntxt.cpp:1691
#6  0x0002b379 in JS_ReportErrorNumberUC (cx=0x1800b90, errorCallback=0x68f4c <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=200) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jsapi.cpp:5508
#7  0x0005e197 in ArrayCompPushImpl (cx=0x1800b90, obj=0x1f041b0, v=@0xbfffdf40) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jsarray.cpp:2006
#8  0x0005375c in js_ArrayCompPush_tn (cx=0x1800b90, obj=0x1f041b0, v=0xbfffdf40) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jsarray.cpp:2027

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: ArrayCompPush can deep-bail, so handle that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 485032 [details] [diff] [review]
ArrayCompPush can deep-bail, so handle that.

I _think_ this is the right fix....

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

We've had some issues in the past about placing arbitrary code in enter/leaveDeepBail blocks, see bug 562734 and bug 592069. So we should make extra sure it's safe.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 485032 [details] [diff] [review]
ArrayCompPush can deep-bail, so handle that.

>+    if (!ArrayCompPushImpl(cx, obj, ValueArgToConstRef(v))) {
>+        SetBuiltinError(cx);
>+        return JS_FALSE;
>+    }
>+
>+    return JS_TRUE;

I believe the 'return JS_TRUE' needs to be 'return cx->tracerState->builtinStatus == 0'.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Could be!  I couldn't figure out from the documentation how this stuff should work, and mostly cribbed it from other places that use deep bail calls....

Comment 7

[Boris Zbarsky [:bzbarsky]]

And I have no idea how to address comment 4.  :(

Comment 8

[Luke Wagner [:luke]]

(In reply to comment #6)
Yes, deep bails are extremely delicate.  The key rule here is "don't return success if you deep-bailed".

Comment 9

[Boris Zbarsky [:bzbarsky]]

That should not be a problem here.  The deep bail is due to constructing a JS exception on a codepath that plans to return failure.  ;)

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
> That should not be a problem here.  The deep bail is due to constructing a JS
> exception on a codepath that plans to return failure.  ;)

Ah, I missed that in the bug summary.  Still, that's an awfully delicate invariant and future modifications may break it.  If its not white-hot code (which I'm thinking its not), perhaps you could leave in the check?

Comment 11

[Boris Zbarsky [:bzbarsky]]

You mean the cx->tracerState->builtinStatus == 0 check?  Sure thing.  Will update patch in a few.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: ArrayCompPush can deep-bail, so handle that.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 485473 [details] [diff] [review]
ArrayCompPush can deep-bail, so handle that.

Hmm. Well, this patch will fix the problem. The opposite approach would be to make it so js_ArrayCompPush_tn never deep-bails. That was the original design: if we run out of memory, instead of reporting the error to cx, just return false without having modified the object in any way. Fall off trace and retry in the interpreter/methodjit code, which will report the error.

That would cost more lines of code, and it would just be one less place where we can deep-bail (among many). To me it seems like the approach you took is a little better, which is too bad -- deep bailing, blah.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/b55d8612e834

Comment 15

[Robert Sayre]

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