Make exception thrown via v8 public API propagate to v8::TryCatch as JS thrown exceptions do.

Make exception thrown via v8 public API propagate to v8::TryCatch as JS thrown exceptions do.

Correctly process failures which can be returned by Object::GetProperty
when performing GetRealNamedProperty* queries.

Callback properties can produce exceptions so we need to wrap access to them
into exception checks.  However, despite of many other methods with exception
checks, property access doesn't mandatroy go via JavaScript and hence we
need to inject code to propagate exception to public API TryCatch handlers.

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

[antonm, 2011-01-28 10:10:28]

Mads and Vitaly,

may you have a look?

This patch requires some adjustments in V8 bindings for layout tests to pass,
but those changes appear reasonable and save to one rebaseline can go in even
without this patch.

When this patch gets in better shape, I'll commit CL for WebKit before this
change hits the trunk.

I haven't added additional checks yet (ones we discussed): I have almost working
patch which unifies pending and scheduled exceptions and will do it as part of
this work.  However, I'd rather stage this patch for some time on DevChannel
before messing with the things.

[Mads Ager (chromium), 2011-01-28 11:39:18]

http://codereview.chromium.org/6397011/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode2585
src/api.cc:2585: has_pending_exception = value->IsFailure();
Should you test for value->IsException instead and assert that it cannot be
OutOfMemory or RetryAfterGC?

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

http://codereview.chromium.org/6397011/diff/1/src/messages.cc#newcode140
src/messages.cc:140: Top::clear_pending_exception();
Should this be moved to the caller? I think we only call this if there is
pending exception. In the caller I think we always set the pending_exception
back at the end. So, in the caller, maybe we should clear the pending exception
before calling this?

If not, I think the comment should be extended to state that this is only called
from a place where the pending exception is restored on return?

Ignoring new exceptions while report the current one seems like the right thing
to do.

http://codereview.chromium.org/6397011/diff/1/src/top.cc
File src/top.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode982
src/top.cc:982: 
Clear pending exception here instead of in ReportMessage? It is restored
correctly below?

http://codereview.chromium.org/6397011/diff/1/src/top.h
File src/top.h (right):

http://codereview.chromium.org/6397011/diff/1/src/top.h#newcode457
src/top.h:457: static void PropagatePendingExceptionToExteranlTryCatch();
Exteranl -> External

[antonm, 2011-01-28 13:37:24]

http://codereview.chromium.org/6397011/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode2585
src/api.cc:2585: has_pending_exception = value->IsFailure();
On 2011/01/28 11:39:18, Mads Ager wrote:
> Should you test for value->IsException instead and assert that it cannot be
> OutOfMemory or RetryAfterGC?

I can, but not sure if it's a good idea.  Plus technically we can probably have
OOM here if callback is JS thing which exhausts the memory.  I hope that
EXCEPTION_ macros should cope with any kind of failure.

Am I missing something?

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

http://codereview.chromium.org/6397011/diff/1/src/messages.cc#newcode140
src/messages.cc:140: Top::clear_pending_exception();
Let me try modified second approach: I'll move all the logic of thread_local_
save and restore over here, does that look reasonable?

On 2011/01/28 11:39:18, Mads Ager wrote:
> Should this be moved to the caller? I think we only call this if there is
> pending exception. In the caller I think we always set the pending_exception
> back at the end. So, in the caller, maybe we should clear the pending
exception
> before calling this?
> 
> If not, I think the comment should be extended to state that this is only
called
> from a place where the pending exception is restored on return?
> 
> Ignoring new exceptions while report the current one seems like the right
thing
> to do.

http://codereview.chromium.org/6397011/diff/1/src/top.cc
File src/top.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode982
src/top.cc:982: 
On 2011/01/28 11:39:18, Mads Ager wrote:
> Clear pending exception here instead of in ReportMessage? It is restored
> correctly below?

See above.

http://codereview.chromium.org/6397011/diff/1/src/top.h
File src/top.h (right):

http://codereview.chromium.org/6397011/diff/1/src/top.h#newcode457
src/top.h:457: static void PropagatePendingExceptionToExteranlTryCatch();
On 2011/01/28 11:39:18, Mads Ager wrote:
> Exteranl -> External

Done.

[Vitaly Repeshko, 2011-01-28 14:03:33]

