Merge js::PropDesc with JSPropertyDescriptor

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(5 attachments)

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.
Depends on: 1133085
Depends on: 1133094
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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.
Attachment #8571412 - Flags: review?(efaustbmo)
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...
Attachment #8571408 - Flags: review?(efaustbmo) → review+
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
Attachment #8571409 - Flags: review?(efaustbmo) → review+
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 :)
Attachment #8571410 - Flags: review?(efaustbmo) → review+
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)
Attachment #8571411 - Flags: review?(efaustbmo) → review+
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...
Attachment #8571412 - Flags: review?(efaustbmo) → review+
(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.
You need to log in before you can comment on or make changes to this bug.