Make Object.freeze/seal/preventExtensions observable

Make Object.freeze/seal/preventExtensions observable

Note: spec has been updated here: http://wiki.ecmascript.org/doku.php?id=harmony:observe_spec_changes.

R=rossberg@chromium.org, rossberg
BUG=v8:2975, v8:2941
Committed: https://code.google.com/p/v8/source/detail?r=17481

[adamk, 2013-10-29 20:46:08]

https://codereview.chromium.org/47703003/diff/20001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/20001/src/object-observe.js#new...
src/object-observe.js:452: else
Please add braces around this clause (and thus around the bodies in this whole
if/else statement).

[rafaelw, 2013-10-29 20:56:26]

https://codereview.chromium.org/47703003/diff/20001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/20001/src/object-observe.js#new...
src/object-observe.js:452: else
On 2013/10/29 20:46:08, adamk wrote:
> Please add braces around this clause (and thus around the bodies in this whole
> if/else statement).

Done.

[arv (Not doing code reviews), 2013-10-29 21:58:17]

FYI

https://codereview.chromium.org/47703003/diff/70001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/70001/src/object-observe.js#new...
src/object-observe.js:448: if (arguments.length == 2) {
This is tangential but ES does not usually use the length of arguments.

https://codereview.chromium.org/47703003/diff/70001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/47703003/diff/70001/src/objects.cc#newcode5527
src/objects.cc:5527: ASSERT(!object->map()->is_observed());
Might be worth pointing out that we use a different code path for observed
objects.

https://codereview.chromium.org/47703003/diff/70001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/70001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:305: var obj = { foo: 'bar '};
bar' }

instead of

bar '}

[rafaelw, 2013-10-30 17:54:36]

andreas: PTAL

https://codereview.chromium.org/47703003/diff/70001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/70001/src/object-observe.js#new...
src/object-observe.js:448: if (arguments.length == 2) {
(talked with arv off-line). This is internal-only API.

On 2013/10/29 21:58:18, arv wrote:
> This is tangential but ES does not usually use the length of arguments.

https://codereview.chromium.org/47703003/diff/70001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/47703003/diff/70001/src/objects.cc#newcode5527
src/objects.cc:5527: ASSERT(!object->map()->is_observed());
That's what the assertion is documenting. If you feel strongly, I'll put a
comment, but it seems superfluous.

On 2013/10/29 21:58:18, arv wrote:
> Might be worth pointing out that we use a different code path for observed
> objects.

https://codereview.chromium.org/47703003/diff/70001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/70001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:305: var obj = { foo: 'bar '};
good catch. done.

On 2013/10/29 21:58:18, arv wrote:
> bar' }
> 
> instead of
> 
> bar '}

[rossberg, 2013-10-30 19:11:45]

LGTM with comments

https://codereview.chromium.org/47703003/diff/130001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/130001/src/object-observe.js#ne...
src/object-observe.js:447: var changeRecord;
I think you can write this more concisely as follows:

var changeRecord = { type: type, object: object };
if (arguments.length >= 3) changeRecord.name = name;
if (arguments.length >= 4) changeRecord.oldValue = oldValue;

https://codereview.chromium.org/47703003/diff/130001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/47703003/diff/130001/src/objects.cc#newcode2171
src/objects.cc:2171: int argc = 4;
int argc = name.is_null() ? 2 : old_value->IsTheHole() ? 3 : 4;

https://codereview.chromium.org/47703003/diff/130001/src/objects.cc#newcode5496
src/objects.cc:5496: EnqueueChangeRecord(object, "preventExtensions",
Handle<Name>::null(),
Nit: Handle<Name>() is enough

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
test/mjsunit/harmony/object-observe.js:303: // Object.preventExtensions
Cases you could add:

- invoking preventExtensions/seal/freeze on an object that already has
preventExtensions (should not create a preventExtensions change record)

- invoking seal/freeze on an object where not all properties can be
sealed/frozen (should still generate a preventExtensions change record, IIRC)

[rafaelw, 2013-10-30 23:00:29]

https://codereview.chromium.org/47703003/diff/130001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/130001/src/object-observe.js#ne...
src/object-observe.js:447: var changeRecord;
We need the changeRecord object to have the default object proto, so only using
literals avoids monkey patches to Object.proto interfering here.

On 2013/10/30 19:11:45, rossberg wrote:
> I think you can write this more concisely as follows:
> 
> var changeRecord = { type: type, object: object };
> if (arguments.length >= 3) changeRecord.name = name;
> if (arguments.length >= 4) changeRecord.oldValue = oldValue;

https://codereview.chromium.org/47703003/diff/130001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/47703003/diff/130001/src/objects.cc#newcode2171
src/objects.cc:2171: int argc = 4;
On 2013/10/30 19:11:45, rossberg wrote:
> int argc = name.is_null() ? 2 : old_value->IsTheHole() ? 3 : 4;

Done.

https://codereview.chromium.org/47703003/diff/130001/src/objects.cc#newcode5496
src/objects.cc:5496: EnqueueChangeRecord(object, "preventExtensions",
Handle<Name>::null(),
On 2013/10/30 19:11:45, rossberg wrote:
> Nit: Handle<Name>() is enough

Done.

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
test/mjsunit/harmony/object-observe.js:303: // Object.preventExtensions
On 2013/10/30 19:11:45, rossberg wrote:
> Cases you could add:
> 
> - invoking preventExtensions/seal/freeze on an object that already has
> preventExtensions (should not create a preventExtensions change record)

Good catch. Note that we now exit early from JSObject::PreventExtensions if the
map is already marked non-extensible.

> 
> - invoking seal/freeze on an object where not all properties can be
> sealed/frozen (should still generate a preventExtensions change record, IIRC)

I can't think of how to generate this case. How can this arise?

[rossberg, 2013-10-31 08:03:20]

Still LGMT

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
test/mjsunit/harmony/object-observe.js:303: // Object.preventExtensions
On 2013/10/30 23:00:30, rafaelw wrote:
> On 2013/10/30 19:11:45, rossberg wrote:
> > Cases you could add:
> > 
> > - invoking preventExtensions/seal/freeze on an object that already has
> > preventExtensions (should not create a preventExtensions change record)
> 
> Good catch. Note that we now exit early from JSObject::PreventExtensions if
the
> map is already marked non-extensible.

Can you add a similar test for seal and freeze?


> > - invoking seal/freeze on an object where not all properties can be
> > sealed/frozen (should still generate a preventExtensions change record,
IIRC)
> 
> I can't think of how to generate this case. How can this arise?

I'm sorry, you are right, it can't -- I keep forgetting that JavaScript has this
crank semantics where you can still change a data property from writable to
non-writable, even if it's allegedly non-configurable. 8/

[arv (Not doing code reviews), 2013-10-31 14:43:41]

On 2013/10/31 08:03:20, rossberg wrote:
> > > - invoking seal/freeze on an object where not all properties can be
> > > sealed/frozen (should still generate a preventExtensions change record,
> IIRC)
> > 
> > I can't think of how to generate this case. How can this arise?
> 
> I'm sorry, you are right, it can't -- I keep forgetting that JavaScript has
this
> crank semantics where you can still change a data property from writable to
> non-writable, even if it's allegedly non-configurable. 8/

Hehe... we had the exact same discussion yesterday on IM before we remembered
the Mark Miller escape hatch.

[rafaelw, 2013-11-05 12:20:30]

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/obj...
test/mjsunit/harmony/object-observe.js:303: // Object.preventExtensions
On 2013/10/31 08:03:20, rossberg wrote:
> On 2013/10/30 23:00:30, rafaelw wrote:
> > On 2013/10/30 19:11:45, rossberg wrote:
> > > Cases you could add:
> > > 
> > > - invoking preventExtensions/seal/freeze on an object that already has
> > > preventExtensions (should not create a preventExtensions change record)
> > 
> > Good catch. Note that we now exit early from JSObject::PreventExtensions if
> the
> > map is already marked non-extensible.
> 
> Can you add a similar test for seal and freeze?
> 
> 
> > > - invoking seal/freeze on an object where not all properties can be
> > > sealed/frozen (should still generate a preventExtensions change record,
> IIRC)
> > 
> > I can't think of how to generate this case. How can this arise?
> 
> I'm sorry, you are right, it can't -- I keep forgetting that JavaScript has
this
> crank semantics where you can still change a data property from writable to
> non-writable, even if it's allegedly non-configurable. 8/

Done.

[rafaelw, 2013-11-05 12:25:39]

Committed patchset #7 manually as r17481 (presubmit successful).