Closed Bug 585891 Opened 11 years ago Closed 10 years ago

ES5 Object.create misbehaves in particular scenario

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: irakli, Assigned: brendan)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
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
Assignee: general → jwalden+bmo
Blocks: es5
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.
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
blocking2.0: --- → beta5+
beta5 blocker.
(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.
(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.
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
blocking2.0: beta5+ → beta6+
No longer blocks: 584707
Assignee: jwalden+bmo → jorendorff
blocking2.0: beta6+ → betaN+
Assignee: jorendorff → jwalden+bmo
Latest patch in bug 552432 adds a JSPROP_SHADOWABLE which should help here.

/be
(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
Attached patch so simple it must be right! (obsolete) — Splinter Review
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
Attachment #497737 - Flags: review?(jwalden+bmo)
OS: Mac OS X → All
Hardware: x86 → All
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.
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.
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.
(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
> 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
Assignee: jwalden+bmo → brendan
Attachment #497737 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #499024 - Flags: review?(jwalden+bmo)
Attachment #497737 - Flags: review?(jwalden+bmo)
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.
Attachment #499024 - Flags: review?(jwalden+bmo) → review+
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
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/89ec0c0b48e2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.