Closed Bug 606141 Opened 10 years ago Closed 9 years ago

Javascript JIT engine crashes when array initialiser too large

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: lyricconch, Assigned: bzbarsky)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 485032 [details] [diff] [review]
ArrayCompPush can deep-bail, so handle that.

I _think_ this is the right fix....
Attachment #485032 - Flags: review?(jorendorff)
Assignee: general → bzbarsky
Priority: -- → P1
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 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'.
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....
And I have no idea how to address comment 4.  :(
(In reply to comment #6)
Yes, deep bails are extremely delicate.  The key rule here is "don't return success if you deep-bailed".
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.  ;)
(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?
You mean the cx->tracerState->builtinStatus == 0 check?  Sure thing.  Will update patch in a few.
Attachment #485032 - Attachment is obsolete: true
Attachment #485032 - Flags: review?(jorendorff)
Attachment #485473 - Flags: review?(jorendorff)
blocking2.0: --- → ?
blocking2.0: ? → beta8+
Whiteboard: [need review]
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.
Attachment #485473 - Flags: review?(jorendorff) → review+
Whiteboard: [need review] → [need landing]
Pushed http://hg.mozilla.org/tracemonkey/rev/b55d8612e834
Flags: in-testsuite?
Whiteboard: [need landing] → fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/b55d8612e834
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 614208
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.