1133081 - Merge js::PropDesc with JSPropertyDescriptor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Shaving this off of bug 1125624.

PropDesc has nicer methods; JSPropertyDescriptor is more widely used outside the engine and is more complete in terms of non-standard features. Let's go with JSPropertyDescriptor.

I'm doing this right now but there are some bugs that have to be fixed first.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Switch from js::PropDesc to JSPropertyDescriptor for all users of js::StandardDefineProperty (mainly Object.defineProperty/Properties and the corresponding Debugger.Object methods)

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Switch from js::PropDesc to JSPropertyDescriptor for js::StandardDefineProperty implementation

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - Switch from js::PropDesc to JSPropertyDescriptor for more odds and ends

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - Reimplement the remaining PropDesc methods and delete PropDesc

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - Remove non-asserting PropertyDescriptor accessors in favor of the new PropDesc-inspired asserting accessors

value() can't assert hasValue() because too many places have plausible reasons for calling it on a PropertyDescriptor they basically know nothing about. One such place is CompartmentChecker::check(Handle<JSPropertyDescriptor>). Another is DefinePropertyByDescriptor. Maybe this will change with time.

In some cases we do things like `desc.hasWritable() && desc.writable() != existing_desc.writable()`. It is OK to write it this way, even though we have not checked existing_desc.hasWritable(), because in these cases we already know existingDesc is a complete property descriptor.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8571408 [details] [diff] [review]
part 1 - Switch from js::PropDesc to JSPropertyDescriptor for all users of js::StandardDefineProperty (mainly Object.defineProperty/Properties and the corresponding Debugger.Object methods)

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

Looks good.

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

This seems scary, just as I read this. I thought the point was a JSPropertyDescriptor without an object() corresponds to an undefined PropDesc?

