582967 - Harmony proxies: Object.getOwnPropertyDescriptor(proxy,name) cannot return undefined

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Tom Van Cutsem]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; nl-nl) AppleWebKit/533.16 (KHTML, like Gecko) Version/5.0 Safari/533.16
Build Identifier: Tracemonkey (hg.mozilla.org/tracemonkey tip 199d64731816+ July 26, 2010)

When calling Object.getOwnPropertyDescriptor(proxy, name) on a trapping proxy, the call fails if the trap returns 'undefined'. However, according to the spec. 'undefined' is a legal return value, and identifies to the caller that a property with the given 'name' does not exist.

This causes the following hg.ecmascript.org proxy tests to fail:
[ err] handlerthis (TypeError: trap getPropertyDescriptor for proxy returned a primitive value)
[ err] illegalargs (TypeError: trap getPropertyDescriptor for proxy returned a primitive value)
[ err] sink (TypeError: trap getPropertyDescriptor for sink returned a primitive value)

There are actually two bugs here:
1) the error message is wrong: it should state 'getOwnPropertyDescriptor' ('getPropertyDescriptor' is never triggered, since Object.getPropertyDescriptor is not even defined)

2) in all cases, the getOwnPropertyDescriptor trap returns undefined, which is a legitimate return value (it indicates that the property does not exist)

Reproducible: Always

Steps to Reproduce:
Execute the following script:

var p = Proxy.create({
  getOwnPropertyDescriptor: function(name) { return undefined; }
});

var result = Object.getOwnPropertyDescriptor(p, 'foo');
Actual Results:  
TypeError: trap getPropertyDescriptor for p returned a primitive value

Expected Results:  
The call to getOwnPropertyDescriptor should return undefined.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 479490 [details] [diff] [review]
patch

>+           ((tvr.value().isUndefined() && IndicatePropertyNotFound(cx, desc)) ||
>+            ReturnedValueMustNotBePrimitive(cx, proxy, ATOM(getPropertyDescriptor), tvr.value()) &&
>+            ParsePropertyDescriptorObject(cx, proxy, id, tvr.value(), desc));

Lack of parens around the two-line && operand of the || at end of first line will get you a GCC warning -- but: this logic is misstated. You want

>+           (tvr.value().isUndefined()
..            ? IndicatePropertyNotFound(cx, desc)
>+            : ReturnedValueMustNotBePrimitive(cx, proxy, ATOM(getPropertyDescriptor), tvr.value()) &&
>+              ParsePropertyDescriptorObject(cx, proxy, id, tvr.value(), desc));

which avoids extra parens and the (isUndefined() && true) || not-undefined-case-here that should be ?:.

r=me, to relieve jorendorff.

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 479490 [details] [diff] [review]
patch

This code ain't going to win any beauty contests, but r=me.

Comment 4

[Jason Orendorff [:jorendorff]]

Honestly I would have liked to see

    if (v.isUndefined()) {
        desc->obj = NULL;
        return true;
    }
    if (!v.isObject()) {
        blah blah;
        return false;
    }
    return ParsePropertyDescriptorObject(...);

Are these two functions identical except for the atom? If so, common them up.

Also this lacks a test. Do we have a plan to get the ecmascript.org tests into our tree?

Comment 5

[Brendan Eich [:brendan]]

I agree with comment 4. I wrote, but then deleted, a question about whether the expression-language style pays off. Once you add static inline helpers that must return true, and simply store a side effect, I think it does not. So (thanks, Jason), the if/if/return imperative style in comment 4 wins.

/be

Comment 6

[David Bruant]

This bug remains in FF4b10. I'm on Ubuntu 10.10.

Comment 7

[Andreas Gal :gal]

Needs landing.

Comment 8

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/df6e9f32a946

Comment 9

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

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/df6e9f32a946