451806 - TM: assert on wikipedia

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

Stack trace: 

Assertion failure: sprop->getter == js_GetCallVar, at /home/dvander/tracemonkey/js/src/jstracer.cpp:2372

Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (s=0xb7f11a48 "sprop->getter == js_GetCallVar", file=0xb7f10f10 "/home/dvander/tracemonkey/js/src/jstracer.cpp", ln=2372)
    at /home/dvander/tracemonkey/js/src/jsutil.cpp:63
63          abort();
(gdb) bt
#0  JS_Assert (s=0xb7f11a48 "sprop->getter == js_GetCallVar", file=0xb7f10f10 "/home/dvander/tracemonkey/js/src/jstracer.cpp", ln=2372)
    at /home/dvander/tracemonkey/js/src/jsutil.cpp:63
#1  0xb7e9f60b in TraceRecorder::activeCallOrGlobalSlot (this=0xaedae590, obj=0xaedcc760, vp=@0xbfffd2b4) at /home/dvander/tracemonkey/js/src/jstracer.cpp:2372
#2  0xb7ea4333 in TraceRecorder::name (this=0xaedae590, vp=@0xbfffd2b4) at /home/dvander/tracemonkey/js/src/jstracer.cpp:4284
#3  0xb7eabaca in TraceRecorder::record_JSOP_FORNAME (this=0xaedae590) at /home/dvander/tracemonkey/js/src/jstracer.cpp:4816

(gdb) print *sprop
$1 = {id = -1238133892, getter = 0, setter = 0, slot = 9, attrs = 5 '\005', flags = 0 '\0', shortid = 0, parent = 0x0, kids = 0xb04b5b70, shape = 23064}

(gdb) print cx->fp->script->filename
$2 = 0xaed16171 "http://meta.wikimedia.org/w/index.php?title=MediaWiki:Wikiminiatlas.js&action=raw&ctype=text/javascript&smaxage=21600&maxage=86400"

(gdb) print JS_PCToLineNumber(cx, cx->fp->script, cx->fp->regs->pc)
$4 = 239

Comment 1

[Brendan Eich [:brendan]]

Good old with (wikiminiatlas) around for (var key in ...) means key gets hoisted above the with, so for-in can't use JSOP_FORVAR, but must use JSOP_FORNAME, which binds a named property of the Call object induced by the with statement. Blech!

Easy to fix.

/be

Comment 2

[Brendan Eich [:brendan]]

Fixed on tracemonkey:

http://hg.mozilla.org/tracemonkey/index.cgi/rev/e22c536061e4

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix

This will conflict with trace-test.js on tracemonkey, sigh.

I also followed Andreas's lead there and used four-space indentation units in trace-test.js. It's what vim does by default.

There's a bug I'll file hiding here: even in a with, where for-in using a local must select JSOP_FORNAME, we should be able to avoid creating an expando on the Call object. I need to debug a bit and file that, but this should go in anyway. You can use eval("var foo = ...") in a function to make an expando on the Call object, and that would botch the assertion hard.

/be

Comment 4

[Andreas Gal :gal]

If you prefer 8-space indentation just re-format the whole file as one commit. I think my eclipse does default 4 on .js but I am pretty sure I can convince it to do 8.

Comment 5

[Brendan Eich [:brendan]]

Shaver used two-space indentation units in trace-test.js and we mostly stuck to that. 8 spaces (or tabs) is too much, I left that style school when I retired from pre-Linux kernel hacking ;-). 4 is consistent with the rest of the code.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 335279 [details] [diff] [review]
fix

No mystery, with makes the function heavyweight, and that means locals are plain old properties. Followup bug fodder, probably in the upvar followup I need to clone and mutate.

/be

Comment 8

[Brendan Eich [:brendan]]

Fixed on mozilla-central:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/e49ce187bae5

/be

Comment 9

[Bob Clary [:bc] (inactive)]

test also included in js1_8_1/trace/trace-test.js