Check for an error handle passed in, in all dart_api functions that return a handle. Pass the erro…

runtime/vm/dart_api_impl.cc

Index: runtime/vm/dart_api_impl.cc
diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc
index d11b155972794ffe1677b64f79acfcceada3c0eb..731d2bf0fb44f37c9f72072603e3765c0cfedf67 100644
--- a/runtime/vm/dart_api_impl.cc
+++ b/runtime/vm/dart_api_impl.cc
@@ -1909,6 +1909,9 @@ DART_EXPORT Dart_Handle Dart_ListGetAt(Dart_Handle list, intptr_t index) {
   if (obj.IsGrowableObjectArray()) {
     GET_LIST_ELEMENT(isolate, GrowableObjectArray, obj, index);
   }
+  if (obj.IsError()) {
+    return list;

[Ivan Posva, 2012/07/31 16:22:47]

This file uses two different styles:
- Same as here.
- return Api::NewHandle(isolate, result.raw());

I defer to Todd to determine the correct style.

[turnidge, 2012/07/31 18:41:28]

I think I prefer "return list" as this saves a handle allocation.

+  }
   // Now check and handle a dart object that implements the List interface.
   const Instance& instance =
       Instance::Handle(isolate, GetListInstance(isolate, obj));
@@ -1957,6 +1960,9 @@ DART_EXPORT Dart_Handle Dart_ListSetAt(Dart_Handle list,
   if (obj.IsGrowableObjectArray()) {
     SET_LIST_ELEMENT(isolate, GrowableObjectArray, obj, index, value);
   }
+  if (obj.IsError()) {
+    return list;
+  }
   // Now check and handle a dart object that implements the List interface.
   const Instance& instance =
       Instance::Handle(isolate, GetListInstance(isolate, obj));
@@ -2033,6 +2039,9 @@ DART_EXPORT Dart_Handle Dart_ListGetAsBytes(Dart_Handle list,
                               offset,
                               length);
   }
+  if (obj.IsError()) {
+    return list;
+  }
   // Now check and handle a dart object that implements the List interface.
   const Instance& instance =
       Instance::Handle(isolate, GetListInstance(isolate, obj));
@@ -2120,6 +2129,9 @@ DART_EXPORT Dart_Handle Dart_ListSetAsBytes(Dart_Handle list,
                               offset,
                               length);
   }
+  if (obj.IsError()) {
+    return list;
+  }
   // Now check and handle a dart object that implements the List interface.
   const Instance& instance =
       Instance::Handle(isolate, GetListInstance(isolate, obj));
@@ -2400,6 +2412,9 @@ DART_EXPORT Dart_Handle Dart_ClosureFunction(Dart_Handle closure) {
     return Api::NewError("Null object passed to Dart_ClosureFunction");
   }
   if (!obj.IsClosure()) {
+    if (obj.IsError()) {

[Ivan Posva, 2012/07/31 16:22:47]

Please pull this out to the outer level just like "if (obj.IsNull())".

[Bill Hesse, 2012/08/03 08:29:26]

Isn't this exactly the semantics we get from
 UnwrapClosureHandle
if (null)
 RETURN_TYPE_ERROR (closure)?

Changed.

+      return closure;
+    }
     return Api::NewError("Invalid closure passed to Dart_ClosureFunction");
   }
   ASSERT(ClassFinalizer::AllClassesFinalized());
@@ -2420,6 +2435,9 @@ DART_EXPORT Dart_Handle Dart_InvokeClosure(Dart_Handle closure,
     return Api::NewError("Null object passed in to invoke closure");
   }
   if (!obj.IsClosure()) {
+    if (obj.IsError()) {

[Ivan Posva, 2012/07/31 16:22:47]

ditto

+      return closure;
+    }
     return Api::NewError("Invalid closure passed to invoke closure");
   }
   ASSERT(ClassFinalizer::AllClassesFinalized());
