Found by consistency checker I added on top of the patch for bug 592556. Patch coming soon. /be

Plus the #ifdef DEBUG_notme (expensive, kills some tests) consistency checking, in jsscope.cpp, JSObject::checkShapeConsistency. /be

[10:43am] brendan: yeah, just about to attach a separate renaming patch, for sanity [10:43am] jorendorff: great [10:43am] brendan: s/freeslot/slotSpan/ almost everywhere [10:43am] brendan: JSObject::freeSlot method vs. JSObject::freeslot member (going away, but still) -- too close [10:44am] brendan: (moving to JSObjectMap, actually) [10:44am] brendan: i was dreaming about better names, came up with slotSpan [10:44am] jorendorff: hmm... Is that really better? [10:45am] brendan: i think so, for that reason and another [10:45am] brendan: PropertyTable::freeslot is SHAPE_INVALID_SLOT or the index of the last-freed slot in the index-threaded freelist linked among slot values [10:45am] brendan: using PrivateUint32 encoded values [10:45am] brendan: oh, that's two reasons [10:46am] brendan: 1. avoid yet another name collision [10:46am] brendan: 2. the freelist means JSObject::freeslot (or JSObjectMap::freeslot in the patch for bug 593256) is not the first free slot [10:46am] brendan: it's the span (distance, to fencepost not last allocated) covering all live slots and some freelist-threaded free ones [10:47am] jorendorff: that makes sense [10:48am] brendan: so three reasons (two kinda the same but not quite) /be

Comment on attachment 471744 [details] [diff] [review] proposed fix Needs tests. r=me with that.

(In reply to comment #3) > Comment on attachment 471744 [details] [diff] [review] > proposed fix > > Needs tests. r=me with that. How, though? Perf-test would have to be pretty sensitive to notice the loss of a table. The existing tests do catch the bugs with CHECK_SHAPE_CONSISTENCY enabled. Jason also mentioned on IRC the idea of keeping JSObject::checkShapeConsistency #ifdef DEBUG but conditioning it on an envar. I'll do that. /be

This should be just cosmetic -- name changes only. /be

These patches are on top of the patch for bug 592556. /be

Comment on attachment 473352 [details] [diff] [review] rebased fix patch, without renamings This is almost unchanged. Just a few tweaks to the DEBUG-only code, looks like? And those look good. r=me.

Comment on attachment 473579 [details] [diff] [review] renaming patch, rebased on top of fixed patch for 592556 Straightforward. r=me!

http://hg.mozilla.org/tracemonkey/rev/3feb012b18a3 http://hg.mozilla.org/tracemonkey/rev/65a532c7885e /be

Hrm, something about the testcase is not working in the browser jsreftest setting: REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tracemonkey_fedora_test-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_8_5/regress/regress-593256.js | Unknown file:///home/cltbld/talos-slave/tracemonkey_fedora_test-jsreftest/build/jsreftest/tests/js1_8_5/regress/regress-593256.js:7: syntax error item 1 from http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1284238792.1284239256.9704.gz&fulltext=1#err0 bclary, sayrer, anyone: advice? Please disable the test if necessary for the short run. Patches should stick. /be

Naturally the test passes in the shell setting. I even ran under gdb to be sure a syntax error wasn't being swallowed: (gdb) r -f shell.js -f js1_8_5/shell.js -f js1_8_5/regress/shell.js -f ./js1_8_5/regress/regress-593256.js Starting program: /Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey/js/src/Darwin_DBG.OBJ/js -f shell.js -f js1_8_5/shell.js -f js1_8_5/regress/shell.js -f ./js1_8_5/regress/regress-593256.js Reading symbols for shared libraries .++++..................................................................................... done PASSED! don't crash Program exited normally. Could be some quoting or line-ending issue? How are jsreftests embedded in HTML script tags? /be

Aha, JSOPTION_ANONFUNFIX is set in browser but not shell. The testcase evals anonymous function expressions in expression-statement position. I'll file a bug on setting this option in the shell, but it'll require testcase changes and fuzzer adjustments. /be

(In reply to comment #22) > I'll file a bug on setting this option in the shell, .... Filed bug 595555. /be

After these changes I have build failure: https://bugzilla.mozilla.org/show_bug.cgi?id=595615

http://hg.mozilla.org/mozilla-central/rev/3feb012b18a3 http://hg.mozilla.org/mozilla-central/rev/65a532c7885e (rename freeslot patch) http://hg.mozilla.org/mozilla-central/rev/0b33419e048d (workaround bug 595555) /be

Please have a look at this https://bugzilla.mozilla.org/show_bug.cgi?id=595693

(In reply to comment #26) > Please have a look at this https://bugzilla.mozilla.org/show_bug.cgi?id=595693 Nothing to do with this bug, but indeed a regression in the Block object patch for bug 592556, fixed by the final patch in that bug (the one aimed successfully at killing a Ts regression). /be