1125437 - Get rid of SetPropertyAttributes and use DefineProperty to follow ES6 specification

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

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.

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

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.

Comment 2

[Tom S [:evilpie]]

Oh and this didn't work either before:
x = new Int8Array(2)
Object.defineProperty(x, 0, {value: 2})
x[0] == 2

Comment 3

[Tom S [:evilpie]]

Attachment: remove-setpropertyattributes

This fails less tests, because I moved the TypedArray checks into StandardDefineProperty in bug 1125628.

Comment 4

[Tom S [:evilpie]]

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.

Comment 5

[Tom S [:evilpie]]

Attachment: v2 - Remove SetPropertyAttributes

Added tests and step annotations.

Comment 6

[Eric Faust [:efaust]]

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.

Comment 7

[Tom S [:evilpie]]

Oh nice, getting this landed is going to be royal pain: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be37a8a5539e

Comment 8

[Tom S [:evilpie]]

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 9

[Tom S [:evilpie]]

Attachment: Remove CheckDefineProperty use StandardDefineProperty instead

Comment 10

[Eric Faust [:efaust]]

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.

Comment 11

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4de5e527fbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c93e99adfc7d

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4de5e527fbf
https://hg.mozilla.org/mozilla-central/rev/c93e99adfc7d