Bug 28724 – [ES5] Implement getOwnPropertyDescriptor

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Oliver Hunt]

ES5 defines getOwnPropertyDescriptor to determine the values and attributes of a property.  In addition to being part of ES5, we need this API so that we can test other APIs in ES5

[Oliver Hunt]

Created attachment 38578 [details]
JavaScriptCore portion of patch

[Oliver Hunt]

Created attachment 38579 [details]
Testcase

[Oliver Hunt]

Created attachment 38580 [details]
WebCore portion

[Geoffrey Garen]

I guess I could read up on this myself, but what is the distinction between a "property descriptor" and property attributes? In other words, why can't we just piggy back on getPropertyAttributes?

[Oliver Hunt]

getOwnPropertySlot is insufficient for getPropertyDescriptor as getOwnPropertySlot doesn't provide the information needed for getters and setters (and QuarantinedObjectWrapper needs to do special work regardless).

[Gavin Barraclough]

Comment on attachment 38579 [details]
Testcase

r-.
Please to be removing __defineGetter__ from the JavaScript language.
Okay r+, but I do not approve.

[Gavin Barraclough]

Comment on attachment 38580 [details]
WebCore portion

"This requires a custom implementation of getOwnPropertyDescriptor for every class with a custom implementation of getOwnPropertyDescriptor."

Well, none do right now ... so, nothing to be added then? ;-)
I guess you mean "with a custom implementation of getOwnPropertySlot"?

 87     if (JSGlobalObject::getOwnPropertyDescriptor(exec, propertyName, descriptor))
 88         return true;
 89     return false;

Um, isn't that  "return JSGlobalObject::getOwnPropertyDescriptor(exec, propertyName, descriptor);" ?

Could you rewrite the CodeGeneratorJS.pm script in a language other then Perl please? - perl sucks.

So, I share Geoff's concern that this duplicates all the getOwnPropertySlot methods.  I understand that they could not be used to implement getOwnPropertyDescriptor, since there is a loss of information re getters (they have already been resolved), but I wonder if we could structure it the other way around – only implement the full complex logic for getOwnPropertyDescriptor, and make gopSlot call this.  If the slot returned by getOwnPropertyDescriptor describes a getter, then gopSlot could invoke it.  Please cogitate and ruminate on this suggestion, and if you can't make it better, r+.

[Gavin Barraclough]

Comment on attachment 38578 [details]
JavaScriptCore portion of patch

> +bool JSVariableObject::symbolTableGet(const Identifier& propertyName, PropertyDescriptor& descriptor)

I don't really like this name.  I expect get to just get values, whereas this gets a description.  It seems that all other methods that retrieve PropertyDescriptors have 'Descriptor' in their name, so it seems like this method should too.

>      /**
> +     * Simplified version of getStaticPropertyDescriptor in case there are no functions, only "values".
> +     * Using this instead of getStaticPropertyDescriptor removes the need for a FuncImp class.
> +     */

The phrase "in case there are no functions" reads oddly to me (and with a different meaning than I think you intend).  I would suggest that "in cases where there are no functions" reads very differently to me (and with a meaning closer to what I think you want).  The same goes for the comment on partner method for only functions.

>  ObjectConstructor::ObjectConstructor(ExecState* exec, PassRefPtr<Structure> structure, ObjectPrototype* objectPrototype, Structure* prototypeFunctionStructure)
> -    : InternalFunction(&exec->globalData(), structure, Identifier(exec, "Object"))
> +: InternalFunction(&exec->globalData(), structure, Identifier(exec, "Object"))
>  {
>      // ECMA 15.2.3.1
>      putDirectWithoutTransition(exec->propertyNames().prototype, objectPrototype, DontEnum | DontDelete | ReadOnly);
> -
> +    
>      // no. of arguments for constructor
>      putDirectWithoutTransition(exec->propertyNames().length, jsNumber(exec, 1), ReadOnly | DontEnum | DontDelete);
> -
> +    
>      putDirectFunctionWithoutTransition(exec, new (exec) NativeFunctionWrapper(exec, prototypeFunctionStructure, 1, exec->propertyNames().getPrototypeOf, objectConstructorGetPrototypeOf), DontEnum);
> +    putDirectFunctionWithoutTransition(exec, new (exec) NativeFunctionWrapper(exec, prototypeFunctionStructure, 2, exec->propertyNames().getOwnPropertyDescriptor, objectConstructorGetOwnPropertyDescriptor), DontEnum);
>  }

3x garbled whitespace.

[Oliver Hunt]

Landed r47780

[Oliver Hunt]

Followups in r47781 and r47782

[Geoffrey Garen]

