Olov Lassus Reporter
	Description • 14 years ago

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

	Boris Zbarsky [:bzbarsky]
	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).

	Jason Orendorff [:jorendorff]
	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.

	Brendan Eich [:brendan] Assignee
	Comment 3 • 14 years ago

Shoulda seen this one coming a mile away...

/be

	Brendan Eich [:brendan] 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

	Olov Lassus Reporter
	Comment 6 • 14 years ago

That's ok - thanks for fixing!

	Jason Orendorff [:jorendorff]
	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.

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] Assignee
	Comment 10 • 14 years ago

http://hg.mozilla.org/tracemonkey/rev/47729824b12c

/be

	Robert Sayre
	Comment 11 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/47729824b12c