Bugs in dictionary-mode property table maintenance

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
JavaScript Engine
P1
major
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla2.0b7
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 9 obsolete attachments)

11.02 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
32.90 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

/be
(Assignee)

Comment 1

7 years ago
Created attachment 471744 [details] [diff] [review]
proposed fix

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

/be
Attachment #471744 - Flags: review?(jorendorff)
(Assignee)

Comment 2

7 years ago
Created attachment 471915 [details] [diff] [review]
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
Attachment #471915 - Flags: review?(jorendorff)
Comment on attachment 471744 [details] [diff] [review]
proposed fix

Needs tests. r=me with that.
Attachment #471744 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 4

7 years ago
(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
(Assignee)

Comment 5

7 years ago
Created attachment 472702 [details] [diff] [review]
refreshed proposed fix
Attachment #471744 - Attachment is obsolete: true
Attachment #472702 - Flags: review?(jorendorff)
(Assignee)

Comment 6

7 years ago
Created attachment 472705 [details] [diff] [review]
rename freeslot (both of them; to different names)

This should be just cosmetic -- name changes only.

/be
Attachment #471915 - Attachment is obsolete: true
Attachment #472705 - Flags: review?(jorendorff)
Attachment #471915 - Flags: review?(jorendorff)
(Assignee)

Comment 7

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

/be
(Assignee)

Comment 8

7 years ago
Created attachment 472925 [details] [diff] [review]
rebased on tm tip and latest bug 592556 patch
Attachment #472702 - Attachment is obsolete: true
Attachment #472925 - Flags: review?(jorendorff)
Attachment #472702 - Flags: review?(jorendorff)
(Assignee)

Comment 9

7 years ago
Created attachment 472926 [details] [diff] [review]
rename freeslot (both of them; to different names)
Attachment #472705 - Attachment is obsolete: true
Attachment #472926 - Flags: review?(jorendorff)
Attachment #472705 - Flags: review?(jorendorff)
(Assignee)

Comment 10

7 years ago
Created attachment 472929 [details] [diff] [review]
rebased on tm tip + bug 592556 patch, plus monotonic check
Attachment #472925 - Attachment is obsolete: true
Attachment #472929 - Flags: review?(jorendorff)
Attachment #472925 - Flags: review?(jorendorff)
(Assignee)

Comment 11

7 years ago
Created attachment 472930 [details] [diff] [review]
rename freeslot (both of them; to different names)
Attachment #472926 - Attachment is obsolete: true
Attachment #472930 - Flags: review?(jorendorff)
Attachment #472926 - Flags: review?(jorendorff)
(Assignee)

Comment 12

7 years ago
Created attachment 473352 [details] [diff] [review]
rebased fix patch, without renamings
Attachment #472929 - Attachment is obsolete: true
Attachment #473352 - Flags: review?(jorendorff)
Attachment #472929 - Flags: review?(jorendorff)
(Assignee)

Comment 13

7 years ago
Created attachment 473353 [details] [diff] [review]
rename freeslot (both of them; to different names)
Attachment #472930 - Attachment is obsolete: true
Attachment #473353 - Flags: review?(jorendorff)
Attachment #472930 - Flags: review?(jorendorff)
(Assignee)

Comment 14

7 years ago
Created attachment 473579 [details] [diff] [review]
renaming patch, rebased on top of fixed patch for 592556
Attachment #473353 - Attachment is obsolete: true
Attachment #473579 - Flags: review?(jorendorff)
Attachment #473353 - Flags: review?(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.
Attachment #473352 - Flags: review?(jorendorff) → review+
Comment on attachment 473579 [details] [diff] [review]
renaming patch, rebased on top of fixed patch for 592556

Straightforward. r=me!
Attachment #473579 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 17

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

/be
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

7 years ago
Duplicate of this bug: 595365
(Assignee)

Updated

7 years ago
Duplicate of this bug: 592214
(Assignee)

Comment 20

7 years ago
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
(Assignee)

Comment 21

7 years ago
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
(Assignee)

Comment 22

7 years ago
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
(Assignee)

Comment 23

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

Filed bug 595555.

/be

Updated

7 years ago
blocking2.0: --- → final+

Comment 24

7 years ago
After these changes I have build failure: https://bugzilla.mozilla.org/show_bug.cgi?id=595615
(Assignee)

Comment 25

7 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 26

7 years ago
Please have a look at this https://bugzilla.mozilla.org/show_bug.cgi?id=595693
(Assignee)

Comment 27

7 years ago
(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

Updated

7 years ago
Depends on: 595846
(Assignee)

Updated

7 years ago
No longer depends on: 595846
You need to log in before you can comment on or make changes to this bug.