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..182e1d56c19f0ab3a964985ca0b043adc759b3a6 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()) {

[Ivan Posva, 2012/08/21 02:48:45]

How about making this into an if-elsif chain?

[Bill Hesse, 2012/08/23 11:17:44]

Done.

+    return list;
+  }
   // Now check and handle a dart object that implements the List interface.
   const Instance& instance =
       Instance::Handle(isolate, GetListInstance(isolate, obj));
@@ -1935,6 +1938,9 @@ DART_EXPORT Dart_Handle Dart_ListGetAt(Dart_Handle list, intptr_t index) {
 #define SET_LIST_ELEMENT(isolate, type, obj, index, value)                     \
   const type& array = type::Cast(obj);                                         \
   const Object& value_obj = Object::Handle(isolate, Api::UnwrapHandle(value)); \
+  if (!value_obj.IsNull() && !value_obj.IsInstance()) {                        \
+    RETURN_TYPE_ERROR(isolate, value, Instance);                               \
+  }                                                                            \
   if ((index >= 0) && (index < array.Length())) {                              \
     array.SetAt(index, value_obj);                                             \
     return Api::Success(isolate);                                              \
@@ -1957,6 +1963,9 @@ DART_EXPORT Dart_Handle Dart_ListSetAt(Dart_Handle list,
   if (obj.IsGrowableObjectArray()) {
     SET_LIST_ELEMENT(isolate, GrowableObjectArray, obj, index, value);
   }
+  if (obj.IsError()) {

[Ivan Posva, 2012/08/21 02:48:45]

ditto: if-elsif chain

[Bill Hesse, 2012/08/23 11:17:44]

Done.

+    return list;
+  }
   // Now check and handle a dart object that implements the List interface.
   const Instance& instance =
       Instance::Handle(isolate, GetListInstance(isolate, obj));
@@ -1969,6 +1978,12 @@ DART_EXPORT Dart_Handle Dart_ListSetAt(Dart_Handle list,
       const Integer& index_obj = Integer::Handle(isolate, Integer::New(index));
       const Object& value_obj =
           Object::Handle(isolate, Api::UnwrapHandle(value));
+      if (!value_obj.IsNull() && !value_obj.IsInstance()) {
+        RETURN_TYPE_ERROR(isolate, value, Instance);
+      }
+      if (value_obj.IsError()) {
+        return value;

[Ivan Posva, 2012/08/21 02:48:45]

How will this "return value" ever be reached? As far as I can see you already
returned above with a RETURN_TYPE_ERROR because value_obj is not null and it is
not an instance.

[Bill Hesse, 2012/08/23 11:17:44]

Correct.  Removed. 
On 2012/08/21 02:48:45, Ivan Posva wrote:

+      }
       GrowableArray<const Object*> args(2);
       args.Add(&index_obj);
       args.Add(&value_obj);
@@ -2033,6 +2048,9 @@ DART_EXPORT Dart_Handle Dart_ListGetAsBytes(Dart_Handle list,
                               offset,
                               length);
   }

[Ivan Posva, 2012/08/21 02:48:45]

ditto: if-elsif chain

[Bill Hesse, 2012/08/23 11:17:44]

Done.

+  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 +2138,9 @@ DART_EXPORT Dart_Handle Dart_ListSetAsBytes(Dart_Handle list,
                               offset,
                               length);
   }

[Ivan Posva, 2012/08/21 02:48:45]

ditto

+  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));
@@ -2395,16 +2416,12 @@ DART_EXPORT bool Dart_IsClosure(Dart_Handle object) {
 DART_EXPORT Dart_Handle Dart_ClosureFunction(Dart_Handle closure) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Object& obj = Object::Handle(isolate, Api::UnwrapHandle(closure));
-  if (obj.IsNull()) {
-    return Api::NewError("Null object passed to Dart_ClosureFunction");
-  }
-  if (!obj.IsClosure()) {
-    return Api::NewError("Invalid closure passed to Dart_ClosureFunction");
+  const Closure& closure_obj = Api::UnwrapClosureHandle(isolate, closure);
+  if (closure_obj.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, closure, Closure);
   }
   ASSERT(ClassFinalizer::AllClassesFinalized());
 
-  const Closure& closure_obj = Closure::Cast(obj);
   RawFunction* rf = closure_obj.function();
   return Api::NewHandle(isolate, rf);
 }
