492844 - ES5: Implement Object.freeze, Object.isFrozen

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

+++ This bug was initially created as a clone of Bug #492840 +++

Object.freeze and Object.isFrozen are specified by 15.2.3.9 and 15.2.3.12 in the ES5 draft:
http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft

Comment 1

[Jeff Walden [:Waldo]]

This bug depends on Brendan's scope-removal work in bug 558451.

Comment 2

[Brendan Eich [:brendan]]

See bug 492849 comment 9.

/be

Comment 3

[Jim Blandy :jimb]

Attachment: Implement Object.freeze, Object.isFrozen

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 476044 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

The logic is wrong here, see ES5 15.2.3.9 Step 2, noted in bug 492849 comment 38.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 476044 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

I'm clearly too tired to review, or do much of anything else. Sorry about that. To make up for it, some nits and an r+.
 
>+        /* If no change is necessary, avoid the setAttributes call. */
>         if ((attrs & JSPROP_PERMANENT) &&
>-            (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)) || (attrs & JSPROP_READONLY))) {
>+            ((attrs & JSPROP_READONLY) || (attrs & (JSPROP_GETTER | JSPROP_SETTER))))
>             continue;
>-        }

Keep the parens per house style (multiline condition is enough to trigger them).

Combine all the tests: (attrs & (JSPROP_READONLY | JSPROP_GETTER | JSPROP_SETTER)).

/be

Comment 6

[Jim Blandy :jimb]

Attachment: Implement Object.freeze, Object.isFrozen

Revised per comments.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 476839 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

Shouldn't deepEqual use === instead of ==? Even === will equate -0 and 0, and not equate NaN with itself. See

http://wiki.ecmascript.org/doku.php?id=strawman:egal

where the fix is coded in JS:

 Object.eq = function(x, y) {
    if (x === y) {
      // 0 === -0, but they are not identical
      return x !== 0 || 1/x === 1/y;
    }
 
    // NaN !== NaN, but they are identical.
    // NaNs are the only non-reflexive value, i.e., if x !== x,
    // then x is a NaN.
    // isNaN is broken: it converts its argument to number, so
    // isNaN("foo") => true
    return x !== x && y !== y;
  }

/be

Comment 8

[Jason Orendorff [:jorendorff]]

FWIW, I agree deepEqual should handle -0 and NaN specially so that

   deepEqual(NaN, NaN) ===>  true
   deepEqual(0, -0)    ===>  false

and I think it should check that Object.prototype.toString.call produces the same result for both operands (that is, that they have the same [[Class]]).

(Admittedly you eventually should stop and go work on other important stuff...)

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 476839 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

