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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 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

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8565090 [details] [diff] [review]
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.
Attachment #8565090 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 1139700
https://hg.mozilla.org/mozilla-central/rev/ca21533bbb1c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.