Closed
Bug 601399
Opened 14 years ago
Closed 14 years ago
Property enumeration order is altered after a property has been read
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: olov.lassus, Assigned: brendan)
References
Details
(Keywords: regression, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/7.0.536.2 Safari/534.10 Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 The following code fails in Firefox 4.0b6 but passes in Firefox 3, Safari and Chrome: function keys(o) { var a = []; for (var key in o) { a.push(key); } return a; } function assert(value, str) { document.write((!value ? "FAIL: " : "PASS: ") + str); } var obj = { 'a': function() {}, 'b': function() {}, 'c': function() {} }; var orig_order = keys(obj).toString(); var tmp = obj["b"]; var read_order = keys(obj).toString(); assert(orig_order === read_order, "property enumeration order should remain the same after a read"); Reproducible: Always
![]() |
||
Comment 1•14 years ago
|
||
Bisect says: The first bad revision is: changeset: 32130:842e6c09e35a user: Brendan Eich <brendan@mozilla.org> date: Thu Sep 03 14:41:19 2009 -0700 summary: Join lambdas assigned or initialized as methods to the compiler-created function object if we can, with a read barrier to clone on method value extractions other than call expressions (471214, r=jorendorff).
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 2•14 years ago
|
||
JSObject::methodShapeChange() calls JSObject::putProperty(), which can remove the existing property and define a new one at the end. We need a non-shuffling putProperty.
Assignee | ||
Comment 3•14 years ago
|
||
Shoulda seen this one coming a mile away... /be
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → mozilla2.0
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #486259 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•14 years ago
|
||
Olov, thanks very much for reporting this and including the test. I've put it in the js/src/tests ("jsreftests") suite with a public domain dedication (creative commons link) at the top. Hope this is ok. /be
Assignee | ||
Updated•14 years ago
|
Target Milestone: mozilla2.0 → mozilla2.0b8
Reporter | ||
Comment 6•14 years ago
|
||
That's ok - thanks for fixing!
Comment 7•14 years ago
|
||
Comment on attachment 486259 [details] [diff] [review] proposed fix In jsscope.cpp, JSObject::putProperty: >+ * We are going to mutate shape in place, so we must regenerate its >+ * shape, and (if shape != lastProp) also lastProp's shape. We will >+ * clearOwnShape() further below after mutating shape. > */ > shape->shape = js_GenerateShape(cx, false); >+ if (shape != lastProp) >+ lastProp->shape = js_GenerateShape(cx, false); Because removeProperty sets ownShape, we don't have to regenerate shape's shape-id. It's a dead letter. (A dead number?) Said another way: If regenerating shape->shape were necessary, it would be equally necessary to regenerate the shape-ids all other shapes from shape to lastProp, right? So just regenerate lastProp->shape unconditionally here, I think. r=me with that.
Attachment #486259 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) > Comment on attachment 486259 [details] [diff] [review] > proposed fix > > In jsscope.cpp, JSObject::putProperty: > >+ * We are going to mutate shape in place, so we must regenerate its > >+ * shape, and (if shape != lastProp) also lastProp's shape. We will > >+ * clearOwnShape() further below after mutating shape. > > */ > > shape->shape = js_GenerateShape(cx, false); > >+ if (shape != lastProp) > >+ lastProp->shape = js_GenerateShape(cx, false); > > Because removeProperty sets ownShape, we don't have to regenerate shape's > shape-id. It's a dead letter. (A dead number?) I was concerned about cache entries depending on shape's number, but you're right, we test only object shapes. Cool! /be
Assignee | ||
Comment 9•14 years ago
|
||
I moved shape generation down till after shape->slot and all the relevant slotSpan members were updated, in case a GC nested due to rt->shapeGen overflow. /be
Attachment #486259 -
Attachment is obsolete: true
Attachment #487496 -
Flags: review+
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/47729824b12c /be
Whiteboard: fixed-in-tracemonkey
Comment 11•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/47729824b12c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•