Added support for isolate unhandled exceptions.

Added support for isolate unhandled exceptions.


BUG=

Committed: https://code.google.com/p/dart/source/detail?r=15352

[Tom Ball, 2012-11-20 21:48:55]

This is a new CL, but is a continuation of the one you've been reviewing.

New changes are that unhandled exceptions aren't called for OOM and overflow
conditions (runtime/vm/isolate.cc), and tests that depend on isolate errors
exiting the VM are now skipped instead of run "Fail, OK" since they timeout now.

[siva, 2012-11-21 00:45:04]

https://codereview.chromium.org/11413101/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/11413101/diff/1/runtime/vm/isolate.cc#newcode131
runtime/vm/isolate.cc:131: }
I presume a return of success == true would keep the isolate running. Does this
mean that when we get an OOM or stack overflow exception we will keep running. I
would have assumed that success should be set to false in those two cases.

Also shouldn't we be calling the Isolate::UnahandledExceptionCallback for the
OOM and stack overflow cases.

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart
File sdk/lib/isolate/base.dart (right):

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart#new...
sdk/lib/isolate/base.dart:23: * arguments. It is illegal to pass a function
closure.
We probably need some documentation here for the unhandledexception callback
parameter here.

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart#new...
sdk/lib/isolate/base.dart:40: * this function returns a [SendPort] derived from
it.
Ditto comment regarding documentation of the unhandled exception callback
parameter.

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart#new...
sdk/lib/isolate/base.dart:149: }
What is this change? Is it related to the UnhandledException stuff...

[Tom Ball, 2012-11-21 05:22:37]

https://codereview.chromium.org/11413101/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/11413101/diff/1/runtime/vm/isolate.cc#newcode131
runtime/vm/isolate.cc:131: }
On 2012/11/21 00:45:04, siva wrote:
> I presume a return of success == true would keep the isolate running. Does
this
> mean that when we get an OOM or stack overflow exception we will keep running.
I
> would have assumed that success should be set to false in those two cases.
> 
> Also shouldn't we be calling the Isolate::UnahandledExceptionCallback for the
> OOM and stack overflow cases.

PTAL -- I refactored it and fixed when the UnhandledExceptionCallback is called
(and hopefully it's easier to read).

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart
File sdk/lib/isolate/base.dart (right):

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart#new...
sdk/lib/isolate/base.dart:23: * arguments. It is illegal to pass a function
closure.
On 2012/11/21 00:45:04, siva wrote:
> We probably need some documentation here for the unhandledexception callback
> parameter here.

Done.

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart#new...
sdk/lib/isolate/base.dart:40: * this function returns a [SendPort] derived from
it.
On 2012/11/21 00:45:04, siva wrote:
> Ditto comment regarding documentation of the unhandled exception callback
> parameter.

Done.

https://codereview.chromium.org/11413101/diff/1/sdk/lib/isolate/base.dart#new...
sdk/lib/isolate/base.dart:149: }
On 2012/11/21 00:45:04, siva wrote:
> What is this change? Is it related to the UnhandledException stuff...

I didn't write this, so I'm guessing it's related to the several hand-merges
I've done over the past week.  Deleted.

[Tom Ball, 2012-11-26 21:45:11]

PTAL: the isolate spawn API callbacks are removed, leaving just the embedder API
for unhandled isolate exceptions.

[siva, 2012-11-26 23:03:01]

LGTM with some comments.

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:6288: ASSERT(strcmp(last_exception,
"UnhandledException") == 0);
I think this should be :
EXPECT_NOTNULL(last_exception);
EXPECT_STREQ("UnhandledException", last_exception);

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:6293: ASSERT(strcmp(last_exception,
"UnhandledException") == 0);
Ditto comment.

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:6296: ASSERT(last_exception == NULL);
This should be :
EXPECT_EQ(NULL, last_exception);

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/exceptions.cc#n...
runtime/vm/exceptions.cc:178: OS::PrintErr("Exiting the process\n");
The comment "Exiting the process" is not valid anymore
maybe it should be "Shutting down the isolate".

[Tom Ball, 2012-11-26 23:14:50]

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:6288: ASSERT(strcmp(last_exception,
"UnhandledException") == 0);
On 2012/11/26 23:03:02, siva wrote:
> I think this should be :
> EXPECT_NOTNULL(last_exception);
> EXPECT_STREQ("UnhandledException", last_exception);

Done.

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:6293: ASSERT(strcmp(last_exception,
"UnhandledException") == 0);
On 2012/11/26 23:03:02, siva wrote:
> Ditto comment.

Done.

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/dart_api_impl_t...
runtime/vm/dart_api_impl_test.cc:6296: ASSERT(last_exception == NULL);
On 2012/11/26 23:03:02, siva wrote:
> This should be :
> EXPECT_EQ(NULL, last_exception);

Done.

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/exceptions.cc#n...
runtime/vm/exceptions.cc:178: OS::PrintErr("Exiting the process\n");
On 2012/11/26 23:03:02, siva wrote:
> The comment "Exiting the process" is not valid anymore
> maybe it should be "Shutting down the isolate".

Done.

https://codereview.chromium.org/11413101/diff/9001/runtime/vm/exceptions.cc#n...
runtime/vm/exceptions.cc:178: OS::PrintErr("Exiting the process\n");
On 2012/11/26 23:03:02, siva wrote:
> The comment "Exiting the process" is not valid anymore
> maybe it should be "Shutting down the isolate".

Done.