Fix bug in deletion of indexed properties

Fix bug in deletion of indexed properties

The delete operator always return true in case of indexed property. It
should return false if an indexed property can't be deleted (eg.
DontDelete attribute is set or a string object is the holder).

Contributed by Peter Varga <pvarga@inf.u-szeged.hu>;

BUG=none
TEST=mjsunit/delete-non-configurable

Committed: https://code.google.com/p/v8/source/detail?r=12736

[Michael Starzinger, 2012-10-10 17:10:10]

The CL generally looks good and also the test coverage is fine. But I think that
a mjsunit test using Object.defineProperty() would be easier to read while
giving similar coverage (just without the forced deletion). Would it be possible
for you to rewrite the current tests in JavaScript and put them in e.g.
mjsunit/delete-non-configurable.js (or a similar name). Also tests for strict
mode behavior might be useful in there.

[Michael Starzinger, 2012-10-15 15:09:33]

LGTM. I'll land this for you. Thanks for the contribution!

[Michael Starzinger, 2012-10-15 15:20:39]

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946
src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) {
This was moved from DeleteElement() to DeleteProperty(), I guess that was an
accident and I'll move it back.

[Peter Varga, 2012-10-15 15:25:34]

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946
src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) {
This is on purpose. Based on ECMA standard it should throw an exception in
strict mode.

[Peter Varga, 2012-10-15 15:30:12]

See section 15.5.5.1
"It (String object) has the attributes { [[Writable]]: false, [[Enumerable]]:
false, [[Configurable]]: false }."

and see section 11.4.1
"In addition, if a delete operator occurs within strict mode code and the
property to be deleted has the attribute { [[Configurable]]: false }, a
TypeError exception is thrown."

This case is tested in the new test.

[Michael Starzinger, 2012-10-15 15:30:18]

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946
src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) {
On 2012/10/15 15:25:34, Peter Varga wrote:
> This is on purpose. Based on ECMA standard it should throw an exception in
> strict mode.

I totally agree but that wasn't the point of my comment. My comment is about
this being in JSObject::DeleteProperty() where we didn't yet convert the
property name into an index. That happens further down and the call is delegated
to JSObject::DeleteElement() in those cases. It only makes sense to check
whether we are accessing a character in a string after that point. Also it just
doesn't compile here! :)

[Peter Varga, 2012-10-15 15:48:23]

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946
src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) {
I see the problem now. It seems git applied the patch wrongly.