Fixes issue 712 causing non-configurable accessors to be overwritable by usin...

Fixes issue 712 causing non-configurable accessors to be overwritable by using
Object.defineProperty with empty property descriptor.

The issue is fixed by implementing step 5 and 6 from DefineOwnProperty in the
specification (ES5 8.12.9).

This also fixes a bug in SameValue when used on boolean values (it
would priorly return a number - not a boolean).


Committed: http://code.google.com/p/v8/source/detail?r=4708

[Rico, 2010-05-20 13:28:05]

Ugly conditional with a lot of comparisons - I don't have an easy solution to
make this more readable.

[Erik Corry, 2010-05-20 14:08:39]

LGTM with comments.

http://codereview.chromium.org/2131019/diff/1/2
File src/runtime.js (right):

http://codereview.chromium.org/2131019/diff/1/2#newcode566
src/runtime.js:566: if (x === 0 && y === 0 && !%_IsSmi(x) && !%_IsSmi(y) &&
Can this be right?  They both have to be non-Smi to get into this 'if'?

http://codereview.chromium.org/2131019/diff/1/2#newcode573
src/runtime.js:573: if (IS_BOOLEAN(x))return y == x;
Missing space after )

http://codereview.chromium.org/2131019/diff/1/3
File src/v8natives.js (right):

http://codereview.chromium.org/2131019/diff/1/3#newcode545
src/v8natives.js:545: // Step 5
Step 5 seems to be a part of Step 6, so you can just omit it, right?

http://codereview.chromium.org/2131019/diff/1/3#newcode562
src/v8natives.js:562: SameValue(desc.getSet(), current.getSet()))) return true;
Multiline 'if's should use {}.

http://codereview.chromium.org/2131019/diff/1/4
File test/mjsunit/regress/regress-712.js (right):

http://codereview.chromium.org/2131019/diff/1/4#newcode28
test/mjsunit/regress/regress-712.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=712
Let's have a short description here too.  It's annoying to have to look up and I
can easily imagine the V8 source code outliving code.google.com URLs.

http://codereview.chromium.org/2131019/diff/1/4#newcode35
test/mjsunit/regress/regress-712.js:35: assertEquals(obj.x, "42");
This test looks a bit minimal.  Perhaps it should be a separate test, but I'd
like to see a test designed to break out of the big 'step 6' if statement at
every disjunction.  Also it should test the +-0 line in the IsSame function.

[Rico, 2010-05-21 12:18:59]

Erik, could you take another look, I changed it quite a bit + added a bunch of
new tests.
By the way, we could consider making it possible to call js functions that are
not made public through InstallFunctions ,if a flag is set in the unit test file
(e.g., Flags: expose_internal_js as internal).
This would make it a lot easier to test methods such as SameValue instead of
doing it indirectly through another method.

http://codereview.chromium.org/2131019/diff/1/2
File src/runtime.js (right):

http://codereview.chromium.org/2131019/diff/1/2#newcode566
src/runtime.js:566: if (x === 0 && y === 0 && !%_IsSmi(x) && !%_IsSmi(y) &&
On 2010/05/20 14:08:39, Erik Corry wrote:
> Can this be right?  They both have to be non-Smi to get into this 'if'?

Right - changed it to be boolean or instead on the smi checks. In addition, I
simplified it since we can simple check if the two infinities are the same.

http://codereview.chromium.org/2131019/diff/1/2#newcode573
src/runtime.js:573: if (IS_BOOLEAN(x))return y == x;
On 2010/05/20 14:08:39, Erik Corry wrote:
> Missing space after )

Done.

http://codereview.chromium.org/2131019/diff/1/3
File src/v8natives.js (right):

http://codereview.chromium.org/2131019/diff/1/3#newcode545
src/v8natives.js:545: // Step 5
On 2010/05/20 14:08:39, Erik Corry wrote:
> Step 5 seems to be a part of Step 6, so you can just omit it, right?

You are absolutely right - thanks.

http://codereview.chromium.org/2131019/diff/1/3#newcode562
src/v8natives.js:562: SameValue(desc.getSet(), current.getSet()))) return true;
On 2010/05/20 14:08:39, Erik Corry wrote:
> Multiline 'if's should use {}.

Done.

http://codereview.chromium.org/2131019/diff/1/4
File test/mjsunit/regress/regress-712.js (right):

http://codereview.chromium.org/2131019/diff/1/4#newcode28
test/mjsunit/regress/regress-712.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=712
On 2010/05/20 14:08:39, Erik Corry wrote:
> Let's have a short description here too.  It's annoying to have to look up and
I
> can easily imagine the V8 source code outliving http://code.google.com URLs.

Done.

http://codereview.chromium.org/2131019/diff/1/4#newcode35
test/mjsunit/regress/regress-712.js:35: assertEquals(obj.x, "42");
On 2010/05/20 14:08:39, Erik Corry wrote:
> This test looks a bit minimal.  Perhaps it should be a separate test, but I'd
> like to see a test designed to break out of the big 'step 6' if statement at
> every disjunction.  Also it should test the +-0 line in the IsSame function.
I have added these tests to the object-define-property mjsunit test since they
are not part of this bug and I guess it is better to put them there.

[Erik Corry, 2010-05-21 13:16:16]

You can run the test with --expose-natives-as foobar

Then you can do foobar.SameValue to get the function you need.

See for example test/mjsunit/global-deleted-property-keyed.js

[Erik Corry, 2010-05-21 13:21:57]

LGTM

http://codereview.chromium.org/2131019/diff/9001/10003
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/2131019/diff/9001/10003#newcode511
test/mjsunit/object-define-property.js:511: // Test that all possible
differences in step 6 in DefineOwnProperty is
differences ... is -> differences ... are

http://codereview.chromium.org/2131019/diff/9001/10003#newcode689
test/mjsunit/object-define-property.js:689: // Make sure that we can't overwrite
+0 with -0 and vise verca.
vise verca -> vice versa

(or was that deliberate :-)

[Rico, 2010-05-25 06:24:50]

http://codereview.chromium.org/2131019/diff/9001/10003
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/2131019/diff/9001/10003#newcode511
test/mjsunit/object-define-property.js:511: // Test that all possible
differences in step 6 in DefineOwnProperty is
On 2010/05/21 13:21:57, Erik Corry wrote:
> differences ... is -> differences ... are

Done.

http://codereview.chromium.org/2131019/diff/9001/10003#newcode689
test/mjsunit/object-define-property.js:689: // Make sure that we can't overwrite
+0 with -0 and vise verca.
On 2010/05/21 13:21:57, Erik Corry wrote:
> vise verca -> vice versa
> 
> (or was that deliberate :-)
No :-) done