[strong] Implement per-object restrictions behaviour of property freezing

[strong] Implement per-object restrictions behaviour of property freezing

Implements the strong mode proposal's restrictions on changing a strong object's
writable, non-configurable property to non-writable.

Setting the strong bit is still wip, so this change will only affect those
objects that have the bit correctly set. The tests reflect this, and will be
expanded as more objects can be marked as strong.

BUG=v8:3956
LOG=N

Committed: https://crrev.com/6edc3e317912d8ea66bdfc031b06755404e83956
Cr-Commit-Position: refs/heads/master@{#28698}

[conradw, 2015-05-21 22:49:24]

conradw@chromium.org changed reviewers:
+ arv@chromium.org, rossberg@chromium.org

[conradw, 2015-05-21 22:49:25]

PTAL

[conradw, 2015-05-21 23:09:25]

On 2015/05/21 22:49:25, conradw wrote:
> PTAL

Actually I think this might be broken just now.

[conradw, 2015-05-22 10:51:10]

On 2015/05/21 23:09:25, conradw wrote:
> On 2015/05/21 22:49:25, conradw wrote:
> > PTAL
> 
> Actually I think this might be broken just now.

fixed

[conradw, 2015-05-22 10:51:11]

On 2015/05/21 23:09:25, conradw wrote:
> On 2015/05/21 22:49:25, conradw wrote:
> > PTAL
> 
> Actually I think this might be broken just now.

fixed

[rossberg, 2015-05-22 13:10:29]

lgtm

https://codereview.chromium.org/1142393003/diff/20001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1142393003/diff/20001/src/messages.h#newcode231
src/messages.h:231: "Cannot redefine non-configurable property '%' of strong
object % to be "  \
Nit: sylise

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:810: if (!currentIsWritable) {
Nit:  if (!currentIsWritable || IS_STRONG(obj))  here

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:812: throw MakeTypeError(kRedefineDisallowed, p);
...and then  throw currentIsWritable ? MakeTypeError(...) : MakeTypeError(...) 
here

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:817: if (IS_STRONG(obj)) {
...and eliminate this duplication

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-freeze-property.js (right):

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:21: // Strong functions can't have
properties added to them.
Nit: indentation

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:30: let nonStrongObjects =
sloppyObjects.concat(strictObjects);
Nit: weakObjects

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:33: Object.defineProperty(o,
"foo", { enumerable:true, writable:true });
Nit: why care about enumerable in all these tests?

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:41: let defProp = function(o) {
Nit: function defProp(o) {    (same below)

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:51: for (let func of [defProp,
defProps, freeze]) {
Nit: you can just use OBject.freeze here directly

[conradw, 2015-05-22 14:07:08]

https://codereview.chromium.org/1142393003/diff/20001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1142393003/diff/20001/src/messages.h#newcode231
src/messages.h:231: "Cannot redefine non-configurable property '%' of strong
object % to be "  \
On 2015/05/22 13:10:28, rossberg wrote:
> Nit: sylise

Done.

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:810: if (!currentIsWritable) {
On 2015/05/22 13:10:28, rossberg wrote:
> Nit:  if (!currentIsWritable || IS_STRONG(obj))  here

Done. Silly to miss this.

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:812: throw MakeTypeError(kRedefineDisallowed, p);
On 2015/05/22 13:10:28, rossberg wrote:
> ...and then  throw currentIsWritable ? MakeTypeError(...) : MakeTypeError(...)

> here

Done.

https://codereview.chromium.org/1142393003/diff/20001/src/v8natives.js#newcod...
src/v8natives.js:817: if (IS_STRONG(obj)) {
On 2015/05/22 13:10:28, rossberg wrote:
> ...and eliminate this duplication

Done.

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-freeze-property.js (right):

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:21: // Strong functions can't have
properties added to them.
On 2015/05/22 13:10:28, rossberg wrote:
> Nit: indentation

Done.

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:30: let nonStrongObjects =
sloppyObjects.concat(strictObjects);
On 2015/05/22 13:10:28, rossberg wrote:
> Nit: weakObjects

Done.

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:33: Object.defineProperty(o,
"foo", { enumerable:true, writable:true });
On 2015/05/22 13:10:28, rossberg wrote:
> Nit: why care about enumerable in all these tests?

The defineProperties case needs it to work, and I can't let it varying or it
will always throw.

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:41: let defProp = function(o) {
On 2015/05/22 13:10:28, rossberg wrote:
> Nit: function defProp(o) {    (same below)

Done.

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:51: for (let func of [defProp,
defProps, freeze]) {
On 2015/05/22 13:10:28, rossberg wrote:
> Nit: you can just use OBject.freeze here directly

Wanted to give an opportunity for some sort of lowering/rewriting to happen, if
implemented (paranoia).

[rossberg, 2015-05-22 14:15:29]

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-freeze-property.js (right):

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:33: Object.defineProperty(o,
"foo", { enumerable:true, writable:true });
On 2015/05/22 14:07:08, conradw wrote:
> On 2015/05/22 13:10:28, rossberg wrote:
> > Nit: why care about enumerable in all these tests?
> 
> The defineProperties case needs it to work, and I can't let it varying or it
> will always throw.

Actually, I don't understand your use of defineProperties. AFAICT it should be
something like

Object.defineProperties(o, {foo: {writable: false}})

whereas yours is equivalent to

Object.defineProperties(o, {foo: undefined})

which basically does nothing. Perhaps that's why you thought you need enumerable
there?

https://codereview.chromium.org/1142393003/diff/40001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1142393003/diff/40001/src/v8natives.js#newcod...
src/v8natives.js:689: MakeTypeError(kStrongRedefineDisallowed, o, p) :
Nit: style guide wants ? and : each first on the next line, plus indentation by
4

[conradw, 2015-05-27 18:15:45]

For the Object.freeze fix, not confident that forcing strong objects to always
take the slow path is a good solution, so consider this WIP.

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-freeze-property.js (right):

https://codereview.chromium.org/1142393003/diff/20001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:33: Object.defineProperty(o,
"foo", { enumerable:true, writable:true });
On 2015/05/22 14:15:29, rossberg wrote:
> On 2015/05/22 14:07:08, conradw wrote:
> > On 2015/05/22 13:10:28, rossberg wrote:
> > > Nit: why care about enumerable in all these tests?
> > 
> > The defineProperties case needs it to work, and I can't let it varying or it
> > will always throw.
> 
> Actually, I don't understand your use of defineProperties. AFAICT it should be
> something like
> 
> Object.defineProperties(o, {foo: {writable: false}})
> 
> whereas yours is equivalent to
> 
> Object.defineProperties(o, {foo: undefined})
> 
> which basically does nothing. Perhaps that's why you thought you need
enumerable
> there?

Fixed now.

https://codereview.chromium.org/1142393003/diff/40001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/1142393003/diff/40001/src/v8natives.js#newcod...
src/v8natives.js:689: MakeTypeError(kStrongRedefineDisallowed, o, p) :
On 2015/05/22 14:15:29, rossberg wrote:
> Nit: style guide wants ? and : each first on the next line, plus indentation
by
> 4

Done.

[arv (Not doing code reviews), 2015-05-28 15:51:51]

LGTM

It is not clear to me why the fast path does not work?

Can you file a bug about making freeze not use the slow path so that we do not
forget about it later?

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-freeze-property.js (right):

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:22: return [({})];
no need for parens here.

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:30: Object.defineProperty(o,
"foo", { configurable:true, writable:true });
{configurable: true, writable: true}

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:31: assertDoesNotThrow(
assertDoesNotThrow is generally pretty useless... consider changing to an IIFE.

[conradw, 2015-05-28 17:00:06]

Just now, the fast path assumes you never throw, and you don't care much about
the previous attribute values, since before strong mode it was impossible to
fail to set writable/enumerable attribute to false. I think it's possible to
modify the fast path to work, but it requires inspecting the previous value in
more detail than is done currently. Forcing the slow path was an easier fix.

I've added a todo, which is consistent with Andreas' treatment of forcing
normally shallow copied strong arrays to take the slow path.

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
File test/mjsunit/strong/object-freeze-property.js (right):

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:22: return [({})];
On 2015/05/28 15:51:51, arv wrote:
> no need for parens here.

Done.

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:30: Object.defineProperty(o,
"foo", { configurable:true, writable:true });
On 2015/05/28 15:51:51, arv wrote:
> {configurable: true, writable: true}

Done.

https://codereview.chromium.org/1142393003/diff/60001/test/mjsunit/strong/obj...
test/mjsunit/strong/object-freeze-property.js:31: assertDoesNotThrow(
On 2015/05/28 15:51:51, arv wrote:
> assertDoesNotThrow is generally pretty useless... consider changing to an
IIFE.

Since this would throw if it was a strong object, I think it gives better
messaging in the error case that the strong semantics are accidently invoked
here.

[conradw, 2015-05-29 11:24:18]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-05-29 11:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1142393003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-29 11:24:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142393003/100001

[commit-bot: I haz the power, 2015-05-29 11:33:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-29 11:33:41]

Patchset 6 (id:??) landed as
https://crrev.com/6edc3e317912d8ea66bdfc031b06755404e83956
Cr-Commit-Position: refs/heads/master@{#28698}