Don't clear exception pending message if we have externally TryCatch and finally on top of stack.

V8 can clear exception pending message, when should not do this.

The case:
v8::TryCatch try_catch;
CompileRun(try { CEvaluate('throw 1;'); } finally {});
CHECK(try_catch.HasCaught());
CHECK(!try_catch.Message().IsEmpty());

CEvaluate is native call. Last check is not passed without patch. Patch contains test TryCatchFinallyStoresMessageUsingTryCatchHandler with more details.

R=mstarzinger@chromium.org, vsevik@chromium.org, yangguo@chromium.org

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

[vsevik, 2014-05-28 18:00:31]

Can we add a test?

https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc#newcod...
src/isolate.cc:1727: // Return true. if we don't pending message and can remove
it
Either comment on particular actions inside the method or add a comment to the
method declaration if you want to document its behavior as a whole.

https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc#newcod...
src/isolate.cc:1731: bool external_caught = IsExternallyCaught();
I think correct naming for the methods would be HasExternalTryCatch and
IsFinallyOnTop now.

[kozyatinskiy1, 2014-05-29 10:26:25]

On 2014/05/28 18:00:31, vsevik wrote:
> Can we add a test?
Yes, we can. Done.

> https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc#newcod...
> src/isolate.cc:1727: // Return true. if we don't pending message and can
remove
> it
> Either comment on particular actions inside the method or add a comment to the
> method declaration if you want to document its behavior as a whole.
Comment moved to method declaration. Done.
 
>
https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc#newcod...
> src/isolate.cc:1731: bool external_caught = IsExternallyCaught();
> I think correct naming for the methods would be HasExternalTryCatch and
> IsFinallyOnTop now.
Done.

[vsevik, 2014-06-02 09:19:55]

https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#ne...
src/isolate.cc:1730: bool has_externaly_caught = HasExternallyCaught();
has_external_try_catch

https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#ne...
src/isolate.cc:1733: bool next_externaly_caught = has_externaly_caught &&
!is_finally_on_top;
should_propagate_now

https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#ne...
src/isolate.cc:1736: if (!has_externaly_caught) return true;
if (!should_propagate_now) return !has_external_try_catch;

https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.h
File src/isolate.h (right):

https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.h#new...
src/isolate.h:1183: // If there is no external try-catch or message successfully
propogate,
or message was successfully propagated

[kozyatinskiy1, 2014-06-02 09:50:53]

On 2014/06/02 09:19:55, vsevik wrote:
> https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#ne...
> src/isolate.cc:1730: bool has_externaly_caught = HasExternallyCaught();
> has_external_try_catch
Done.

>
https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#ne...
> src/isolate.cc:1733: bool next_externaly_caught = has_externaly_caught &&
> !is_finally_on_top;
> should_propagate_now
Done.
 
>
https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#ne...
> src/isolate.cc:1736: if (!has_externaly_caught) return true;
> if (!should_propagate_now) return !has_external_try_catch;
Done.
 
> https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.h
> File src/isolate.h (right):
> 
>
https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.h#new...
> src/isolate.h:1183: // If there is no external try-catch or message
successfully
> propogate,
> or message was successfully propagated
Done.

[vsevik, 2014-06-02 14:53:29]

lgtm

[vsevik, 2014-06-02 14:54:00]

yangguo@, Could you please have a look on this?

[Yang, 2014-06-02 15:21:16]

Looking good. I got some comments. And I would like to add mstarzinger@ for a
second opinion.

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ne...
src/isolate.cc:1154: return false;
Can we simply return an expression?

return (thread_local_top()->catcher_ != NULL) && (try_catch_handler() ==
thread_local_top()->catcher_);

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ne...
src/isolate.cc:1192: bool canClearMessage =
PropagatePendingExceptionToExternalTryCatch();
please name this 'can_clear_message' instead.

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ne...
src/isolate.cc:1220: clear_pending_message();
no line break here.

[vsevik, 2014-06-02 15:31:27]

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc
File src/isolate.cc (left):

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ol...
src/isolate.cc:1157: if (!is_catchable_by_javascript(pending_exception())) {
Looking at this once again I think this check is still needed.

Unfortunately you will probably need to extract another function. :(

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.h#new...
src/isolate.h:608: bool HasExternalTryCatch();
I think these two could be made private.
yangguo@, mstarzinger@ WDYT?

[kozyatinskiy1, 2014-06-03 09:14:58]

On 2014/06/02 15:21:16, Yang wrote:
> Looking good. I got some comments. And I would like to add mstarzinger@ for a
> second opinion.
> 
> https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ne...
> src/isolate.cc:1154: return false;
> Can we simply return an expression?
Yes, we can. Done.

>
https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ne...
> src/isolate.cc:1192: bool canClearMessage =
> PropagatePendingExceptionToExternalTryCatch();
> please name this 'can_clear_message' instead.
Done.

>
https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ne...
> src/isolate.cc:1220: clear_pending_message();
> no line break here.
Done.

On 2014/06/02 15:31:27, vsevik wrote:
> https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc
> File src/isolate.cc (left):
> 
>
https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#ol...
> src/isolate.cc:1157: if (!is_catchable_by_javascript(pending_exception())) {
> Looking at this once again I think this check is still needed.
> 
> Unfortunately you will probably need to extract another function. :(
I've reused function is Isolate::is_catchable_by_javascript. Done.

[Yang, 2014-06-04 07:21:34]

LGTM from my side, but please wait for Michael to take a look. I'll ask him
later to do that.

[Michael Starzinger, 2014-06-05 11:02:08]

LGTM from my end as well. Just nits. Sorry for the delay.

https://codereview.chromium.org/306463002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/306463002/diff/80001/src/isolate.h#newcode1182
src/isolate.h:1182: // Propagate pending exception message to the external
try-catch.
nit: s/external try-catch/v8::TryCatch/

https://codereview.chromium.org/306463002/diff/80001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/306463002/diff/80001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:8401: CHECK_EQ(*exception_value, "1");
Let's call "try_catch.Reset()" here before running the second test snippet,
otherwise we don't have a clean slate.

[kozyatinskiy1, 2014-06-05 12:33:00]

> https://codereview.chromium.org/306463002/diff/80001/src/isolate.h
> File src/isolate.h (right):
> 
> https://codereview.chromium.org/306463002/diff/80001/src/isolate.h#newcode1182
> src/isolate.h:1182: // Propagate pending exception message to the external
> try-catch.
> nit: s/external try-catch/v8::TryCatch/
Done.

> https://codereview.chromium.org/306463002/diff/80001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/306463002/diff/80001/test/cctest/test-api.cc#...
> test/cctest/test-api.cc:8401: CHECK_EQ(*exception_value, "1");
> Let's call "try_catch.Reset()" here before running the second test snippet,
> otherwise we don't have a clean slate.
Done.

[yurys, 2014-06-05 13:08:01]

Committed patchset #6 manually as r21697.