Make sure that we don't actually overwrite a property that has failed access ...

Make sure that we don't actually overwrite a property that has failed access checsk with Object.defineProperty.


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

[antonm, 2011-02-04 11:41:32]

LGTM, but I'd be really glad to see pure v8 test for this stuff to catch
regressions sooner.

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js#newcode497
src/v8natives.js:497: function ConvertDescriptorArrayToDescriptor(desc_array) {
maybe you want to add something like assert(desc_array !== false) (in the form
if (desc_array === false) throw 'Internal error: invalid desc_array)

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js#newcode539
src/v8natives.js:539: // If p is not a property on obj undefined is returned.
you might want to clarify what returned false means

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js#newcode551
src/v8natives.js:551: if (current_or_access == false) return void 0;
===?

[Rico, 2011-02-04 12:14:18]

Addressed comments, I will make another change with a regression test to get
this in and make the tree green.

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js#newcode497
src/v8natives.js:497: function ConvertDescriptorArrayToDescriptor(desc_array) {
On 2011/02/04 11:41:33, antonm wrote:
> maybe you want to add something like assert(desc_array !== false) (in the form
> if (desc_array === false) throw 'Internal error: invalid desc_array)

Done.

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js#newcode539
src/v8natives.js:539: // If p is not a property on obj undefined is returned.
On 2011/02/04 11:41:33, antonm wrote:
> you might want to clarify what returned false means

Done.

http://codereview.chromium.org/6246103/diff/3006/src/v8natives.js#newcode551
src/v8natives.js:551: if (current_or_access == false) return void 0;
On 2011/02/04 11:41:33, antonm wrote:
> ===?

No, this will not return true on the return value (because we return the wrapper
boolean)

[antonm, 2011-02-04 12:16:23]

Sweet.  Still LGTM