565585 - GetPropertyByAttributesById doesn't return value for non-native objects

Status: RESOLVED DUPLICATE of bug 1124935

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

if (ok) {
        if (obj2->isNative()) {
        } else {
            desc->getter = NULL;
            desc->setter = NULL;
            desc->value = JSVAL_VOID;
            ^^^
        }
    }

Why do we do this?

This is observable through the new ES5 reflection API.

Comment 1

[Jeff Walden [:Waldo]]

Your URL doesn't shed much light on the problem for me because nothing specifies the structure of the property descriptor used to represent window.location, so it's not clear to me whether you should see a data descriptor or an accessor descriptor.  It's not immediately clear to me what you consider objectionable about this result from a recent build (in a tab opened to that location):

({value:({}), writable:true, enumerable:true, configurable:false}) == file:///tmp/crash.html

What do you expect to happen instead?

Comment 2

[Andreas Gal :gal]

({value:"file:///tmp/crash.html", writable:true, enumerable:true, configurable:false}) ==
file:///tmp/crash.html

Comment 3

[Jeff Walden [:Waldo]]

window.location isn't a string value.  It's an object.  That object has no enumerable properties (hash/port/href/replace/etc. are all non-enumerable), so it gets source-ified as {}.

Comment 4

[Andreas Gal :gal]

Ah right. Ok **** test case. But can you see what I mean or should I patch this myself? We are not fetching the value for non-native objects but we should, and this will bite anything from e4x to proxy objects.

Comment 5

[Andreas Gal :gal]

Actually I am not sure I can make a scripted test case for this. Your code doesn't even feed into this API. Anyway, still looks like a bug. Patch in a sec.

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Jeff Walden [:Waldo]]

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

With a jsapi-test of this, r=me

Comment 8

[Andreas Gal :gal]

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

I am not sure its the right thing to do actually. Its like triggering a getter, which we don't do in case of a native object with a getter. Blake, what do you think?

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #8)
> which we don't do in case of a native object with a getter.

I think we're kinda-sorta-trying to move away from this.  Object.defineProperty treats such properties as if they were data properties and will call the getter to determine the value the property has (e.g. [].length).  ES descriptors don't conceptually map well onto our full property representation.

Comment 10

[André Bargull [:anba]]

GetPropertyByAttributesById = GetPropertyDescriptorById was removed in https://hg.mozilla.org/mozilla-central/rev/3054048c724b (bug 1124935).

https://hg.mozilla.org/mozilla-central/rev/ff99308cdefc