Propagate exceptions thrown when setting elements.

Propagate exceptions thrown when setting elements.

Plus use more robust path when formatting messages---work
directly with fixed arrays.

BUG=v8:1107
TEST=test/mjsunit/getter-in-prototype.js,test/mjsunit/regress/regress-1107.js

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

[antonm, 2011-02-08 14:05:20]

Lasse and Mads,

may you have a look?

And I'll send enough path which adds checks in CEntry stub what return value and
Top::pending_exception are consistent on coming back to JS code.

[Lasse Reichstein, 2011-02-08 14:17:56]

LGTM

http://codereview.chromium.org/6451004/diff/1/src/messages.cc
File src/messages.cc (right):

http://codereview.chromium.org/6451004/diff/1/src/messages.cc#newcode78
src/messages.cc:78: Factory::NewJSArrayWithElements(arguments_elements);
Is this fixing anything, or is it just a rewrite?

http://codereview.chromium.org/6451004/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6451004/diff/1/src/objects.cc#newcode6967
src/objects.cc:6967: if (result != Heap::the_hole_value()) return result;
Check if the result is a failure instead.
You are comparing pointers of different type here (which is slightly icky, even
if it's true).

http://codereview.chromium.org/6451004/diff/1/src/objects.cc#newcode7102
src/objects.cc:7102: if (result != Heap::the_hole_value()) return result;
Same here. Check result->IsFailure()

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/getter-in-prototyp...
File test/mjsunit/getter-in-prototype.js (right):

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/getter-in-prototyp...
test/mjsunit/getter-in-prototype.js:58: this[0] = 42;
I would think "this" here refers to the global object, not to "o". Can you say
why this works?

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/regress/regress-11...
File test/mjsunit/regress/regress-1107.js (right):

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/regress/regress-11...
test/mjsunit/regress/regress-1107.js:32: assertThrows("x.x");
Should the property name be "x", not 0?

[antonm, 2011-02-08 14:30:21]

Thanks a lot for comments.

http://codereview.chromium.org/6451004/diff/1/src/messages.cc
File src/messages.cc (right):

http://codereview.chromium.org/6451004/diff/1/src/messages.cc#newcode78
src/messages.cc:78: Factory::NewJSArrayWithElements(arguments_elements);
On 2011/02/08 14:17:56, Lasse Reichstein wrote:
> Is this fixing anything, or is it just a rewrite?

It can be omitted, I wrote about that in follow up message, sorry, should have
posted with request for review.

http://codereview.chromium.org/6451004/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6451004/diff/1/src/objects.cc#newcode6967
src/objects.cc:6967: if (result != Heap::the_hole_value()) return result;
On 2011/02/08 14:17:56, Lasse Reichstein wrote:
> Check if the result is a failure instead.
> You are comparing pointers of different type here (which is slightly icky,
even
> if it's true).

Let me double check.  I cannot use plain IsFailure as I need to be able to
distinguish between three outcomes: no setter in proto chain, setting threw,
setting went fine.  I used this hole to signify no setter case.

I can first check it's not a failure and when compare Object to the hole.  I can
too pass a pointer to a flag.

Which one do you prefer?

http://codereview.chromium.org/6451004/diff/1/src/objects.cc#newcode7102
src/objects.cc:7102: if (result != Heap::the_hole_value()) return result;
On 2011/02/08 14:17:56, Lasse Reichstein wrote:
> Same here. Check result->IsFailure()

See above.

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/getter-in-prototyp...
File test/mjsunit/getter-in-prototype.js (right):

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/getter-in-prototyp...
test/mjsunit/getter-in-prototype.js:58: this[0] = 42;
On 2011/02/08 14:17:56, Lasse Reichstein wrote:
> I would think "this" here refers to the global object, not to "o". Can you say
> why this works?

Very good catch.  That works as we set p to be a proto of global object above
(ln. 45).  I think I'll just drop this case and reorganize tests slightly.

Thanks a lot.

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/regress/regress-11...
File test/mjsunit/regress/regress-1107.js (right):

http://codereview.chromium.org/6451004/diff/1/test/mjsunit/regress/regress-11...
test/mjsunit/regress/regress-1107.js:32: assertThrows("x.x");
On 2011/02/08 14:17:56, Lasse Reichstein wrote:
> Should the property name be "x", not 0?

Copy paste from original report.  Actually, it uses x just to throw an
exception.  I can simplify it to "x" instead of "x.x".  Will do

[Mads Ager (chromium), 2011-02-08 18:59:11]

LGTM when this comment is addressed.

http://codereview.chromium.org/6451004/diff/6002/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6451004/diff/6002/src/objects.cc#newcode1728
src/objects.cc:1728: return Heap::the_hole_value();
Can we do an output parameter called something like |found| instead?

Then the callers can do:

bool called_setter;
MaybeObject* result = SetElementWithCallbackSetterInPrototypes(...,
&called_setter);
if (called_setter) return result;

http://codereview.chromium.org/6451004/diff/6002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6451004/diff/6002/src/objects.h#newcode1582
src/objects.h:1582: MUST_USE_RESULT MaybeObject*
Can you split the line after the name and put the arguments on the new line or
is that too long?

[antonm, 2011-02-08 19:36:02]

Submitting

http://codereview.chromium.org/6451004/diff/6002/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6451004/diff/6002/src/objects.cc#newcode1728
src/objects.cc:1728: return Heap::the_hole_value();
On 2011/02/08 18:59:11, Mads Ager wrote:
> Can we do an output parameter called something like |found| instead?
> 
> Then the callers can do:
> 
> bool called_setter;
> MaybeObject* result = SetElementWithCallbackSetterInPrototypes(...,
> &called_setter);
> if (called_setter) return result;

Done.

http://codereview.chromium.org/6451004/diff/6002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6451004/diff/6002/src/objects.h#newcode1582
src/objects.h:1582: MUST_USE_RESULT MaybeObject*
On 2011/02/08 18:59:11, Mads Ager wrote:
> Can you split the line after the name and put the arguments on the new line or
> is that too long?

Done.