585891 - ES5 Object.create misbehaves in particular scenario

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Evaluation of the code below throws TypeError:

can't redefine non-configurable property 'boom'

Object.create(Object.create({}, { boom: { get: function() { return 'base' }}}), { boom: { get: function() { return 'overridden' }}}).boom

It should not throw exception since property 'boom' is not defined on an object itself, it is inherited.

Comment 1

[Brendan Eich [:brendan]]

Thanks for the bug report.

I should have caught this in review -- the bug hides in the complex interaction of the bad old shared-permanent optimization and the cleaner-as-in-the-spec (but by no means simple) ES5-based code in jsobj.cpp (DefinePropertyOnObject). Have to be careful when obj2 != obj.

/be

Comment 2

[Jeff Walden [:Waldo]]

This is just shared-permanent properties pretending to be own plus Object.defineProperty choosing to conform to the current Object.prototype.hasOwnProperty algorithm: quite well known but perhaps not previously filed.  Unless we can get rid of that post-scopekilling, I don't see this happening for Firefox 4, unfortunately.

Comment 3

[Brendan Eich [:brendan]]

Jeff: this is easily fixed, all you need to do is treat current as undefined when obj != obj2 and current has no shared-permanent status.

/be

Comment 4

[Robert Sayre]

beta5 blocker.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> Jeff: this is easily fixed, all you need to do is treat current as undefined
> when obj != obj2 and current has no shared-permanent status.

But current here does have shared-permanent status.  If it didn't, js_HasOwnProperty wouldn't have returned it.  Reformatting the test here for readability:

var proto = Object.create({}, { boom: { get: function() { return 'base' } } });
var obj =
  Object.create(proto,
                { boom: { get: function() { return 'overridden'; } } });

proto is an object with [[Prototype]] as a new object, with the single own property "boom" having the descriptor { [[Get]]: fun1, [[Set]]: undefined, [[Enumerable]]: false, [[Configurable]]: false }.  Because it is an accessor descriptor we also throw in JSPROP_SHARED so as not to associate a slot with it.  Therefore it's JSPROP_READONLY | JSPROP_PERMANENT | JSPROP_SHARED, and the shared-permanent-implies-own interaction applies to it.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> But current here does have shared-permanent status.  If it didn't,
> js_HasOwnProperty wouldn't have returned it.

"if it didn't *and obj != obj2*", sorry.

Comment 7

[Brendan Eich [:brendan]]

Yes, it's time to stop using JSPROP_SHARED for accessors. See bug 552432 for some discussion. Without JSPROP_SHARED, comment 3 should work.

/be

Comment 8

[Brendan Eich [:brendan]]

Latest patch in bug 552432 adds a JSPROP_SHADOWABLE which should help here.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Latest patch in bug 552432 adds a JSPROP_SHADOWABLE which should help here.

Or not. Don't really want too many meanings for that flag. Indeed the only issue here is JSPROP_SHARED is overloaded. On Object.prototype.__proto__, the sole __ SHARED property remaining, it means "make __proto__ appear to be 'own' in all objects delegating to Object.prototype". This is detectable by

js> p = {q:1};
[object Object]
js> o = {__proto__: p};
[object Object]
js> print(o.q);          // 1
1
js> o.__proto__ = null;
null
js> o.__proto__ = p;
[object Object]
js> print(o.q);          // oops, undefined
undefined
js> o
can't convert Object to string

Anyway, for all other properties, SHARED means "appear to be own in delgating objects *of the same class*." In general the "own" fiction can't cross class boundaries and it should not. 

In that light a straightforward patch comes to mind. Coming up.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: so simple it must be right!

Waldo, can you poke a hole in this, ideally with tests? I can't take the bug but if this helps, please run it into the end zone for victory.

I thought about checking for id != '__proto__' or obj2 not Object.prototype but it's hard to check for the latter and it doesn't seem worth it just to deal with a case where someone nulls __proto__ and then uses Object.defineProperty to make a new and different '__proto__'. The FIXMEs point the way, and we don't need to make this perfect in the face of __proto__ nulling.

/be

Comment 11

[Jeff Walden [:Waldo]]

