Bug 34639 – Object.defineProperty accepts wrong descriptor

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Rico]

The JavaScript:
Object.defineProperty({}, 'foo', {get:undefined, value:42})
According to spec ToPropertyDescriptor (8.10.5),
which is called from Object.defineProperty (15.2.3.6) with argument
{get:undefined, value:true} should in this case throw an exception: 
step 5 will set desc.value to true
step 6 will set desc.get to undefined
step 9 should in this case throw an exception since desc.get is present (although undefined)

Attached js file should produce:

Yes

It produces:

No

This bug was experienced using JSC 54131 from 2010-02-01

[Rico]

Created attachment 48215 [details]
Attachment failed on original post

[Xan Lopez]

The problem is that as far as the code is concerned there is no difference between not having been set a Getter and having an undefined Getter. If we want to be more subtle we'd need to check the attribute flags of the descriptor, which are modified when a Getter is set. Something like:


diff --git a/JavaScriptCore/runtime/ObjectConstructor.cpp b/JavaScriptCore/runtime/ObjectConstructo$
index b1f9d70..e47a4e6 100644
--- a/JavaScriptCore/runtime/ObjectConstructor.cpp
+++ b/JavaScriptCore/runtime/ObjectConstructor.cpp
@@ -229,7 +229,7 @@ static bool toPropertyDescriptor(ExecState* exec, JSValue in, PropertyDescriptor
         desc.setSetter(set);
     }

-    if (!desc.isAccessorDescriptor())
+    if (!desc.attributes() & Getter || !desc.attributes() & Setter)
         return true;

     if (desc.value()) {

That makes the testcase print 'Yes' here.

[Xan Lopez]

We want to check if both are not present, actually:


diff --git a/JavaScriptCore/runtime/ObjectConstructor.cpp b/JavaScriptCore/runtime/ObjectConstructor.cpp
index b1f9d70..3537576 100644
--- a/JavaScriptCore/runtime/ObjectConstructor.cpp
+++ b/JavaScriptCore/runtime/ObjectConstructor.cpp
@@ -229,7 +229,7 @@ static bool toPropertyDescriptor(ExecState* exec, JSValue in, PropertyDescriptor
         desc.setSetter(set);
     }

-    if (!desc.isAccessorDescriptor())
+    if (!(desc.attributes() & Getter || desc.attributes() & Setter))
         return true;

     if (desc.value()) {

[Xan Lopez]

Created attachment 65685 [details]
objectdescriptor.diff

OK, so I think this change might make sense to be done at this level instead of hardcoding it in toPropertyDescriptor. It changes the result of two tests; one is essentially the one the reporter submitted, and the other just makes getOwnProperty descriptor be undefined if the getter is undefined. With this we match Chrome in both behaviors.

[Darin Adler]

Comment on attachment 65685 [details]
objectdescriptor.diff

Seems OK. Would be nice if the JavaScript standard was 100% clear on this. I don’t want “matches V8” to be a substitute for “matches the specification”.

r=me

[Oliver Hunt]

I'm hesitant to change this behaviour -- as the spec seems to imply that we should simply end up with an undefined getter, you shouldn't end up with the property not being present.

I'll email es-discuss to determine what the correct behaviour should be.  I don't think the behaviour introduced by this patch is correct.

[Oliver Hunt]

Comment on attachment 65685 [details]
objectdescriptor.diff

r-'ing this pending a response on es-discuss as to what correct behaviour should be.

[Rico]

Hi,

Any updates on this?

The thread you started on es-discuss was actually discussing another issue (what the outcome should be of): 
Object.defineProperty({}, 'foo', {get:undefined})
Which is also very relevant since this is also a place where the Chrome implementation differs from Safari (Safari will make this a data property).

The original suggestion/question is still valid, since the spec is pretty clear on what should happen when ToPropertyDescriptor is called from Object.defineProperty with the following:
Object.defineProperty({}, 'foo', {get:undefined, value:42})

Step 9a says that in the case where Get or Set is present (but potentially undefined as in our case) and either writable or value is also present we should throw an error (and not define any property at all).

Cheers,
Rico

[Gavin Barraclough]

This is fixed in ToT