@@ -2415,21 +2432,25 @@ DART_EXPORT Dart_Handle Dart_InvokeClosure(Dart_Handle closure,
                                            Dart_Handle* arguments) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Object& obj = Object::Handle(isolate, Api::UnwrapHandle(closure));
-  if (obj.IsNull()) {
-    return Api::NewError("Null object passed in to invoke closure");
+  const Closure& closure_obj = Api::UnwrapClosureHandle(isolate, closure);
+  if (closure_obj.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, closure, Closure);
   }
-  if (!obj.IsClosure()) {
-    return Api::NewError("Invalid closure passed to invoke closure");
+  if (number_of_arguments < 0) {
+    return Api::NewError(
+        "%s expects argument 'number_of_arguments' to be non-negative.",
+        CURRENT_FUNC);
   }
   ASSERT(ClassFinalizer::AllClassesFinalized());
 
   // Now try to invoke the closure.
-  const Closure& closure_obj = Closure::Cast(obj);
   GrowableArray<const Object*> dart_arguments(number_of_arguments);
   for (int i = 0; i < number_of_arguments; i++) {
     const Object& arg =
         Object::Handle(isolate, Api::UnwrapHandle(arguments[i]));
+    if (!arg.IsNull() && !arg.IsInstance()) {
+      RETURN_TYPE_ERROR(isolate, arguments[i], Instance);
+    }
     dart_arguments.Add(&arg);
   }
   const Array& kNoArgumentNames = Array::Handle(isolate);
@@ -2487,8 +2508,7 @@ DART_EXPORT bool Dart_IsInterface(Dart_Handle handle) {
 DART_EXPORT Dart_Handle Dart_ClassName(Dart_Handle clazz) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Class& cls = Class::Handle(
-      isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
+  const Class& cls = Api::UnwrapClassHandle(isolate, clazz);

[Bill Hesse, 2012/08/16 11:54:13]

Am I missing somethigng here?  Is there a reason to make two handles, or is this
just to create a non-const handle (which we don't need here).

[turnidge, 2012/08/17 18:23:40]

The new code you've written here looks better.  I see no good reason for the
existing code to have the weird double-handle thing.

   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
@@ -2500,8 +2520,7 @@ DART_EXPORT Dart_Handle Dart_ClassName(Dart_Handle clazz) {
 DART_EXPORT Dart_Handle Dart_ClassGetLibrary(Dart_Handle clazz) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Class& cls = Class::Handle(
-      isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
+  const Class& cls = Api::UnwrapClassHandle(isolate, clazz);
   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
@@ -2520,8 +2539,7 @@ DART_EXPORT Dart_Handle Dart_ClassGetLibrary(Dart_Handle clazz) {
 DART_EXPORT Dart_Handle Dart_ClassGetDefault(Dart_Handle clazz) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Class& cls = Class::Handle(
-      isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
+  const Class& cls = Api::UnwrapClassHandle(isolate, clazz);
   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
@@ -2543,8 +2561,7 @@ DART_EXPORT Dart_Handle Dart_ClassGetInterfaceCount(Dart_Handle clazz,
                                                     intptr_t* count) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Class& cls = Class::Handle(
-      isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
+  const Class& cls = Api::UnwrapClassHandle(isolate, clazz);
   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
@@ -2563,8 +2580,7 @@ DART_EXPORT Dart_Handle Dart_ClassGetInterfaceAt(Dart_Handle clazz,
                                                  intptr_t index) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Class& cls = Class::Handle(
-      isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
+  const Class& cls = Api::UnwrapClassHandle(isolate, clazz);
   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
@@ -3100,8 +3116,8 @@ DART_EXPORT Dart_Handle Dart_New(Dart_Handle clazz,
   }
 
   // Get the class to instantiate.
-  Class& cls = Class::Handle(
-      isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
+  Class& cls =
+      Class::Handle(isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
@@ -3118,9 +3134,7 @@ 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 {
-    return Api::NewError(
-        "%s expects argument 'constructor_name' to be of type String.",
-        CURRENT_FUNC);
+    RETURN_TYPE_ERROR(isolate, constructor_name, String);
   }
 
   const char* msg = CheckIsolateState(isolate);
@@ -3471,6 +3485,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 +3628,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.",
@@ -3625,22 +3643,23 @@ DART_EXPORT Dart_Handle Dart_CreateNativeWrapperClass(Dart_Handle library,
                                                       int field_count) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  const Object& param = Object::Handle(isolate, Api::UnwrapHandle(name));
-  if (param.IsNull() || !param.IsString() || field_count <= 0) {
-    return Api::NewError(
-        "Invalid arguments passed to Dart_CreateNativeWrapperClass");
+  const String& cls_name = Api::UnwrapStringHandle(isolate, name);
+  if (cls_name.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, name, String);
   }
-  String& cls_name = String::Handle(isolate);
-  cls_name ^= param.raw();
-  cls_name = Symbols::New(cls_name);
-  Library& lib = Library::Handle(isolate);
-  lib ^= Api::UnwrapHandle(library);
+  Library& lib = Library::Handle(isolate,
+      Api::UnwrapLibraryHandle(isolate, library).raw());
   if (lib.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, library, Library);
+  }
+  if (field_count <= 0) {
     return Api::NewError(
-        "Invalid arguments passed to Dart_CreateNativeWrapperClass");
+        "Negative field_count passed to Dart_CreateNativeWrapperClass");
   }
+
+  String& cls_symbol = String::Handle(isolate, Symbols::New(cls_name));
   const Class& cls = Class::Handle(
-      isolate, Class::NewNativeWrapper(&lib, cls_name, field_count));
+      isolate, Class::NewNativeWrapper(&lib, cls_symbol, field_count));

[Ivan Posva, 2012/08/21 02:48:45]

This should have never expected a Library*. I am changing this in an independent
CL to expect a const Library& and then you should be able to use
Api::UnwrapLibraryHandle above.

https://chromiumcodereview.appspot.com/10827434

[Bill Hesse, 2012/08/23 11:17:44]

Done.

   if (cls.IsNull()) {
     return Api::NewError(
         "Unable to create native wrapper class : already exists");
@@ -3707,13 +3726,15 @@ DART_EXPORT Dart_Handle Dart_SetNativeInstanceField(Dart_Handle obj,
 DART_EXPORT Dart_Handle Dart_ThrowException(Dart_Handle exception) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
+  const Instance& excp = Api::UnwrapInstanceHandle(isolate, exception);
+  if (excp.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, exception, Instance);
+  }
   if (isolate->top_exit_frame_info() == 0) {
     // There are no dart frames on the stack so it would be illegal to
     // throw an exception here.
     return Api::NewError("No Dart frames on stack, cannot throw exception");
   }
-  const Instance& excp =
-      Instance::CheckedHandle(isolate, Api::UnwrapHandle(exception));
   // Unwind all the API scopes till the exit frame before throwing an
   // exception.
   ApiState* state = isolate->api_state();
@@ -3728,16 +3749,20 @@ DART_EXPORT Dart_Handle Dart_ReThrowException(Dart_Handle exception,
                                               Dart_Handle stacktrace) {
   Isolate* isolate = Isolate::Current();
   CHECK_ISOLATE(isolate);
+  DARTSCOPE(isolate);
+  const Instance& excp = Api::UnwrapInstanceHandle(isolate, exception);
+  if (excp.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, exception, Instance);
+  }
+  const Instance& stk = Api::UnwrapInstanceHandle(isolate, stacktrace);
+  if (stk.IsNull()) {
+    RETURN_TYPE_ERROR(isolate, stacktrace, Instance);
+  }
   if (isolate->top_exit_frame_info() == 0) {
     // There are no dart frames on the stack so it would be illegal to
     // throw an exception here.
     return Api::NewError("No Dart frames on stack, cannot throw exception");
   }
-  DARTSCOPE(isolate);
-  const Instance& excp =
-      Instance::CheckedHandle(isolate, Api::UnwrapHandle(exception));
-  const Instance& stk =
-      Instance::CheckedHandle(isolate, Api::UnwrapHandle(stacktrace));
   // Unwind all the API scopes till the exit frame before throwing an
   // exception.
   ApiState* state = isolate->api_state();