593256 - Bugs in dictionary-mode property table maintenance

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

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

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

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

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: rename freeslot (the method of JSObject, the data member of JSObjectMap) to slotSpan

[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 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 471744 [details] [diff] [review]
proposed fix

Needs tests. r=me with that.

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Brendan Eich [:brendan]]

Attachment: refreshed proposed fix

Comment 6

[Brendan Eich [:brendan]]

Attachment: rename freeslot (both of them; to different names)

This should be just cosmetic -- name changes only.

/be

Comment 7

[Brendan Eich [:brendan]]

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

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: rebased on tm tip and latest bug 592556 patch

Comment 9

[Brendan Eich [:brendan]]

Attachment: rename freeslot (both of them; to different names)

Comment 10

[Brendan Eich [:brendan]]

Attachment: rebased on tm tip + bug 592556 patch, plus monotonic check

Comment 11

[Brendan Eich [:brendan]]

Attachment: rename freeslot (both of them; to different names)

Comment 12

[Brendan Eich [:brendan]]

Attachment: rebased fix patch, without renamings

Comment 13

[Brendan Eich [:brendan]]

Attachment: rename freeslot (both of them; to different names)

Comment 14

[Brendan Eich [:brendan]]

Attachment: renaming patch, rebased on top of fixed patch for 592556

Comment 15

[Jason Orendorff [:jorendorff]]

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 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 473579 [details] [diff] [review]
renaming patch, rebased on top of fixed patch for 592556

Straightforward. r=me!

Comment 17

[Brendan Eich [:brendan]]

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

/be

Comment 20

[Brendan Eich [:brendan]]

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

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[Brendan Eich [:brendan]]

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

Comment 23

[Brendan Eich [:brendan]]

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

Filed bug 595555.

/be

Comment 24

[ojab]

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

Comment 25

[Brendan Eich [:brendan]]

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

Comment 26

[Gary [:streetwolf52]]

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

Comment 27

[Brendan Eich [:brendan]]

(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