I think this might be a reasonable stopgap, and I don't see obvious holes.  It is still a stopgap, and it deliberately introduces an inconsistency between appearance of an own property and ability to perform actions which should be permitted when no own properties exist.  But it might be the right path forward for the immediate future.

I'm thinking it might also be worth propagating this into Object.getOwnPropertyDescriptor as well -- will take a look at that before moving forward here.

Comment 12

[Jason Orendorff [:jorendorff]]

Ideally, I think we should continue to get it wrong as consistently as possible. That is, I want to avoid hasOwnProperty and getOwnPropertyDescriptor and defineProperty appearing to have different notions of what own properties are.

That suggests either (a) modifying js_HasOwnProperty, rather than narrowly DefinePropertyOnObject, to respect this slimmer version of the shared permanent hack; or (b) a different approach, ignoring the shared permanent hack entirely in Object.create, but not from any of the other methods (e.g. by adding a bool wantSharedPermanentHack parameter to DefinePropertyOnObject).

Post-ff4, Waldo or I will run, not walk, to fix bug 575997.

Comment 13

[Jason Orendorff [:jorendorff]]

I'm in favor of option (b) for now; I think whatever we do will leave some corner ES5 bugs until 575997 can be fixed.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (b) a different approach, ignoring the shared permanent hack entirely
> in Object.create, but not from any of the other methods

Sounds like comment 3. The patch gets simpler:

diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -1998,16 +1998,33 @@ DefinePropertyOnObject(JSContext *cx, JS
     JSProperty *current;
     JSObject *obj2;
     JS_ASSERT(!obj->getOps()->lookupProperty);
     if (!js_HasOwnProperty(cx, NULL, obj, desc.id, &obj2, &current))
         return JS_FALSE;
 
     JS_ASSERT(!obj->getOps()->defineProperty);
 
+    /*
+     * If we find a shared permanent property in a different object obj2 from
+     * obj, then if the property is shared permanent (an old hack to optimize
+     * per-object properties into one prototype property), ignore that lookup
+     * result (null current).
+     *
+     * FIXME: bug 575997 (see also bug 607863).
+     */
+    if (current && obj2 != obj && obj2->isNative()) {
+        /* See same assertion with comment further below. */
+        JS_ASSERT(obj2->getClass() == obj->getClass());
+
+        Shape *shape = (Shape *) current;
+        if (shape->isSharedPermanent())
+            current = NULL;
+    }
+
     /* 8.12.9 steps 2-4. */
     if (!current) {
         if (!obj->isExtensible())
             return Reject(cx, obj, JSMSG_OBJECT_NOT_EXTENSIBLE, throwError, rval);
 
         *rval = true;
 
         if (desc.isGenericDescriptor() || desc.isDataDescriptor()) {

This passes ecma_5 and jit tests. It allows __proto__ to be shadowed.

> (e.g. by adding a bool wantSharedPermanentHack parameter to
> DefinePropertyOnObject).

I don't see that helping. Whether the caller uses Object.defineProperty or Object.create, if the prototype object has the same class as the direct object (which for Object.create is by definition Object), then we want the same rules for shadowing.

For reconfiguring, there is no issue since the shared permanent properties are non-configurable and a direct hit (not proto-prop) will respect that.

/be

Comment 15

[Brendan Eich [:brendan]]

> This passes ecma_5 and jit tests. It allows __proto__ to be shadowed.

Shadowed via Object.defineProperty, not via assignment of course. See bug 552432.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: even simpler, per comments

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 499024 [details] [diff] [review]
even simpler, per comments

Hacks, hacks, hacks.  But for a few months, the ES5-using web can probably deal.

Please include some tests with this.  I don't overly care how comprehensive they are (or at least sight-unseen I don't care -- put them in front of me for review and I may well ask for improvements ;-) ), just make sure at least some of them fail pre-patch and pass post-patch.

Comment 18

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/89ec0c0b48e2

I added the test from comment 0 -- hope it's ok to add to the section-numbered test for Object.create. I'm gonna leave further testing (or better, JSPROP_SHARED removal) to you guys.

/be

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/89ec0c0b48e2