978238 - ES6 Proxies: [[GetOwnProperty]] is in the weeds

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

Similar to [[DefineProperty]], this was carefully written to a different specification. Also needs a detailed look.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 0: Clean up PropDesc<->PropertyDescriptor Conversions

add PropDesc::populatePropertyDescriptor(), as well as ensuring that undefined PropertyDescritors convert to undefined PropDescs.

Comment 2

[Eric Faust [:efaust]]

Attachment: Re-implement [[GetOwnProperty]]

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8418988 [details] [diff] [review]
Part 0: Clean up PropDesc<->PropertyDescriptor Conversions

Review of attachment 8418988 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/jsobj.cpp
@@ +153,5 @@
>  {
>      MOZ_ASSERT(isUndefined());
>  
> +    if (!desc.object())
> +        return;

...huh.

I'll be happier when we use Maybe for this case, but until then, we can just MOZ_ASSERT(desc.isUndefined()) and MOZ_ASSERT(!desc.isUndefined()) all over the place...

@@ +197,5 @@
> +    desc.setGetter(getter());
> +    desc.setSetter(setter());
> +    desc.setAttributes(attributes());
> +    desc.object().set(obj);
> +}

Oh, great! I just wrote this method, but worse, in a patch over in bug 1008441 that hasn't landed yet.

Comment 4

[Jason Orendorff [:jorendorff]]

(The reason I feel a little funny about that "if (!desc.object()) return;" is that I suspect sometimes in actual use, PropertyDescriptor::object is nullptr-because-we-don't-care-about-that rather than nullptr-because-a-lookup-found-nothing. For example, when defining, maybe? In any case, obviously that isn't happening in places where we convert to PropDesc.)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8418991 [details] [diff] [review]
Re-implement [[GetOwnProperty]]

Review of attachment 8418991 [details] [diff] [review]:
-----------------------------------------------------------------

Are there other tests that should be added here?

::: js/src/jsproxy.cpp
@@ +1582,5 @@
> +            return true;
> +        }
> +
> +        // substep b
> +        if (targetDesc.isPermanent()) {

Please add an isConfigurable() method that returns !isPermanent() and use it here.

Comment 6

[Jason Orendorff [:jorendorff]]

Please land this!

Comment 7

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c25abea181d7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea412568c4be

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes during Linux64 debug build packaging.
https://hg.mozilla.org/integration/mozilla-inbound/rev/51dd4f1ba186

https://tbpl.mozilla.org/php/getParsedLog.php?id=40982075&tree=Mozilla-Inbound

Comment 9

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4e738415f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17f1c024a18d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0d4e738415f6
https://hg.mozilla.org/mozilla-central/rev/17f1c024a18d