1037770 - Object.defineProperty will wipe an object in this case.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[dindog]

>Code:
foo = 1;
Object.defineProperty(window,"foo",{writable:false});
foo = 2;
alert(foo);

>Outcomes:
IE 11:
       foo=1
Chrome 36:
       foo=1
Opera 22:
       foo=1

Firefox:
       foo is undefined!

Comment 1

[Till Schneidereit [:till]]

Confirmed, and I bet it's related to proxies.

Comment 2

[André Bargull [:anba]]

Yep, related proxies. Same as bug 671099?

Shell test case:
---
(function testCase() {
  var p = new Proxy({}, {});

  p.foo = 1;
  print("1: ", Object.getOwnPropertyDescriptor(p, "foo").toSource());

  Object.defineProperty(p, "foo", {writable: false});
  print("2: ", Object.getOwnPropertyDescriptor(p, "foo").toSource());
})();


Actual output:
1:  ({configurable:true, enumerable:true, value:1, writable:true})
2:  ({configurable:false, enumerable:false, value:(void 0), writable:false})

Expected output:
1:  ({configurable:true, enumerable:true, value:1, writable:true})
2:  ({configurable:true, enumerable:true, value:1, writable:false})

Comment 3

[Eric Faust [:efaust]]

Oh boy...Nice catch. I'll take this.

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix?

This seems to fix the problem.

Some parts of it definitely make me queasy. The whole |updateValue| shenanigan seems a bit sketchy.

I can't tell whether to flag this as review or feedback. It definitely needs a close look, but it's not totally slapped together, either

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8464376 [details] [diff] [review]
Fix?

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

Stealing.

This is a half measure, but worth taking in my view.

One concern here is that the JSPROP_IGNORE_ bits are not checked in every path through DefineNativeProperty.

They should always either (a) be checked and selectively applied to the pre-existing property's attrs; or (b) be checked anyway, and ignored values replaced with the defaults, if no pre-existing property is found. This is what's specified in ES6 draft rev 26 9.1.6.3 <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-validateandapplypropertydescriptor> 2.c.i and 2.d.i; and see Table 4 <http://people.mozilla.org/~jorendorff/es6-draft.html#table-4> for the default values.

At least, it seems easy enough to implement that right now.

A bigger concern is the lack of tests. At least the test case in comment 0 should go on. But it seems like there should be similar tests that get at this code via proxies, or failing that, jsapi-tests, just to make sure each path is exercised.

r=me with these comments addressed.

::: js/src/jsapi.h
@@ +905,5 @@
>  /************************************************************************/
>  
>  /* Property attributes, set in JSPropertySpec and passed to API functions. */
> +/* NB: The data structure some of these values in only uses a uint8_t to store
> +       the relevant information. Proceed with caution if trying to reorder the

The first sentence here doesn't parse for me.

Style nit: please change this to a single comment:

/*
 * Property attributes, ...
 *
 * NB: ...
 *     ...
 */

::: js/src/jsobj.cpp
@@ +668,1 @@
>  // See comments on CheckDefineProperty in jsobj.h.

It's declared in jsfriendapi.h; please update the comment.

@@ +673,5 @@
>  js::CheckDefineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue value,
>                          unsigned attrs, PropertyOp getter, StrictPropertyOp setter)
>  {
> +    // Discard any JSPROP_IGNORE that might have come in.
> +    attrs = uint8_t(attrs);

Instead, how about we assert that none came in. API usage mistakes shouldn't pass silently.

@@ +3987,5 @@
> +SanitizeIgnoredPropAttrs(unsigned attrs, HandleShape shape)
> +{
> +    /* Respect the fact that some callers may want to preserve existing attributes as much as
> +     * possible.
> +     */

Style nit: Use // please, or else make the multiline comment conform to SpiderMonkey style:

  /*
   * Respect ...
   * possible.
   */

@@ +3994,5 @@
> +        if (shape->enumerable()) {
> +            attrs |= JSPROP_ENUMERATE;
> +        } else {
> +            attrs &= ~JSPROP_ENUMERATE;
> +        }

Style nit: Lose the extra curly braces, since the "if" "then" and "else" parts are all single-line.

@@ +4030,1 @@
>      /*

Style nit: blank line before this comment.

@@ +4049,5 @@
>                      return false;
>                  shape = obj->nativeLookup(cx, id);
>              }
>              if (shape->isAccessorDescriptor()) {
> +                attrs = SanitizeIgnoredPropAttrs(attrs, shape);

I think ApplyAttributes or ApplySelectedAttributes would be a better name for this. "Sanitize" just doesn't seem like the right sort of verb to me. Your call.

@@ +4054,2 @@
>                  shape = JSObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs,
> +                                                                        JSPROP_GETTER | JSPROP_SETTER,

Style nit: It looks like it was aligned correctly before.

@@ +4065,4 @@
>              }
>          }
> +    } else if (!(attrs & JSPROP_IGNORE_VALUE)) {
> +        /* We might still want to ignore redefining some of our attributes, if the

Style nit: comment style here

@@ +4070,5 @@
> +         * a data property.
> +         * FIXME: All this logic should be removed when Proxies use PropDesc, but we need to
> +         *        remove JSPropertyOp getters and setters first.
> +         */
> +        shape = obj->nativeLookup(cx, id);

How can this return anything other than null? We already did a LookupOwnProperty(cx, obj, id) and it didn't find a shape.

@@ +4074,5 @@
> +        shape = obj->nativeLookup(cx, id);
> +        if (shape && shape->isDataDescriptor())
> +            attrs = SanitizeIgnoredPropAttrs(attrs, shape);
> +    } else {
> +        /* We have been asked merely to update some attributes by a caller of

Style nit: comment style here

Comment 6

[Eric Faust [:efaust]]

Needed a small change, due to failures on try.

Passing 0 as the mask throws away things like JSPROP_SHADOWABLE or JSPROP_GETTER from the shape in JSObject::changeProperty. The following diff was added to only allow changes to the bits we mean to change:

> -            shape = JSObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs, 0,
> +            /* Keep everything from the shape that isn't the things we're changing */
> +            unsigned attrMask = ~(JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
> +            shape = JSObject::changeProperty<SequentialExecution>(cx, obj, shape, attrs, attrMask,
>                                                                    shape->getter(), shape->setter());

Comment 7

[Eric Faust [:efaust]]

Attachment: With tests and nits addressed.

This is already looking pretty promising on try: https://tbpl.mozilla.org/?tree=Try&rev=3a170251e640

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8467457 [details] [diff] [review]
With tests and nits addressed.

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

::: js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
@@ +50,5 @@
> +                            IgnoreAll | JSPROP_NATIVE_ACCESSORS, (JSPropertyOp)Getter));
> +
> +    CHECK(JS_GetPropertyDescriptor(cx, obj, "foo", &desc));
> +    // Note that since JSPROP_READONLY means nothing for accessor properties, we will actually
> +    // claim to be writable, since the flag is not included in the mask.

Style nit: blank line before a comment (except at the top of a block).

::: js/src/jsobj.cpp
@@ +4141,1 @@
>      /*

Blank line before comment.

Converting this comment to // style would be ok.

@@ +4196,5 @@
> +            return false;
> +
> +        if (shape) {
> +            attrs = ApplyOrDefaultAttributes(attrs, shape);
> +            /* Keep everything from the shape that isn't the things we're changing */

Blank line before comment.

Comment 9

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28ccb18dc241

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/28ccb18dc241