(function(e) { eval("\ [(function() {\ x.k = function(){}\ })() \ for (x in [0])]\ ") })() asserts js debug shell without -j on TM tip at Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp:2290 autoBisect shows this is probably related to bug 452498: The first bad revision is: changeset: 26784:2cf0bbe3772a user: Brendan Eich date: Sun Apr 05 21:17:22 2009 -0700 summary: upvar2, aka the big one take 2 (452598, r=mrbkap).

Tricksy. Look for a patch after I finish a review and make regex literals trace again. The bisect result looks red-herring-ish; I think extra tracing just happened to expose a latent bug. Writing user-specified values into dslots seems very very dodgy. I don't want to have to think about how or whether that could be exploited, erring on the side of paranoia. I take it this is the assert at start of js_ArrayCompPush? 2290 lands on this right now according to hgweb: 2289 ok = js_MergeSort(vec, (size_t) newlen, elemsize, 2290 sort_compare_strings, cx, mergesort_tmp);

(In reply to comment #1) > I take it this is the assert at start of js_ArrayCompPush? 2290 lands on this > right now according to hgweb: Yup, should be that assert: js> (function(e) { eval("\ [(function() {\ x.k = function(){}\ })() \ for (x in [0])]\ ") })() Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp:2407 Program received signal SIGABRT, Aborted. 0x00007fff85343fe6 in __kill () (gdb) bt #0 0x00007fff85343fe6 in __kill () #1 0x0000000100138deb in JS_Assert (s=0x1001c8b9c "OBJ_IS_DENSE_ARRAY(cx, obj)", file=0x1001bf4e3 "../jsarray.cpp", ln=2407) at ../jsutil.cpp:70 #2 0x00000001000287a3 in js_ArrayCompPush (cx=0x100411580, obj=0x1003f6600, v=22) at ../jsarray.cpp:2407 #3 0x00000001000a03c5 in js_Interpret (cx=0x100411580) at jsops.cpp:4142 #4 0x00000001000a42ec in js_Execute (cx=0x100411580, chain=0x1003f6580, script=0x100415060, down=0x100889450, flags=16, result=0x7fff5fbfe988) at jsinterp.cpp:1620 #5 0x00000001000bf9e1 in obj_eval (cx=0x100411580, obj=0x1003f6000, argc=1, argv=0x100889520, rval=0x7fff5fbfe988) at ../jsobj.cpp:1543 #6 0x00000001000a525a in js_Invoke (cx=0x100411580, argc=1, vp=0x100889510, flags=2) at jsinterp.cpp:1378 #7 0x0000000100090cc2 in js_Interpret (cx=0x100411580) at jsops.cpp:2305 #8 0x00000001000a42ec in js_Execute (cx=0x100411580, chain=0x1003f6000, script=0x100414fe0, down=0x0, flags=0, result=0x7fff5fbff5e8) at jsinterp.cpp:1620 #9 0x00000001000113f6 in JS_ExecuteScript (cx=0x100411580, obj=0x1003f6000, script=0x100414fe0, rval=0x7fff5fbff5e8) at ../jsapi.cpp:4970 #10 0x0000000100009cac in Process (cx=0x100411580, obj=0x1003f6000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:532 #11 0x000000010000a574 in ProcessArgs (cx=0x100411580, obj=0x1003f6000, argv=0x7fff5fbff7e8, argc=0) at ../../shell/js.cpp:848 #12 0x000000010000a88b in main (argc=0, argv=0x7fff5fbff7e8, envp=0x7fff5fbff7f0) at ../../shell/js.cpp:4898 (gdb) f 2 #2 0x00000001000287a3 in js_ArrayCompPush (cx=0x100411580, obj=0x1003f6600, v=22) at ../jsarray.cpp:2407 2407 JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj)); (gdb) l 2402 } 2403 2404 JSBool JS_FASTCALL 2405 js_ArrayCompPush(JSContext *cx, JSObject *obj, jsval v) 2406 { 2407 JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj)); 2408 uint32_t length = (uint32_t) obj->fslots[JSSLOT_ARRAY_LENGTH]; 2409 JS_ASSERT(length <= js_DenseArrayCapacity(obj)); 2410 2411 if (length == js_DenseArrayCapacity(obj)) { (gdb)

(In reply to comment #1) > I take it this is the assert at start of js_ArrayCompPush? 2290 lands on this > right now according to hgweb: > > 2289 ok = js_MergeSort(vec, (size_t) newlen, elemsize, > 2290 sort_compare_strings, cx, mergesort_tmp); Apologies, I think I copied the assert with the wrong line number. I had taken the assert from TM repository 2cf0bbe3772a, which was from 2009. Comment 2 should be the up-to-date one.

Now asserts at Assertion failure: obj->isDenseArray(), at ../jsarray.cpp:2380 A worse-looking testcase is coming up - this second one seems to be able to crash opt shells too.

function f(e) { eval("\ [((function g(o, bbbbbb) {\ if (aaaaaa = bbbbbb) {\ return window.r = []\ }\ g(aaaaaa, bbbbbb + 1);\ #3={}\ })([], 0)) \ for (window in this) \ for each(x in [0, 0])\ ]\ ") } t = 1; f() crashes opt shell on TM tip without -j at js_GC near null and asserts js debug shell on TM tip without -j at Assertion failure: obj->isDenseArray(), at ../jsarray.cpp:2380 === Mac js opt crash stack: Exception Type: EXC_BAD_ACCESS (SIGBUS) Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000016 Crashed Thread: 0 Dispatch queue: com.apple.main-thread Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 js-opt-32-tm-darwin 0x0004f392 js_GC + 5762 1 js-opt-32-tm-darwin 0x0002085b js_DestroyContext(JSContext*, JSDestroyContextMode) + 155 2 js-opt-32-tm-darwin 0x0000eda9 JS_DestroyContext + 25 3 js-opt-32-tm-darwin 0x00008cf5 main + 1189 4 js-opt-32-tm-darwin 0x000029ed _start + 208 5 js-opt-32-tm-darwin 0x0000291c start + 40

function f(e) { eval("[function () { w.r = 0 }() for (w in [0])]") } f(0); The lambda compiles to: 00000: trace 00001: name "w" 00004: zero 00005: setprop "r" 00008: pop 00009: stop The NAME instruction is loading the wrong slot. It should get the string "0". Instead it gets the array being constructed. The crash goes away if I delete the parameter `e`.

*steal* Should be quickish.

Correction: If I pass the lambda to dis(), it compiles to the code in comment 6. In the actual crashing test, it compiles to: main: 00000: 4 trace 00001: 4 getupvar 0 <--- insn to load w 00004: 4 zero 00005: 4 setprop "r" 00008: 4 pop 00009: 4 stop The upvar cookie is MAKE_UPVAR_COOKIE(1, 1). Comprehension-transplanting trouble, maybe.

I will look at this and try to advise... I'll be the advisor, like Dr. Lazarus in "Galaxy Quest". Maybe cdleary (who has gumption) can lose his shirt actually fighting the rock monster. "Look around you, can you form some kind of rudimentary lathe?" - Guy Fleegman /be

It's not comprehension-transplanting trouble. This fails too: function f(y) { eval("let (z=2, w=y) { (function () { w.p = 7; })(); }"); } var x = {}; f(x); assertEq(x.p, 7); Here's what I think the trouble is. In js_GetUpvar, there are four cases: if (!fp->fun) { vp = fp->slots + fp->script->nfixed; } else if (slot < fp->fun->nargs) { vp = fp->argv; } else if (slot == CALLEE_UPVAR_SLOT) { vp = &fp->argv[-2]; slot = 0; } else { slot -= fp->fun->nargs; JS_ASSERT(slot < fp->script->nslots); vp = fp->slots; } fp is the frame in which w is defined: the eval frame. I think we should go to the first branch. Instead we end up in the fourth, adjusting for fp->fun->nargs, which I'm pretty sure makes no sense in this frame.

This isn't going to win any awards, but it seems like the minimal change. We don't attempt to JIT this case: trace stopped: 13951: Null closure function object parent must be global object Abort recording of tree tests/js1_8_5/regress/regress-541255-4.js:8@60 at tests/js1_8_5/regress/regress-541255-4.js:8@64: lambda. Apparently the lambda created by the compiler (invoked from eval) is not parented to the Call object for the enclosing function, not the global. Is that correct? I added another test anyway, just in case that ever gets traced.

(In reply to comment #11) > Apparently the lambda created by the compiler (invoked from eval) is not Stray "not" at end of this line, right? > parented to the Call object for the enclosing function, not the global. Is that > correct? Correct, striking the stray not. > I added another test anyway, just in case that ever gets traced. Thanks. This is the notorious MakeUpvarForEval special case. I thought we had it covered (there are other tests; dmandelin knows of them too). Thanks again, will r+ with a nit in a sec. /be

Comment on attachment 435306 [details] [diff] [review] v1 >+ if (!fp->fun || fp->flags & JSFRAME_EVAL) { House style overparenthesizes bitwise against logical connectives, as well as against most other ops. >+ if (!fp->fun || fp->flags & JSFRAME_EVAL) { Ditto. /be

Want this on branches, but for now not blocking 1.9.1/1.9.2 pending landing on mozilla-central. Please request approval when you're ready to land.

Gary: Does this really affect the 1.9.1 branch as you nominated? Elsewhere it looks like it was pinned down to a regression from a fix that didn't land on that branch.

(In reply to comment #17) > Gary: Does this really affect the 1.9.1 branch as you nominated? Elsewhere it > looks like it was pinned down to a regression from a fix that didn't land on > that branch. Yes, it really does still affect. Upvar2 landed on 1.9.1, and the testcases in comment #0 and comment #6 still assert: 32-bit 1.9.1 changeset 2eb0724d74f8: $ ./js-dbg-32-191-darwin js> function f(e) { eval("[function () { w.r = 0 }() for (w in [0])]") } f(0); js> Assertion failure: OBJ_IS_DENSE_ARRAY(cx, obj), at ../jsarray.cpp:2361 Trace/BPT trap

Does the same patch work for 1.9.1 and 1.9.2?

Moving forward to the next release as this didn't make it for 3.6.9 and 3.5.12

Deferring to a future point release as we have run out of time. If this absolutely needs to be fixed and cal land today or tomorrow, please send a note to release-drivers@mozilla.org

dmandelin: please find someone on your team to get this into the 1.9.2 branch before code freeze (Aug 1).

(In reply to comment #22) > dmandelin: please find someone on your team to get this into the 1.9.2 > branch before code freeze (Aug 1). The existing patch applies directly, so requesting approval on that. Btw, feel free to ask the original patch author directly to do branch landings.

Comment on attachment 435306 [details] [diff] [review] v1 Approved for 1.9.2.20, a=dveditz for release-drivers

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-541255-2.js.

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.