Closed Bug 1133094 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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.
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.
Attachment #8565090 - Flags: review?(efaustbmo)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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.
Attachment #8565090 - Flags: review?(efaustbmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/ca21533bbb1c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.