1122619 - Remove getPropertyAttributes object ops

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Tom S [:evilpie]]

Attachment: remove-getgenericattributes

The hardest part with this patch is figuring out when and if it's valid to not walk the proto-chain. Sadly SuppressDeletedPropertyHelper needs a way to get property descriptors from the whole chain. We really need to look into removing this function.

I think the call to itself in GetOwnPropertyDescriptor, when we remove lookupGeneric, because after that NonProxyLookupOwnProperty should not return pobj at all. (Or maybe still for shared properties? At least it should be a native object as well.)

Comment 1

[Tom S [:evilpie]]

Attachment: v1 Remove getGenericAttributes ObjectOp

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8550786 [details] [diff] [review]
v1 Remove getGenericAttributes ObjectOp

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

Clearing review flag for now, but please r?me again soon! It looks like good stuff, mostly.

::: js/src/jsapi.cpp
@@ +2962,5 @@
> +    if (obj2->is<ProxyObject>())
> +        return Proxy::getPropertyDescriptor(cx, obj2, id, desc);
> +
> +    // Assume other non-natives (i.e. TypedObjects) behave in a sane way.
> +    return GetOwnPropertyDescriptor(cx, obj2, id, desc);

Before I review this in detail, can you explain this justification more carefully? It seems like GetPropertyDescriptor and GetOwnPropertyDescriptor are two different operations, so there's no reason to think this would be correct. You can *simulate* one using the other, but it would involve manually walking the prototype chain, right?

