Second version of support for specifying an unhandled exception callback

Second version of support for specifying an unhandled exception callback
in Dart.  Isolate.spawnFunction optionally takes a callback parameter,
which is not resolved until an unhandled exception occurs.  If a callback
wasn't specified, the VM will then call an "_unhandledExceptionCallback"
function if it is defined in the isolate's source file. If no callback
is provided, the error is logged as the isolate is closed.
Committed: https://code.google.com/p/dart/source/detail?r=16186

[Tom Ball, 2012-12-12 22:41:11]

This is an update to issue 11473026, which I can't update due to a merge issue
with "git svn rebase".

The main changes are: removed callback parameter from Isolate.spawnUri(), and it
now calls a _unhandledExceptionCallback() function, if it's defined in the
isolate's source file.

[siva, 2012-12-13 18:30:38]

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/lib/isolate.cc...
runtime/lib/isolate.cc:205: explicit SpawnState(const Function& func, const
Function& callback_func)
explicit not needed anymore as it has 2 arguments now.

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/lib/isolate.cc...
runtime/lib/isolate.cc:231: function_name_(NULL) {
function_name_(NULL),
exception_callback_name_(NULL) {

to be consistent with the other fields.

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/lib/isolate.cc...
runtime/lib/isolate.cc:242: if (exception_callback_name_ != NULL) {
Can exception_callback_name_ be NULL?

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/lib/isolate.cc...
runtime/lib/isolate.cc:339: HandleScope handle_scope(child_isolate);
Do we need a new zone and handle scope for the code below? It could use the one
that was created above.

Maybe:

if (result.IsError()) {
  ...
  ...
} else {
  const String& callback_name = String::Handle(child_isolate,
String::New(state->exception_callback_name);
  ...
  ...
}

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/isolate_unhandled_exception_test.dart (right):

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_unhandled_exception_test.dart:1: import
'dart:isolate';
We normally have copyrights in test files too.

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/isolate_unhandled_exception_test2.dart (right):

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_unhandled_exception_test2.dart:1: import
'dart:isolate';
ditto.

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_unhandled_exception_test2.dart:5: // throwing an
unhandled exception, if it was created with an
instead of created with an unhandled exception it would be if there is a top
level method ....

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/isolate_unhandled_exception_uri_test.dart (right):

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_unhandled_exception_uri_test.dart:15: Future f =
isolate_port.call('throw exception');
Is this a bug in the library implementation because this future never completes
on an unhandled exception?

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:198: const Object& result = Object::Handle(response);
We try not to keep these raw pointers in local variables
(the danger is that somebody might add code in between the two lines without
realizing the invariant).

const Object& result = Object::Handle(DartEntry::InvokeStatic(...));
would be safer.

https://chromiumcodereview.appspot.com/11558034/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:207: bool continue_from_exception = (response ==
Bool::True());
bool continue_from_exception = result.value();

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/lib/isolate...
runtime/lib/isolate.cc:445: callback_func = Function::null();
When a handle is created it is initialized with null there is no need to assign
a null again to it.
Maybe you can structure as:
if (callback.IsClosure()) {
   ...
} else if (!callback.IsNull()) {
   ...
}

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/lib/isolate...
runtime/lib/isolate.cc:465: ASSERT(ctx.num_variables() == 0);
We need a similar assert for callback.

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/vm/isolate....
runtime/vm/isolate.cc:149: }
How does this work if we have multiple libraries which have the same top level
name.

When registering the name in SpawnFunction we remember the library_uri also and
that is used to resolve the function to invoke, shouldn't we be doing something
similar here.

For the "-unhandledExceptionCallback" case we should be looking in the isolate
library right, not core library or any of the other libs.

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/vm/object_s...
File runtime/vm/object_store.h (right):

https://chromiumcodereview.appspot.com/11558034/diff/8001/runtime/vm/object_s...
runtime/vm/object_store.h:559: RawString* isolate_library_uri_;
What is this isolate_library_uri_ field? It doesn't seem to be used anywhere.

[Tom Ball, 2012-12-13 19:33:22]

https://codereview.chromium.org/11558034/diff/8001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/11558034/diff/8001/runtime/lib/isolate.cc#new...
runtime/lib/isolate.cc:445: callback_func = Function::null();
On 2012/12/13 18:30:38, siva wrote:
> When a handle is created it is initialized with null there is no need to
assign
> a null again to it.
> Maybe you can structure as:
> if (callback.IsClosure()) {
>    ...
> } else if (!callback.IsNull()) {
>    ...
> }

Fixed.

https://codereview.chromium.org/11558034/diff/8001/runtime/lib/isolate.cc#new...
runtime/lib/isolate.cc:465: ASSERT(ctx.num_variables() == 0);
On 2012/12/13 18:30:38, siva wrote:
> We need a similar assert for callback.

Done.

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

https://codereview.chromium.org/11558034/diff/8001/runtime/vm/isolate.cc#newc...
runtime/vm/isolate.cc:149: }
On 2012/12/13 18:30:38, siva wrote:
> How does this work if we have multiple libraries which have the same top level
> name.
> 
> When registering the name in SpawnFunction we remember the library_uri also
and
> that is used to resolve the function to invoke, shouldn't we be doing
something
> similar here.
> 
> For the "-unhandledExceptionCallback" case we should be looking in the isolate
> library right, not core library or any of the other libs.

As we discussed, the function is now just looked up in the root library.

https://codereview.chromium.org/11558034/diff/8001/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

https://codereview.chromium.org/11558034/diff/8001/runtime/vm/object_store.h#...
runtime/vm/object_store.h:559: RawString* isolate_library_uri_;
On 2012/12/13 18:30:38, siva wrote:
> What is this isolate_library_uri_ field? It doesn't seem to be used anywhere.

Removed -- it was cruft from an earlier design.

[siva, 2012-12-14 06:28:35]

LGTM with some minor nits.

https://chromiumcodereview.appspot.com/11558034/diff/22004/runtime/lib/isolat...
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/11558034/diff/22004/runtime/lib/isolat...
runtime/lib/isolate.cc:460: ASSERT(callback_func.IsFunction() ||
callback_func.IsNull());
I actually meant something like:

#if defined(DEBUG)
Context& ctx = Context::Handle();
ctx = Closure::context(closure);
ASSERT(ctx.num_variables() == 0);
if (!callback.IsNull()) {
  ctx = Closure::context(callback);
  ASSERT(ctx.num_variables() == 0);
}
#endif

https://chromiumcodereview.appspot.com/11558034/diff/22004/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/11558034/diff/22004/runtime/vm/isolate...
runtime/vm/isolate.cc:139: Library& lib = Library::Handle(isolate_);
can be written as
const Library& lib = Library::Handle(isolate_,
isolate_->object_store()->root_library());
same for func.

https://chromiumcodereview.appspot.com/11558034/diff/22004/runtime/vm/isolate...
runtime/vm/isolate.cc:142: func = lib.LookupLocalFunction(callback_name);
I thought for the spawnFunction case you wanted to resolve callback_name in the
library which spawned the isolate.

https://chromiumcodereview.appspot.com/11558034/diff/22004/runtime/vm/isolate...
runtime/vm/isolate.cc:158: Object& exception = Object::Handle();
Ditto comment
const Object& exception = Object::Handle(Exceptions::Create(...);

[Tom Ball, 2012-12-14 21:22:44]

https://codereview.chromium.org/11558034/diff/22004/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/11558034/diff/22004/runtime/lib/isolate.cc#ne...
runtime/lib/isolate.cc:460: ASSERT(callback_func.IsFunction() ||
callback_func.IsNull());
On 2012/12/14 06:28:36, siva wrote:
> I actually meant something like:
> 
> #if defined(DEBUG)
> Context& ctx = Context::Handle();
> ctx = Closure::context(closure);
> ASSERT(ctx.num_variables() == 0);
> if (!callback.IsNull()) {
>   ctx = Closure::context(callback);
>   ASSERT(ctx.num_variables() == 0);
> }
> #endif

Done.

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

https://codereview.chromium.org/11558034/diff/22004/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:139: Library& lib = Library::Handle(isolate_);
On 2012/12/14 06:28:36, siva wrote:
> can be written as
> const Library& lib = Library::Handle(isolate_,
> isolate_->object_store()->root_library());
> same for func.

Done.

https://codereview.chromium.org/11558034/diff/22004/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:142: func = lib.LookupLocalFunction(callback_name);
On 2012/12/14 06:28:36, siva wrote:
> I thought for the spawnFunction case you wanted to resolve callback_name in
the
> library which spawned the isolate.

Now checks isolate_library first, and falls back to root_library. The
isolate_library is only set by Isolate.spawnFunction(), not spawnUri().

https://codereview.chromium.org/11558034/diff/22004/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:158: Object& exception = Object::Handle();
On 2012/12/14 06:28:36, siva wrote:
> Ditto comment
> const Object& exception = Object::Handle(Exceptions::Create(...);

Done.

[Mads Ager (google), 2013-01-22 13:52:22]

Sorry for the extremely slow drive-by:

What is the plan for making it possible to have a top-level exception handler
for the main isolate as well? Maybe the isolate library could have a top-level
function instead that can be set from user-code. That way the main isolate and
isolates spawned later will behave the same.