@@ +193,4 @@
>      if (!hasConfigurable())
>          attrs |= JSPROP_IGNORE_PERMANENT;
> +    if (!isAccessorDescriptor()) {
> +        if (!hasWritable())

yeah, OK. This is certainly more right.

@@ +341,5 @@
>      id = NameToId(cx->names().get);
>      if (!GetPropertyIfPresent(cx, desc, id, &v, &found))
>          return false;
>      if (found) {
> +        if (!v.isObject() && !v.isUndefined()) {

This is all going away eventually, but the interactions between this and checkGetter() and the requirement of Objectness on JSPropertyDescriptor's account could use a comment. Similarly below.

::: js/src/vm/SelfHosting.cpp
@@ +377,5 @@
>      MOZ_ASSERT(bool(attributes & ATTR_CONFIGURABLE) != bool(attributes & ATTR_NONCONFIGURABLE),
>                 "_DefineDataProperty must receive either ATTR_CONFIGURABLE xor "
>                 "ATTR_NONCONFIGURABLE");
> +    if (attributes & ATTR_NONCONFIGURABLE)
> +        attrs |= JSPROP_PERMANENT;

as an aside, these enums are super annoying, and can probably go. I guess they are...

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8571409 [details] [diff] [review]
part 2 - Switch from js::PropDesc to JSPropertyDescriptor for js::StandardDefineProperty implementation

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

This is pretty sweet.

::: js/src/jsobj.cpp
@@ +484,5 @@
>  
>          MOZ_ASSERT(desc.isAccessorDescriptor());
>  
> +        unsigned attrs = desc.attributes() & (JSPROP_PERMANENT | JSPROP_ENUMERATE |
> +                                              JSPROP_SHARED | JSPROP_GETTER | JSPROP_SETTER);

Can/should/(do we elsewhere) assert that desc.hasAttribute(JSPROP_SHARED)?

@@ +654,5 @@
> +            // The hasSetterValue() and hasGetterValue() calls below ought to
> +            // be redundant here, because accessor shapes should always have
> +            // both JSPROP_GETTER and JSPROP_SETTER. But this is not the case
> +            // currently; in particular Object.defineProperty(obj, key, {get: fn})
> +            // creates a property without JSPROP_SETTER (bug 1133315).

UNENDING TORMENT

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8571410 [details] [diff] [review]
part 3 - Switch from js::PropDesc to JSPropertyDescriptor for more odds and ends

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

I was wondering when I would see the proxy code :)

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8571411 [details] [diff] [review]
part 4 - Reimplement the remaining PropDesc methods and delete PropDesc

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

Very cool.

::: js/src/jsobj.cpp
@@ +824,5 @@
> +        attrs &= ~(JSPROP_IGNORE_READONLY | JSPROP_IGNORE_VALUE);
> +    }
> +
> +    desc.setAttributes(attrs);
> +    MOZ_ASSERT_IF(attrs & JSPROP_READONLY, !(attrs & (JSPROP_GETTER | JSPROP_SETTER)));

MOZ_ASSERT_IF(attrs & (JSPROP_GETTER | JSPROP_SETTER), attrs & JSPROP_SHARED);

Trivial, but worth maybe worth it.

::: js/src/jsobj.h
@@ +1211,5 @@
> +inline JSSetterOp
> +CastAsSetterOp(JSObject *object)
> +{
> +    return JS_DATA_TO_FUNC_PTR(JSSetterOp, object);
> +}

I remember when I stole these out of Shape.h for PropDesc. I guess them landing here is no worse. (No change necessary)

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8571412 [details] [diff] [review]
part 5 - Remove non-asserting PropertyDescriptor accessors in favor of the new PropDesc-inspired asserting accessors

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

I guess it is romans and greeks...

Comment 11

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=78bcfd34dfe5

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #6)
> ::: js/src/jsobj.cpp
> @@ -147,5 @@
> >  {
> >      MOZ_ASSERT(isUndefined());
> >  
> > -    if (!desc.object())
> > -        return;
> 
> This seems scary, just as I read this. I thought the point was a
> JSPropertyDescriptor without an object() corresponds to an undefined
> PropDesc?

Without this change, switching from PropDesc toPropertyDescriptor in intrinsic_DefineDataProperty breaks the engine.

When you're looking at the return value of GetOwnPropertyDescriptor, yes, null object() means undefined. But everywhere else we should be ignoring the object() field. It would be really strange for a DefineProperty call to silently do nothing (or define a blank data property) because the object() field wasn't populated on input.

I'd like to replace the object() field with a `bool found;` field, used only for GetOwnPropertyDescriptor lookups. Not quite there yet.

> This is all going away eventually, but the interactions between this and checkGetter() and the
> requirement of Objectness on JSPropertyDescriptor's account could use a comment. Similarly below.

I added the comment, but you're right, it gets deleted in part 4 of the stack in this bug!

> ::: js/src/vm/SelfHosting.cpp
> > +    if (attributes & ATTR_NONCONFIGURABLE)
> > +        attrs |= JSPROP_PERMANENT;
> 
> as an aside, these enums are super annoying, and can probably go. I guess
> they are...

Yes. That would be great.

> >          MOZ_ASSERT(desc.isAccessorDescriptor());
> >  
> > +        unsigned attrs = desc.attributes() & (JSPROP_PERMANENT | JSPROP_ENUMERATE |
> > +                                              JSPROP_SHARED | JSPROP_GETTER | JSPROP_SETTER);
> 
> Can/should/(do we elsewhere) assert that desc.hasAttribute(JSPROP_SHARED)?

Not yet, but bug 1138499 is coming!

> > +            // The hasSetterValue() and hasGetterValue() calls below ought to
> > +            // be redundant here, because accessor shapes should always have
> > +            // both JSPROP_GETTER and JSPROP_SETTER. But this is not the case
> > +            // currently; in particular Object.defineProperty(obj, key, {get: fn})
> > +            // creates a property without JSPROP_SETTER (bug 1133315).
> 
> UNENDING TORMENT

This particular torment's days are numbered.

> MOZ_ASSERT_IF(attrs & (JSPROP_GETTER | JSPROP_SETTER), attrs & JSPROP_SHARED);

Added.

Comment 13

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=984ec603c9e6

Comment 14

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a96f2eed5c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c7748956ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd31041fe5d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/63dbcc4fd0f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78a9d1273c5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a96f2eed5c9
https://hg.mozilla.org/mozilla-central/rev/00c7748956ad
https://hg.mozilla.org/mozilla-central/rev/fd31041fe5d6
https://hg.mozilla.org/mozilla-central/rev/63dbcc4fd0f0
https://hg.mozilla.org/mozilla-central/rev/c78a9d1273c5