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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p1])

Attachments

(2 attachments)

Assignee

Description

5 years ago
Similar to [[DefineProperty]], this was carefully written to a different specification. Also needs a detailed look.
Component: JavaScript Engine → JavaScript: Standard Library
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:p1]
Assignee

Comment 1

5 years ago
add PropDesc::populatePropertyDescriptor(), as well as ensuring that undefined PropertyDescritors convert to undefined PropDescs.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8418988 - Flags: review?(jorendorff)
Assignee

Comment 2

5 years ago
Attachment #8418991 - Flags: review?(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.
Attachment #8418988 - Flags: review?(jorendorff) → review+
(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 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.
Attachment #8418991 - Flags: review?(jorendorff) → review+
Please land this!
https://hg.mozilla.org/mozilla-central/rev/0d4e738415f6
https://hg.mozilla.org/mozilla-central/rev/17f1c024a18d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.