In jsobj.cpp, in GetFirstArgumentAsObject:
>    if (v.isPrimitive()) {

Uber-micro-nit: it would be nice for this to say instead:

     if (!v.isObject()) {

Exactly the same meaning, but this goes nicer with the .toObject() call that follows.

In jsobj.cpp:
>+static JSBool
>+obj_freeze(JSContext *cx, uintN argc, Value *vp)
>+{
>+    JSObject *obj;
>+    if (!GetFirstArgumentAsObject(cx, argc, vp, "Object.preventExtensions", &obj))

The string there should say "Object.freeze".

I could stand to see a few more tests:

 - calling freeze() and isFrozen() without arguments or with non-object arguments should throw

 - freezing one object shouldn't freeze a separate object with the same (empty or non-empty) shape, of course

 - after freezing an Array, .length and its elements, should appear on it, and they should all be read-only -- you shouldn't be able to set .length afterwards. (Actually I may not correctly understand 15.4.5.1 on this point, but whatever it says, we should do...)

 - likewise, freezing a Function should cause .arity and .name properties to appear.

 - and freezing an arguments object should do whatever ES5 says, even in a non-strict mode function (shudder) and even after the corresponding function call has returned

 - freezing an E4X object and then poking at it shouldn't crash

 - freezing the global object, so help us, should do whatever ES5 says

 - freezing the global object on trace should deep-bail:

    var arr = [{}, {}, {}, {}, this];
    for (var i = 0; i < arr.length; i++)   // once we go on trace, this.i isn't updated
        Object.freeze(arr[i]);             // needs to freeze this.i == 4
    assertEq(this.i, arr.indexOf(this));

 - freezing shouldn't change [[Class]] (as observable by Object.prototype.toString.call(obj))

 - freezing an object while it's the target of an active with-block, and then trying to write to one of its properties, should fail as expected

 - freezing an object that has methods (in the sense of js::Shape::METHOD), and then getting a method out, should work as expected (basically I think freezing needs to trip the method read barrier and force all the methods to be really truly created)

 - freezing a proxy should do something (throw, if that's what you've got implemented at this point)

 - the ES5 spec says that an inextensible object's [[Prototype]] may not change; we should test that. (I don't see a test for it in bug 492849; sorry if I'm missing something!)

How is freezing supposed to interact with resolve/newResolve hooks? How does it work now; is that likely to break existing JSAPI code (of which we have a bunch)?

r=me with the nits picked and as many tests as you think wise.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #9)
>     var arr = [{}, {}, {}, {}, this];
>     for (var i = 0; i < arr.length; i++)   // once we go on trace, this.i isn't
> updated
>         Object.freeze(arr[i]);             // needs to freeze this.i == 4
>     assertEq(this.i, arr.indexOf(this));

Oops! The loop will never terminate. But you get the idea I hope. :)

Comment 11

[Jason Orendorff [:jorendorff]]

I meant to mention: in one of the tests, a function getPropertyOf is defined but never called.

Comment 12

[Brendan Eich [:brendan]]

> How is freezing supposed to interact with resolve/newResolve hooks? How does it
> work now; is that likely to break existing JSAPI code (of which we have a bunch)?

Resolve is just a way of defining (as in JS_DefineProperty) properties on demand, lazily. So API tests that exercise JS_Define* and JS_Set* from a resolve hook could be enough, if we don't already have tests using ES5 Object.defineProperty etc. on frozen objects. I recall some of those, though.

Point is, resolve can't do anything not already doable via the JS API if not via ES5's in-language meta-programming API.

/be

Comment 13

[Jim Blandy :jimb]

(In reply to comment #7)
> Shouldn't deepEqual use === instead of ==? Even === will equate -0 and 0, and
> not equate NaN with itself. See

Fixed.

Comment 14

[Jim Blandy :jimb]

(In reply to comment #9)
> In jsobj.cpp, in GetFirstArgumentAsObject:
> >    if (v.isPrimitive()) {
> 
> Uber-micro-nit: it would be nice for this to say instead:
> 
>      if (!v.isObject()) {
> 
> Exactly the same meaning, but this goes nicer with the .toObject() call that
> follows.

Fixed.

> In jsobj.cpp:
> >+static JSBool
> >+obj_freeze(JSContext *cx, uintN argc, Value *vp)
> >+{
> >+    JSObject *obj;
> >+    if (!GetFirstArgumentAsObject(cx, argc, vp, "Object.preventExtensions", &obj))
> 
> The string there should say "Object.freeze".

Fixed, thanks.

> I could stand to see a few more tests:

I agree, but given the time pressure, I've filed this as bug 598390.

Comment 15

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/1a49e9c79d5a

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1a49e9c79d5a

Comment 17

[Fabian (Crash) Grutschus]

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/freeze
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/isFrozen

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/freeze
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/isFrozen

My understanding is that changing properties on a frozen object will only throw an exception in strict mode but that isn't mentioned in the docs. Am I mistaken?

Comment 19

[Fabian (Crash) Grutschus]

(In reply to comment #18)
> My understanding is that changing properties on a frozen object will only throw
> an exception in strict mode but that isn't mentioned in the docs. Am I
> mistaken?

I can't find anything about that, even not in ECMA-262. Object.freeze loops over all properties of the object and sets writable and configurable to false and I can't also find any documentation that setting, deleting such properties only throw an exception in strict mode.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> (In reply to comment #18)
> > My understanding is that changing properties on a frozen object will only throw
> > an exception in strict mode but that isn't mentioned in the docs. Am I
> > mistaken?
> 
> I can't find anything about that, even not in ECMA-262. Object.freeze loops
> over all properties of the object and sets writable and configurable to false
> and I can't also find any documentation that setting, deleting such properties
> only throw an exception in strict mode.

11.4.1 The delete Operator

The production UnaryExpression : delete UnaryExpression is evaluated as follows:

1. Let ref be the result of evaluating UnaryExpression.
2. If Type(ref) is not Reference, return true.
3. If IsUnresolvableReference(ref) then,
a. If IsStrictReference(ref) is true, throw a SyntaxError exception.
b. Else, return true.
4. If IsPropertyReference(ref) is true, then
a. Return the result of calling the [[Delete]] internal method on ToObject(GetBase(ref) providing GetReferencedName(ref) and IsStrictReference(ref) as the arguments.
5. Else, ref is a Reference to an Environment Record binding, so
a. If IsStrictReference(ref) is true, throw a SyntaxError exception.
b. Let bindings be GetBase(ref).
c. Return the result of calling the DeleteBinding concrete method of bindings, providing GetReferencedName(ref) as the argument.

NOTE When a delete operator occurs within strict mode code, a SyntaxError exception is thrown if its UnaryExpression is a direct reference to a variable, function argument, or function name. In addition, if a delete operator occurs within strict mode code and the property to be deleted has the attribute { [[Configurable]]: false }, a TypeError exception is thrown.

For assignment, see 8.7.2 PutValue (V, W), IsStrictReference, etc. It's all there, as Mossop said.

/be

Comment 21

[Fabian (Crash) Grutschus]

Added it to the documentation, also at Object.seal, Object.preventExtensions and here: https://developer.mozilla.org/en/JavaScript/Strict_mode