Closed Bug 601399 Opened 9 years ago Closed 9 years ago

Property enumeration order is altered after a property has been read

Categories

(Core :: JavaScript Engine, defect, P2)

defect

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)

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
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).
Blocks: 471214
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Keywords: regression
blocking2.0: ? → betaN+
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.
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
Attached patch proposed fix (obsolete) — Splinter Review
Attachment #486259 - Flags: review?(jorendorff)
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
Target Milestone: mozilla2.0 → mozilla2.0b8
That's ok - thanks for fixing!
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+
(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
Attached patch patch to commitSplinter Review
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+
http://hg.mozilla.org/tracemonkey/rev/47729824b12c

/be
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/47729824b12c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 626596
You need to log in before you can comment on or make changes to this bug.