(In reply to comment #5)
> getOwnPropertySlot is insufficient for getPropertyDescriptor as
> getOwnPropertySlot doesn't provide the information needed for getters and
> setters (and QuarantinedObjectWrapper needs to do special work regardless).

I didn't ask about getOwnPropertySlot. I asked about getPropertyAttributes.

I really don't like the approach of copying all the getOwnPropertySlot functions. Duplicate code is error prone.

[Mark S. Miller]

Since the changeset that closed this out says:

> ES5 is vague
> about how getOwnPropertyDescriptor should work in the context of
> "host" functions with polymorphic 'GetOwnProperty', so it seem okay that 
> occasionally we "guess" what attributes -- eg. determining
> whether a property is writable.

it is worth pointing out that, while ES5 gives some freedom, it also imposes some real constraints, especially on the writable and configurable attributes. The note at the end of 8.6.2 on page 34 of http://wiki.ecmascript.org/lib/exe/fetch.php?id=es3.1%3Aes3.1_proposal_working_draft&cache=cache&media=es3.1:es5-final-8-22.pdf says:

-------------------
The [[GetOwnProperty]] internal method of a host object must conform to the following invariants for each property of the host object:

    * If a property is described as a data property and it may return different values over time, then either or both of the [[Writable]] and [[Configurable] attributes must be true even if no mechanism to change the value is exposed via the other internal methods.
    * If a property is described as a data property and its [[Writable]] and [[Configurable]] are both false, then the SameValue (according to 9.12) must be returned for the [[Value]] attribute of the property on all calls to [[GetOwnProperty]].
    * If the attributes other than [[Writable]] may change over time or if the property might disappear, then the [[Configurable]] attribute must be true.
    * If the [[Writable]] attribute may change from false to true, then the [[Configurable]] attribute must be true.
    * If the value of the host object‘s [[Extensible]] internal property is has been observed by ECMAScript code to be false, then if a call to [[GetOwnProperty]] describes a property as non-existent all subsequent calls must also describe that property as non-existent.

The [[DefineOwnProperty]] internal method of a host object must not permit the addition of a new property to a host object if the [[Extensible]] internal property of that host object has been observed by ECMAScript code to be false.
    If the [[Extensible]] internal property of that host object has been observed by ECMAScript code to be false then it must not subsequently become true.
-------------------


If the changeset in question does not conform to these constraints, I ask that this bug be reopened.

[Oliver Hunt]

(In reply to comment #12)
> Since the changeset that closed this out says:
> 
> > ES5 is vague
> > about how getOwnPropertyDescriptor should work in the context of
> > "host" functions with polymorphic 'GetOwnProperty', so it seem okay that 
> > occasionally we "guess" what attributes -- eg. determining
> > whether a property is writable.
> 
> it is worth pointing out that, while ES5 gives some freedom, it also imposes
> some real constraints, especially on the writable and configurable attributes.
> The note at the end of 8.6.2 on page 34 of
> http://wiki.ecmascript.org/lib/exe/fetch.php?id=es3.1%3Aes3.1_proposal_working_draft&cache=cache&media=es3.1:es5-final-8-22.pdf
> says:
> 
> -------------------
> The [[GetOwnProperty]] internal method of a host object must conform to the
> following invariants for each property of the host object:
> 
>     * If a property is described as a data property and it may return different
> values over time, then either or both of the [[Writable]] and [[Configurable]
> attributes must be true even if no mechanism to change the value is exposed via
> the other internal methods.
>     * If a property is described as a data property and its [[Writable]] and
> [[Configurable]] are both false, then the SameValue (according to 9.12) must be
> returned for the [[Value]] attribute of the property on all calls to
> [[GetOwnProperty]].
>     * If the attributes other than [[Writable]] may change over time or if the
> property might disappear, then the [[Configurable]] attribute must be true.
>     * If the [[Writable]] attribute may change from false to true, then the
> [[Configurable]] attribute must be true.
>     * If the value of the host object‘s [[Extensible]] internal property is has
> been observed by ECMAScript code to be false, then if a call to
> [[GetOwnProperty]] describes a property as non-existent all subsequent calls
> must also describe that property as non-existent.

The reality is that it is highly likely that this is something that we can guarantee will be true in all cases, more over we cannot even be sure that the behaviour (on the DOM) will match anyone elses in future  -- getOwnPropertyDescriptor is going to be a world of pain for things like the DOM as basically it exposes the underlying implementation in a way that should never have been allowed. eg. nothing in any spec defines how style.offsetLeft is implemented, but getOwnPropertyDescriptor will expose whether it's implemented as a value (what we do) or as getter/setter (as firefox _likely_) does.  It gets better -- due to security constraints accessing proeprties of the same object from a different origin may give you wholly different values.  Weee!

[Mark S. Miller]

Regarding the ES5 constraints quoted in comment #12 and the reply in #13 
  > The reality is that it is highly likely that this is something that we 
  > can guarantee will be true in all cases...
you can always trivially meet the stated requirements: For these host objects, always say that 'writable', 'configurable', and [[Extensible]] are true. This does not mean that these properties/objects can necessarily be written, configured, or extended. It just avoids making a promise that they can't. Any of these as false does make such a promise. Don't make promises you can't keep.

Of course the remaining issue is a quality of implementation issue. If you do know that a property's value and attributes will never change, it is more useful but not required to say {writable: false, configurable: false}. Likewise if you know an object will never be extended it is more useful for [[Extensible]] to be false.


> expose whether it's implemented as a value [..] or as getter/setter

Within these constraints the spec does allow getter/setter implementations to describe themselves via getOwnPropertyDescriptor as a data/value property, and vice versa. The joining of non-strict 'arguments' to parameter variables is specified in terms of getter/setters. But the behavior of getOwnPropertyDescriptor on non-strict arguments is specified to "lie" about this and explain these properties as if they were data properties. Host objects are free to use the same subterfuge.

[Oliver Hunt]

(In reply to comment #14)
>  Likewise if you know an object will never be extended it is more useful for
> [[Extensible]] to be false.
[[Extensible]] will not be an issue as that is a property of the object not of an individual property

> 
> 
> > expose whether it's implemented as a value [..] or as getter/setter
> 
> Within these constraints the spec does allow getter/setter implementations to
> describe themselves via getOwnPropertyDescriptor as a data/value property, and
> vice versa. The joining of non-strict 'arguments' to parameter variables is
> specified in terms of getter/setters. But the behavior of
> getOwnPropertyDescriptor on non-strict arguments is specified to "lie" about
> this and explain these properties as if they were data properties. Host objects
> are free to use the same subterfuge.
Arguments is defined in strict mode as having getter/setters so that has to be exposed -- the specific issue is the DOM which is not defined by ECMA, and the mechanism of implementation of a DOM lookup is completely undefined.