(I wonder if bhackett's unboxed objects might expose this bug -- if it is a bug.)

Comment 3

[Tom S [:evilpie]]

You sadly can't see it in the context of the patch, but obj2 is the result of LookupProperty. So in theory, unless something implements lookupGeneric in the wrong way, we should be correct by using GetOwnProperty on the object where the property was found. For the LookupProperty removal we probably want to replace this with a GetOwnProperty -> GetProto loop.

Comment 4

[Tom S [:evilpie]]

Attachment: v1 Rebased

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8553259 [details] [diff] [review]
v1 Rebased

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

r=me, and thank you!

::: js/public/Class.h
@@ +390,5 @@
>  
>  #define JS_NULL_OBJECT_OPS                                                    \
>      {nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,  \
>       nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,  \
> +     nullptr, nullptr}

I think you only mean to be removing one of these, not two. (?)

The compiler should catch it, though, unless this macro is unused now.

::: js/src/jsapi.cpp
@@ +2959,5 @@
> +
> +    // When we hit a proxy during lookup, the property might be
> +    // on the prototype of the proxy, thus use getPropertyDescriptor.
> +    if (obj2->is<ProxyObject>())
> +        return Proxy::getPropertyDescriptor(cx, obj2, id, desc);

The changes here are fine.

But this function is a mess. In a separate patch maybe, could you try reimplementing it as follows, and see if it passes tests? I think it will...

static bool
GetPropertyDescriptorById(JSContext *cx, HandleObject obj, HandleId id,
                          MutableHandle<PropertyDescriptor> desc)
{
    RootedObject proto(cx);
    for (RootedObject current(cx, obj); current; current = proto) {
        if (current->is<ProxyObject>())
            return Proxy::getPropertyDescriptor(cx, current, id, desc);
        if (!GetOwnPropertyDescriptor(cx, current, id, desc))
            return false;
        if (desc.object())
            return true;
        if (!GetPrototype(cx, current, &proto))
            return false;
    }
    MOZ_ASSERT(!desc.object());
    return true;
}

And while you're doing that, you could rename it to js::GetPropertyDescriptor(), move it to jsobj.h/cpp, and change the handful of other internal uses of JS_GetPropertyDescriptorById to use that.

::: js/src/jsobj.cpp
@@ +1094,5 @@
>              if ((attrs | new_attrs) == attrs)
>                  continue;
>  
>              attrs |= new_attrs;
>              if (!SetPropertyAttributes(cx, obj, id, &attrs))

Please file a bug, if you haven't already, blocking the es6 tracking bug: this doesn't follow the spec. We should get rid of SetPropertyAttributes and use JSPROP_IGNORE_ENUMERATE and JSPROP_IGNORE_VALUE to do this sort of partial DefineProperty.

@@ +1166,5 @@
>          // frozen. If the property is a writable data property, this object is
>          // not frozen.
>          if (!(attrs & JSPROP_PERMANENT) ||
>              (level == IntegrityLevel::Frozen &&
>               !(attrs & (JSPROP_READONLY | JSPROP_GETTER | JSPROP_SETTER))))

Style nit: Blank line before the comment.

I think this will read a lot better if you change:

    if (desc.configurable() ||
        (level == IntegrityLevel::Frozen && desc.isDataDescriptor() && desc.writable()))

Oh, right, I haven't added all those methods to PropertyDescriptor handles yet. Darn it. I'll get to it as soon as I can. :-|

::: js/src/vm/ArgumentsObject.cpp
@@ +325,5 @@
>          return true;
>      Handle<NormalArgumentsObject*> argsobj = obj.as<NormalArgumentsObject>();
>  
> +    Rooted<PropertyDescriptor> desc(cx);
> +    if (!GetOwnPropertyDescriptor(cx, argsobj, id, &desc))

Whew. This whole function is one big train wreck.

This change is fine though.

Comment 6

[Tom S [:evilpie]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Comment on attachment 8553259 [details] [diff] [review]
> v1 Rebased
> 
> Review of attachment 8553259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, and thank you!
> 
> ::: js/public/Class.h
> @@ +390,5 @@
> >  
> >  #define JS_NULL_OBJECT_OPS                                                    \
> >      {nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,  \
> >       nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,  \
> > +     nullptr, nullptr}
> 
> I think you only mean to be removing one of these, not two. (?)
> 
> The compiler should catch it, though, unless this macro is unused now.
>
It's actually to few! The compilers automatically initializes the rest to null anyway.
> ::: js/src/jsapi.cpp
> @@ +2959,5 @@
> > +
> > +    // When we hit a proxy during lookup, the property might be
> > +    // on the prototype of the proxy, thus use getPropertyDescriptor.
> > +    if (obj2->is<ProxyObject>())
> > +        return Proxy::getPropertyDescriptor(cx, obj2, id, desc);
> 
> The changes here are fine.
> 
> But this function is a mess. In a separate patch maybe, could you try
> reimplementing it as follows, and see if it passes tests? I think it will...
> 
> static bool
> GetPropertyDescriptorById(JSContext *cx, HandleObject obj, HandleId id,
>                           MutableHandle<PropertyDescriptor> desc)
> {
>     RootedObject proto(cx);
>     for (RootedObject current(cx, obj); current; current = proto) {
>         if (current->is<ProxyObject>())
>             return Proxy::getPropertyDescriptor(cx, current, id, desc);
>         if (!GetOwnPropertyDescriptor(cx, current, id, desc))
>             return false;
>         if (desc.object())
>             return true;
>         if (!GetPrototype(cx, current, &proto))
>             return false;
>     }
>     MOZ_ASSERT(!desc.object());
>     return true;
> }
> 
> And while you're doing that, you could rename it to
> js::GetPropertyDescriptor(), move it to jsobj.h/cpp, and change the handful
> of other internal uses of JS_GetPropertyDescriptorById to use that.
> 
bug 1124935
> ::: js/src/jsobj.cpp
> @@ +1094,5 @@
> >              if ((attrs | new_attrs) == attrs)
> >                  continue;
> >  
> >              attrs |= new_attrs;
> >              if (!SetPropertyAttributes(cx, obj, id, &attrs))
> 
> Please file a bug, if you haven't already, blocking the es6 tracking bug:
> this doesn't follow the spec. We should get rid of SetPropertyAttributes and
> use JSPROP_IGNORE_ENUMERATE and JSPROP_IGNORE_VALUE to do this sort of
> partial DefineProperty.
> 
Bug 1125437
> @@ +1166,5 @@
> >          // frozen. If the property is a writable data property, this object is
> >          // not frozen.
> >          if (!(attrs & JSPROP_PERMANENT) ||
> >              (level == IntegrityLevel::Frozen &&
> >               !(attrs & (JSPROP_READONLY | JSPROP_GETTER | JSPROP_SETTER))))
> 
> Style nit: Blank line before the comment.
> 
> I think this will read a lot better if you change:
> 
>     if (desc.configurable() ||
>         (level == IntegrityLevel::Frozen && desc.isDataDescriptor() &&
> desc.writable()))
> 
> Oh, right, I haven't added all those methods to PropertyDescriptor handles
> yet. Darn it. I'll get to it as soon as I can. :-|
> 
I did what I could without introducing new ones.

Comment 7

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff99308cdefc

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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