@@ -3118,6 +3136,9 @@ DART_EXPORT Dart_Handle Dart_New(Dart_Handle clazz,
     const String& dot = String::Handle(isolate, Symbols::Dot());
     dot_name = String::Concat(dot, String::Cast(name_obj));
   } else {
+    if (name_obj.IsError()) {

[Ivan Posva, 2012/07/31 16:22:47]

Please pull this out to the outer level:

if (name_obj.IsNull()) {
  ...
} else if (name_obj.IsString()) {
  ...
} else if (name_obj.IsError()) {
  ...
} else {

[Bill Hesse, 2012/08/03 08:29:26]

Done.

+      return constructor_name;
+    }
     return Api::NewError(
         "%s expects argument 'constructor_name' to be of type String.",
         CURRENT_FUNC);
@@ -3471,6 +3492,8 @@ DART_EXPORT Dart_Handle Dart_GetField(Dart_Handle container, Dart_Handle name) {
                            CURRENT_FUNC, field_name.ToCString());
     }
 
+  } else if (obj.IsError()) {
+      return container;
   } else {
     return Api::NewError(
         "%s expects argument 'container' to be an object, class, or library.",
@@ -3612,6 +3635,8 @@ DART_EXPORT Dart_Handle Dart_SetField(Dart_Handle container,
                            CURRENT_FUNC, field_name.ToCString());
     }
 
+  } else if (obj.IsError()) {
+      return container;
   } else {
     return Api::NewError(
         "%s expects argument 'container' to be an object, class, or library.",
@@ -3627,6 +3652,9 @@ DART_EXPORT Dart_Handle Dart_CreateNativeWrapperClass(Dart_Handle library,
   DARTSCOPE(isolate);
   const Object& param = Object::Handle(isolate, Api::UnwrapHandle(name));
   if (param.IsNull() || !param.IsString() || field_count <= 0) {
+    if (param.IsError()) {
+      return name;
+    }
     return Api::NewError(
         "Invalid arguments passed to Dart_CreateNativeWrapperClass");
   }
@@ -3636,6 +3664,9 @@ DART_EXPORT Dart_Handle Dart_CreateNativeWrapperClass(Dart_Handle library,
   Library& lib = Library::Handle(isolate);
   lib ^= Api::UnwrapHandle(library);
   if (lib.IsNull()) {
+    if (lib.IsError()) {

[Ivan Posva, 2012/07/31 16:22:47]

This makes no sense: lib is Null, so it cannot be an Error.

[turnidge, 2012/07/31 18:41:28]

I would move the check for a bad library to the top of the function and make it
use UnwrapLibraryHandle.  Like in Dart_LibraryName...

  const Library& lib = Api::UnwrapLibraryHandle(isolate, library);
  if (lib.IsNull()) {
    RETURN_TYPE_ERROR(isolate, library, Library);
  }

[Bill Hesse, 2012/08/03 08:29:26]

Done - changed to UnwrapStringHandle and UnwrapLibraryHandle.  I wasn't sure
that there wasn't
a complicated reason not to use these.


Oh - there was.  UnwrapLibraryHandle returns a const Library.  Const removed.
On 2012/07/31 18:41:28, turnidge wrote:

+      return library;
+    }
     return Api::NewError(
         "Invalid arguments passed to Dart_CreateNativeWrapperClass");
   }
@@ -3712,6 +3743,7 @@ DART_EXPORT Dart_Handle Dart_ThrowException(Dart_Handle exception) {
     // throw an exception here.
     return Api::NewError("No Dart frames on stack, cannot throw exception");
   }
+  // An error handle passed in causes a fatal error here.

[Ivan Posva, 2012/07/31 16:22:47]

Isn't this CL about fixing those FATALs?

[Bill Hesse, 2012/08/03 08:29:26]

Maybe FATAL does belong here, since these functions should not normally return. 
But I fixed them, so now I think people should usually write
PropagateError(Dart_ThrowException(exception));
Or should PropagateError be built in to ThrowException?


On 2012/07/31 16:22:47, Ivan Posva wrote:

   const Instance& excp =
       Instance::CheckedHandle(isolate, Api::UnwrapHandle(exception));
   // Unwind all the API scopes till the exit frame before throwing an
@@ -3734,6 +3766,7 @@ DART_EXPORT Dart_Handle Dart_ReThrowException(Dart_Handle exception,
     return Api::NewError("No Dart frames on stack, cannot throw exception");
   }
   DARTSCOPE(isolate);
+  // Any error handles passed in lead to a fatal error here.
   const Instance& excp =
       Instance::CheckedHandle(isolate, Api::UnwrapHandle(exception));
   const Instance& stk =