Closed Bug 1125437 Opened 5 years ago Closed 5 years ago

Get rid of SetPropertyAttributes and use DefineProperty to follow ES6 specification

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1122619 Comment 5
> 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.
Attached patch WIP (obsolete) — Splinter Review
I had to add checks for TypedArray, this should probably be a different bug.

Two tests:
    ecma_5/extensions/reviver-mutates-holder-object-nonnative.js
    ecma_5/extensions/reviver-mutates-holder-array-nonnative.js
Fail, but they are probably flawed, because you can't define something with configurable: true. Which should happen in the CreateDataProperty call per Spec.

Something weird that I noticed is that:
Object.defineProperty(t, 0, {configurable: true}) doesn't throw.
Oh and this didn't work either before:
x = new Int8Array(2)
Object.defineProperty(x, 0, {value: 2})
x[0] == 2
Attached patch remove-setpropertyattributes (obsolete) — Splinter Review
This fails less tests, because I moved the TypedArray checks into StandardDefineProperty in bug 1125628.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8554282 - Flags: review?(jorendorff)
Attachment #8554179 - Attachment is obsolete: true
Oh and I would much rather use the PropDesc version of StandardDefineProperty, but we first need to invent a good syntax for e.g. "enumerable" is absent/true/false.
Added tests and step annotations.
Attachment #8554282 - Attachment is obsolete: true
Attachment #8554282 - Flags: review?(jorendorff)
Attachment #8559126 - Flags: review?(jorendorff)
Comment on attachment 8559126 [details] [diff] [review]
v2 - Remove SetPropertyAttributes

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

Stealing, as discussed on IRC. This is great. Glad to see this go. Drop-in attribute changes were unappropriately checked. r=me

::: js/src/jsobj.cpp
@@ +1152,5 @@
>          RootedId id(cx);
>          Rooted<PropertyDescriptor> desc(cx);
> +
> +        const unsigned AllowConfigure = JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_READONLY |
> +                                        JSPROP_IGNORE_VALUE;

Man, I didn't expect people would actually *use* these when I added them. What a hack. Oh well, it's a useful stopgap.
Attachment #8559126 - Flags: review?(jorendorff) → review+
Oh nice, getting this landed is going to be royal pain: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be37a8a5539e
Okay I think I found the problem. When invoking defineProperty on a scripted proxy we forward to DirectProxyHandler::defineProperty, which uses CheckDefineProperty. This function doesn't handle JSPROP_IGNORE_VALUE, so it complains when trying to change a non-configurable and non-writable property.

e.g. QueryInterface is such a property, we invoke defineProperty(.., {configurable: false, writable: false}). But CheckDefineProperty sees a value of `undefined`, which is different from the QueryInterface function.

I am not quite sure why DirectProxyHandler::defineProperty uses CheckDefineProperty, instead of StandardDefineProperty. If we can't fix that we should at least change ScriptedDirectProxyHandler to call StandardDefineProperty instead.
Comment on attachment 8562894 [details] [diff] [review]
Remove CheckDefineProperty use StandardDefineProperty instead

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

Yes yes yes yes yes yes yes. So happy to see this go. It was horribly wrong, relative to the current spec.
Attachment #8562894 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/f4de5e527fbf
https://hg.mozilla.org/mozilla-central/rev/c93e99adfc7d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Duplicate of this bug: 1060068
You need to log in before you can comment on or make changes to this bug.