Thanks for looking into this stuff! I have a bunch of questions for now and I'll
need another pass.


-- Vitaly

http://codereview.chromium.org/6397011/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode1411
src/api.cc:1411: i::Object* exception =
reinterpret_cast<i::Object*>(exception_);
Hmm, how is this supposed to work with a raw pointer? Can't a GC happen before
we read it?

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

http://codereview.chromium.org/6397011/diff/1/src/messages.cc#newcode151
src/messages.cc:151: // Consider logging it somehow.
What if we get an OOM or termination exception here? There are probably not safe
to ignore.

http://codereview.chromium.org/6397011/diff/1/src/top.cc
File src/top.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode936
src/top.cc:936: thread_local_.TryCatchHandler()->can_continue_ = false;
try_catch_handler() is a nicer alias for thread_local_.TryCatchHandler().

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode942
src/top.cc:942: Handle<Object> exception(pending_exception_object);
Unused? If it is intended to be used, shouldn't we allocate a new handle scope
for it?

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode953
src/top.cc:953: void Top::ReportPendingMessages() {
It reports only a single message, doesn't it? Drop the trailing "s"?

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode955
src/top.cc:955: setup_external_caught();
This looks slightly backwards: one function sets a global flag and the other
checks it. Can we convert the first one to a predicate without side effects
(IsPendingExceptionCaughtExternally) and use its return value?

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode970
src/top.cc:970: if (thread_local_.has_pending_message_) {
Join with "else" above?

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode978
src/top.cc:978: // TODO(antonm): consider saving the whole thread_local_.
I don't think we should save the whole thread local state (even if it works
now). Maybe instead extract a smaller sub-object holding all the exception
related stuff?

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode982
src/top.cc:982: 
On 2011/01/28 11:39:18, Mads Ager wrote:
> Clear pending exception here instead of in ReportMessage? It is restored
> correctly below?

I agree. This logic should be all moved here.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode1040
src/top.cc:1040: if (it.done() || (it.frame()->sp() > external_handler_address))
{
Does this work correctly under ARM simulator?

[antonm, 2011-01-28 14:43:18]

Thanks a lot for comments, Vitaly

http://codereview.chromium.org/6397011/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode1411
src/api.cc:1411: i::Object* exception =
reinterpret_cast<i::Object*>(exception_);
On 2011/01/28 14:03:33, Vitaly wrote:
> Hmm, how is this supposed to work with a raw pointer? Can't a GC happen before
> we read it?

Take a look at Top::Iterate---it turns exception_ in all trycatches roots if I
am reading the code correctly.

Or are you asking something else?

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

http://codereview.chromium.org/6397011/diff/1/src/messages.cc#newcode151
src/messages.cc:151: // Consider logging it somehow.
On 2011/01/28 14:03:33, Vitaly wrote:
> What if we get an OOM or termination exception here? There are probably not
safe
> to ignore.

That sounds like a very good point.  I think we should be more or less safe for
OOM which will mark the context, but termination exception might be clobbered
here.

But let's investigate and address it later, ok?  Before my change throwing
exception from callbacks would just crash/trigger asserts in v8 anyway.

http://codereview.chromium.org/6397011/diff/1/src/top.cc
File src/top.cc (right):

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode936
src/top.cc:936: thread_local_.TryCatchHandler()->can_continue_ = false;
On 2011/01/28 14:03:33, Vitaly wrote:
> try_catch_handler() is a nicer alias for thread_local_.TryCatchHandler().

Done.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode942
src/top.cc:942: Handle<Object> exception(pending_exception_object);
On 2011/01/28 14:03:33, Vitaly wrote:
> Unused? If it is intended to be used, shouldn't we allocate a new handle scope
> for it?

Thanks a lot, indeed unused.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode953
src/top.cc:953: void Top::ReportPendingMessages() {
On 2011/01/28 14:03:33, Vitaly wrote:
> It reports only a single message, doesn't it? Drop the trailing "s"?

I don't know.  Let's investigate it later.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode955
src/top.cc:955: setup_external_caught();
On 2011/01/28 14:03:33, Vitaly wrote:
> This looks slightly backwards: one function sets a global flag and the other
> checks it. Can we convert the first one to a predicate without side effects
> (IsPendingExceptionCaughtExternally) and use its return value?

I don't want to rework the things too much.  I agree that this function is weird
and even thought about getting rid of it altogether and merging it into
Propagate..., but as far as I can, I'd like to keep the change small.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode970
src/top.cc:970: if (thread_local_.has_pending_message_) {
On 2011/01/28 14:03:33, Vitaly wrote:
> Join with "else" above?

I'd like to keep identical structure of failure analysis (OOM, termination,
regular exception).  But I can merge if you insist.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode978
src/top.cc:978: // TODO(antonm): consider saving the whole thread_local_.
On 2011/01/28 14:03:33, Vitaly wrote:
> I don't think we should save the whole thread local state (even if it works
> now). Maybe instead extract a smaller sub-object holding all the exception
> related stuff?

Yep, that's why I left it as todo (there is this fancy stuff like SaveContext,
etc.)

For now I just introduced helper Scope subclass (see next patch) which stores
necessary information.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode982
src/top.cc:982: 
On 2011/01/28 14:03:33, Vitaly wrote:
> On 2011/01/28 11:39:18, Mads Ager wrote:
> > Clear pending exception here instead of in ReportMessage? It is restored
> > correctly below?
> 
> I agree. This logic should be all moved here.

See patch 2.

http://codereview.chromium.org/6397011/diff/1/src/top.cc#newcode1040
src/top.cc:1040: if (it.done() || (it.frame()->sp() > external_handler_address))
{
On 2011/01/28 14:03:33, Vitaly wrote:
> Does this work correctly under ARM simulator?

I guess so, there is a special translation for simulator (the address is
actually a fake as C++ and JS stacks a different)

[Vitaly Repeshko, 2011-02-01 19:07:17]

LGTM with a few more comments.

http://codereview.chromium.org/6397011/diff/13001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/13001/src/api.cc#newcode2581
src/api.cc:2581: receiver->GetProperty(*receiver, lookup, *name, &attributes);
Wrap this in CALL_HEAP_FUNCTION?

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc
File src/messages.cc (right):

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc#newcode134
src/messages.cc:134: Top::Scope top_scope;
I don't like the name. ExceptionScope?
Also given that we explicitly manipulate some of its fields here, maybe we
should just inline this object into this function.

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc#newcode145
src/messages.cc:145: // Consider logging it somehow.
Please file a bug to not forget about "scary" exceptions here.

http://codereview.chromium.org/6397011/diff/13001/src/top.h
File src/top.h (right):

http://codereview.chromium.org/6397011/diff/13001/src/top.h#newcode539
src/top.h:539: Scope() :
Does it lint?

http://codereview.chromium.org/6397011/diff/13001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6397011/diff/13001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:8016: CHECK(!withJsGetter.IsEmpty());
nit: Avoid camel case.

[antonm, 2011-02-01 20:14:39]

Hopefully all the comments are addressed.

I am not going to submit it right now---first I need WebKit counterpart to be
submitted.

Thanks a lot for review, Mads and Vitaly.

http://codereview.chromium.org/6397011/diff/13001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/13001/src/api.cc#newcode2581
src/api.cc:2581: receiver->GetProperty(*receiver, lookup, *name, &attributes);
On 2011/02/01 19:07:17, Vitaly wrote:
> Wrap this in CALL_HEAP_FUNCTION?

Done.

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc
File src/messages.cc (right):

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc#newcode134
src/messages.cc:134: Top::Scope top_scope;
On 2011/02/01 19:07:17, Vitaly wrote:
> I don't like the name. ExceptionScope?
> Also given that we explicitly manipulate some of its fields here, maybe we
> should just inline this object into this function.

Renamed to ExceptionScope.

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc#newcode145
src/messages.cc:145: // Consider logging it somehow.
On 2011/02/01 19:07:17, Vitaly wrote:
> Please file a bug to not forget about "scary" exceptions here.

Will do.

http://codereview.chromium.org/6397011/diff/13001/src/top.h
File src/top.h (right):

http://codereview.chromium.org/6397011/diff/13001/src/top.h#newcode539
src/top.h:539: Scope() :
On 2011/02/01 19:07:17, Vitaly wrote:
> Does it lint?

Yes

http://codereview.chromium.org/6397011/diff/13001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6397011/diff/13001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:8016: CHECK(!withJsGetter.IsEmpty());
On 2011/02/01 19:07:17, Vitaly wrote:
> nit: Avoid camel case.

Done.