First cut at bug 992

First cut at bug 992

Fixes JS portion of DefineOwnProperty when there is
an existing property and the new descriptor is generic.

Makes code follow spec steps more closely.

Fixes typo for check for unchanged enumerable in step 6.

Adds regression test.

BUG=
TEST=

[Peter Hallam, 2011-01-04 22:33:51]

Hello Rico, it looks like we've both been working on DefineOwnProperty. Does
this look reasonable?

[Rico, 2011-01-05 13:37:01]

Hi Peter,

Thanks a lot for the patch.

In addition to the in file comments it would be great if we could add tests to
test/mjsunit/object-define-property.js that exercises all the possible paths
(i.e., that this works in all combinations (accessor with only a setter,
accessor with only a getter, accessor with bot getter and setter, data property
combined with the different descriptors that can be given (combinations of
enumerable, writable, configurable)).
I know that this a bit of work, but some of the many existing tests in
object-define-property have given early warnings of bugs before when changing
various parts of the property functionality in objects.cc and runtime.cc)

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3496
src/runtime.cc:3496: // Implements part of 8.12.9 DefinePropertyAccessor
DefinePropertyAccessor -> DefineOwnProperty

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3498
src/runtime.cc:3498: // Step 4b - defining a new accessor property
Period at end of the comment + below and above

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3533
src/runtime.cc:3533: // Implements part of 8.12.9 DefineOwnProperty
Period at end of comment + below

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode550
src/v8natives.js:550: if ((IsGenericDescriptor(desc) ||
Remove space at the end of the line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode568
src/v8natives.js:568: if (desc.isConfigurable() ||
Remove space at the end of the line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode569
src/v8natives.js:569: (desc.hasEnumerable() &&
Remove space at the end of the line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode618
src/v8natives.js:618: if (IsDataDescriptor(desc) ||
Remove space at the end of the line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode619
src/v8natives.js:619: (IsGenericDescriptor(desc) &&
Remove space at the end of the line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode622
src/v8natives.js:622: // Step 4a - defining a new data property
Period at end of comment + below

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode623
src/v8natives.js:623: // Steps 9b & 12 - replacing an existing accessor property
with a data
Remove space at the end of the line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode642
src/v8natives.js:642: 
Remove spaces from the empty line

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode644
src/v8natives.js:644: } else {
How about:
} else if (IsGenericDescriptor(desc)){
    // Update a genereic accessor with a generic descriptor.
    %DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(), flag);
  } else {
    // There are 3 cases that lead here:
    // Step 4b - defining a new accessor property.
    // Steps 9c & 12 - replacing an existing data property with an accessor
    //                 property.
    // Step 12 - updating an existing accessor property with an accessor.

    if (desc.hasGetter()) {
      %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
    }

    if (desc.hasSetter()) {
      %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
    }
  }

This saves us an extra call to runtime in case a generic descriptor is passed or
if we are defining or redefining either just the setter or just the getter

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode646
src/v8natives.js:646: // Step 4b - defining a new accessor property
period at the end of the comment + below

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:765: var descAccessor = { get: function()
{ return 0; } };
could we also try with descElementNonWritable, which should also throw

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:855: var descAccessor = { get: function()
{ return 0; } };
Again, could we also try with descElementNonWritable and descNonEnumerable which
should both throw

[Rico, 2011-01-05 13:40:39]

Two additional (very) small nits

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:753: // configurable property
period at end of comment

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:763: // the non configurable property.
a the -> a

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:843: // configurable property of an array
period at end of comment

[Peter Hallam, 2011-01-05 23:50:31]

Thanks for the code review Rico.

This update addresses all the in file comments. I added the 2 test cases that
you mentioned in the in file comments, but they don't throw. This was a surprise
to me as well, but the spec is clear. I confirmed with an email thread to
es-discuss. I've attached the thread below.

I agree that more tests in object-define-property.js would be a good thing. I'm
going on vacation in a couple of hours so I thought I'd upload what I have. I'll
resume this when I'm back next week.

Cheers,
Peter

From es-discuss:

On Wed, Jan 5, 2011 at 2:09 PM, Allen Wirfs-Brock <allen@wirfs-brock.com> wrote:

Yes, it is intentional and was allowed to enable the "freezing" of objects that
already had non-configurable properties.  MarkM can probably provide some
specific use cases in the context of secure sandboxes.

Follow up from MarkM:

According to the ES5 spec (I haven't double checked against ES5.1), the
following properties are initialized to {writable: true, configurable: false}:

    13.2 step 18, 15.3.5.2
    aFunction.prototype

    15.4.5.2
    anArray.length

    15.10.7.5
    aRegExp.lastIndex

However, we need to be able to freeze functions, arrays, and regExps. Were we
willing to make the above properties configurable: true, then we wouldn't have
needed this loophole in the spec. Given that we were not willing to make these
properties configurable, this loophole was the least unpleasant way we could
find to still allow us to freeze such objects. (Another approach that was
mentioned at the time and promptly rejected: introducing yet another attribute
flag to distinguish not-deletable-but-freezable from non-configurable.)

I think there were some other cases as well, but can't find them right now. No
matter, the above cases are sufficient.

In addition, if I read the spec right, the following code

    15.5.5.2
    Object.getOwnPropertyDescriptor(Object("foo"), '1')

should return a descriptor claiming in effect that the (non-existent) '1'
property on the String object is writable but not configurable. Is this a spec
error? Is it already recorded as an errata? Cc'ing es5-discuss just in case.

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3496
src/runtime.cc:3496: // Implements part of 8.12.9 DefinePropertyAccessor
On 2011/01/05 13:37:01, Rico wrote:
> DefinePropertyAccessor -> DefineOwnProperty

Done.

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3498
src/runtime.cc:3498: // Step 4b - defining a new accessor property
On 2011/01/05 13:37:01, Rico wrote:
> Period at end of the comment + below and above

Done.

http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3533
src/runtime.cc:3533: // Implements part of 8.12.9 DefineOwnProperty
On 2011/01/05 13:37:01, Rico wrote:
> Period at end of comment + below

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode550
src/v8natives.js:550: if ((IsGenericDescriptor(desc) ||
On 2011/01/05 13:37:01, Rico wrote:
> Remove space at the end of the line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode568
src/v8natives.js:568: if (desc.isConfigurable() ||
On 2011/01/05 13:37:01, Rico wrote:
> Remove space at the end of the line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode569
src/v8natives.js:569: (desc.hasEnumerable() &&
On 2011/01/05 13:37:01, Rico wrote:
> Remove space at the end of the line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode618
src/v8natives.js:618: if (IsDataDescriptor(desc) ||
On 2011/01/05 13:37:01, Rico wrote:
> Remove space at the end of the line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode619
src/v8natives.js:619: (IsGenericDescriptor(desc) &&
On 2011/01/05 13:37:01, Rico wrote:
> Remove space at the end of the line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode622
src/v8natives.js:622: // Step 4a - defining a new data property
On 2011/01/05 13:37:01, Rico wrote:
> Period at end of comment + below

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode623
src/v8natives.js:623: // Steps 9b & 12 - replacing an existing accessor property
with a data
On 2011/01/05 13:37:01, Rico wrote:
> Remove space at the end of the line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode642
src/v8natives.js:642: 
On 2011/01/05 13:37:01, Rico wrote:
> Remove spaces from the empty line

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode644
src/v8natives.js:644: } else {
On 2011/01/05 13:37:01, Rico wrote:
> How about:
> } else if (IsGenericDescriptor(desc)){
>     // Update a genereic accessor with a generic descriptor.
>     %DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(), flag);
>   } else {
>     // There are 3 cases that lead here:
>     // Step 4b - defining a new accessor property.
>     // Steps 9c & 12 - replacing an existing data property with an accessor
>     //                 property.
>     // Step 12 - updating an existing accessor property with an accessor.
> 
>     if (desc.hasGetter()) {
>       %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
>     }
> 
>     if (desc.hasSetter()) {
>       %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
>     }
>   }
> 
> This saves us an extra call to runtime in case a generic descriptor is passed
or
> if we are defining or redefining either just the setter or just the getter

Done.

http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode646
src/v8natives.js:646: // Step 4b - defining a new accessor property
On 2011/01/05 13:37:01, Rico wrote:
> period at the end of the comment + below

Done.

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:753: // configurable property
On 2011/01/05 13:40:39, Rico wrote:
> period at end of comment

Done.

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:763: // the non configurable property.
On 2011/01/05 13:40:39, Rico wrote:
> a the -> a

Done.

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:765: var descAccessor = { get: function()
{ return 0; } };
I've added a test for descElementNonWritable. Interestingly enough it does not
throw. I confirmed with a thread on es-discuss.

On 2011/01/05 13:37:01, Rico wrote:
> could we also try with descElementNonWritable, which should also throw

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:843: // configurable property of an array
On 2011/01/05 13:40:39, Rico wrote:
> period at end of comment

Done.

http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-p...
test/mjsunit/object-define-property.js:855: var descAccessor = { get: function()
{ return 0; } };
On 2011/01/05 13:37:01, Rico wrote:
> Again, could we also try with descElementNonWritable and descNonEnumerable
which
> should both throw

Done. See comment above.

[Rico, 2011-01-07 09:47:30]

LGTM
I had not considered the possibility of being able to change writable to false
even with configurable set to false. I saw the discussion on es-discuss and that
is a somehow valid point (although it intuitively does not make a lot of sense).
But we should of course follow the spec here.

http://codereview.chromium.org/6035014/diff/10001/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/6035014/diff/10001/src/v8natives.js#newcode645
src/v8natives.js:645: // Step 12 - updating an existing accessor property with
generic
with generic -> with a generic

[Rico, 2011-01-07 11:06:49]

Hi Peter,
I will land this for you with the nit (since you do not have commit rights).
Lets put the additional tests in another patch and get this in now. Thanks a lot
for the effort.

Cheers,
Rico

On 2011/01/07 09:47:30, Rico wrote:
> LGTM
> I had not considered the possibility of being able to change writable to false
> even with configurable set to false. I saw the discussion on es-discuss and
that
> is a somehow valid point (although it intuitively does not make a lot of
sense).
> But we should of course follow the spec here.
> 
> http://codereview.chromium.org/6035014/diff/10001/src/v8natives.js
> File src/v8natives.js (right):
> 
> http://codereview.chromium.org/6035014/diff/10001/src/v8natives.js#newcode645
> src/v8natives.js:645: // Step 12 - updating an existing accessor property with
> generic
> with generic -> with a generic

[Rico, 2011-01-07 12:02:12]

Landed in revision 6220 
http://code.google.com/p/v8/source/detail?r=6220


On 2011/01/07 11:06:49, Rico wrote:
> Hi Peter,
> I will land this for you with the nit (since you do not have commit rights).
> Lets put the additional tests in another patch and get this in now. Thanks a
lot
> for the effort.
> 
> Cheers,
> Rico
> 
> On 2011/01/07 09:47:30, Rico wrote:
> > LGTM
> > I had not considered the possibility of being able to change writable to
false
> > even with configurable set to false. I saw the discussion on es-discuss and
> that
> > is a somehow valid point (although it intuitively does not make a lot of
> sense).
> > But we should of course follow the spec here.
> > 
> > http://codereview.chromium.org/6035014/diff/10001/src/v8natives.js
> > File src/v8natives.js (right):
> > 
> >
http://codereview.chromium.org/6035014/diff/10001/src/v8natives.js#newcode645
> > src/v8natives.js:645: // Step 12 - updating an existing accessor property
with
> > generic
> > with generic -> with a generic

[Peter Hallam, 2011-01-29 00:56:59]

Hey Rico,

this version passes webkit layout test
http/tests/security/xss-DENIED-defineProperty.html.

The issue was that when going through the SetLocalPropertyIgnoreAttributes path,
the object is first Normalized. This breaks when the object is a JSGlobalProxy -
Normalization copies properties from the GlobalObject to the Proxy which allows
the properties to be accessed without security checks and also breaks the
invariant that JSGlobalPRoxy should never have properties on it directly.

[Rico, 2011-02-01 07:04:54]

LGTM, thanks a lot for fixing this Peter.

Since this also fixes 1083 we should add a regression test for that as well.

http://codereview.chromium.org/6035014/diff/21001/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-992.js (right):

http://codereview.chromium.org/6035014/diff/21001/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-992.js:29: // should just update
enumerable/configurable flags
Period at end of comment

http://codereview.chromium.org/6035014/diff/21001/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-992.js:33: desc =
Object.getOwnPropertyDescriptor(obj, 'p');
var desc = Object.getOwnPropertyDescriptor(obj, 'p');

[Peter Hallam, 2011-02-01 14:45:38]

Addresses review comments and adds regression test for 1083.

http://codereview.chromium.org/6035014/diff/21001/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-992.js (right):

http://codereview.chromium.org/6035014/diff/21001/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-992.js:29: // should just update
enumerable/configurable flags
On 2011/02/01 07:04:55, Rico wrote:
> Period at end of comment

Done.

http://codereview.chromium.org/6035014/diff/21001/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-992.js:33: desc =
Object.getOwnPropertyDescriptor(obj, 'p');
On 2011/02/01 07:04:55, Rico wrote:
> var desc = Object.getOwnPropertyDescriptor(obj, 'p');

Done.