1133094 - Object.defineProperty() on scripted proxy incorrectly sets {[[Configurable]]: true} if it's missing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The PropDesc record set up by PropDesc::initialize() in this case is bogus.
Both of the following are true:

    desc.hasConfigurable() == false

    (desc.attrs() & JSPROP_PERMANENT) != 0

It's observable from script:

    var p = new Proxy({x: 1}, {
        defineProperty() { return true; }
    });

    Object.defineProperty(p, "x", {get: function () {}});

This Object.defineProperty() call should be effectively a no-op. Instead it throws a TypeError.

Comment 1

[Jason Orendorff [:jorendorff]]

Oh it's even weirder.

The PropDesc weird, and as a result PropDesc::populatePropertyDescriptor creates a PropertyDescriptor with both JSPROP_PERMANENT and JSPROP_IGNORE_PERMANENT set.

We should ban that combination in the new order. It makes no sense and invites this kind of bug.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Fix complicated bug with Object.defineProperty() and scripted proxy

It is not immediately clear why the test has anything to do with the code changes, but the code changes do in fact make that test pass, because they cause us not to generate nonsensical PropDesc/PropertyDescriptor records that confuse our DefineProperty machinery.

The first hunk in jsobj.cpp is the key bit, and the rest is mopping up regressions from that change.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8565090 [details] [diff] [review]
Fix complicated bug with Object.defineProperty() and scripted proxy

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

See comment below. Maybe it's even worth taking seriously? r=me, either way.

::: js/src/jsobj.cpp
@@ +681,5 @@
> +            if (shapeDataDescriptor)
> +                unchanged |= JSPROP_READONLY;
> +            else
> +                descAttrs |= JSPROP_READONLY;
> +        }

This case is a bit of an uncomfortable thorn. I was going to suggest doing something like desc.attributesWithDefaults() that checks isDataDescriptor() or isAccessorDescriptor() and fills in the defaults according to has*(). This kinda makes that unsavory. I would in general like to keep it so that PropDesc is allowed to be "smarter" than JSPropertyDescriptor in that it's not just a hollow vessel for the bits, but might actually have some idea how the algorithms work, so that we don't have to sprinkle in |s and &~s everywhere.

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #3)
> Comment on attachment 8565090 [details] [diff] [review]
> Fix complicated bug with Object.defineProperty() and scripted proxy
> 
> Review of attachment 8565090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comment below. Maybe it's even worth taking seriously? r=me, either way.
> 
> ::: js/src/jsobj.cpp
> @@ +681,5 @@
> > +            if (shapeDataDescriptor)
> > +                unchanged |= JSPROP_READONLY;
> > +            else
> > +                descAttrs |= JSPROP_READONLY;
> > +        }
> 
> This case is a bit of an uncomfortable thorn. I was going to suggest doing
> something like desc.attributesWithDefaults() that checks isDataDescriptor()
> or isAccessorDescriptor() and fills in the defaults according to has*().
> This kinda makes that unsavory.

I think I might actually have patches that rework this code to switch from "unchanged" to "changed" (whitelisting rather than blacklisting bits from the input descriptor).

In any case 100% of this is being deleted in the short run (bug 1125624), so I don't want to polish it much.

> I would in general like to keep it so that
> PropDesc is allowed to be "smarter" than JSPropertyDescriptor in that it's
> not just a hollow vessel for the bits, but might actually have some idea how
> the algorithms work, so that we don't have to sprinkle in |s and &~s
> everywhere.

I agree. I'll add something along these lines to JSPropertyDescriptor when rewriting DefineNativeProperty in bug 1125624. PropDesc too is going to be removed in a matter of days (bug 1133081) so I don't want to add a method now.

Comment 5

[Jason Orendorff [:jorendorff]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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