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

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "include/dart_api.h"
 
 #include "vm/bigint_operations.h"
 #include "vm/class_finalizer.h"
 #include "vm/compiler.h"
 #include "vm/dart.h"
@@ skipping 1891 matching lines @@
 DART_EXPORT Dart_Handle Dart_ListGetAt(Dart_Handle list, intptr_t index) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
   const Object& obj = Object::Handle(isolate, Api::UnwrapHandle(list));
   if (obj.IsArray()) {
     GET_LIST_ELEMENT(isolate, Array, obj, 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));
   if (!instance.IsNull()) {
     String& name = String::Handle(isolate, String::New("[]"));
     const Function& function =
         Function::Handle(isolate,
                          Resolver::ResolveDynamic(instance, name, 2, 0));
     if (!function.IsNull()) {
       GrowableArray<const Object*> args(1);
@@ skipping 28 matching lines @@
   const Object& obj = Object::Handle(isolate, Api::UnwrapHandle(list));
   if (obj.IsArray()) {
     if (obj.IsImmutableArray()) {
       return Api::NewError("Cannot modify immutable array");
     }
     SET_LIST_ELEMENT(isolate, Array, obj, index, value);
   }
   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));
   if (!instance.IsNull()) {
     String& name = String::Handle(isolate, String::New("[]="));
     const Function& function =
         Function::Handle(isolate,
                          Resolver::ResolveDynamic(instance, name, 3, 0));
     if (!function.IsNull()) {
       const Integer& index_obj = Integer::Handle(isolate, Integer::New(index));
       const Object& value_obj =
           Object::Handle(isolate, Api::UnwrapHandle(value));
       GrowableArray<const Object*> args(2);

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

If value is an error then you would be adding an Error object into the list
here. Not what is intended.

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

Ivan is right.  We should check for error values up front in the same way that
we do in Dart_SetField...

  // Since null is allowed for value, we don't use UnwrapInstanceHandle.
  const Object& value_obj = Object::Handle(isolate, Api::UnwrapHandle(value));
  if (!value_obj.IsNull() && !value_obj.IsInstance()) {
    RETURN_TYPE_ERROR(isolate, value, Instance);
  }
  Instance& value_instance = Instance::Handle(isolate);
  value_instance ^= value_obj.raw();

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

Done here and in SET_LIST_ELEMENT macro.

       args.Add(&index_obj);
       args.Add(&value_obj);
       const Array& kNoArgumentNames = Array::Handle(isolate);
       return Api::NewHandle(
           isolate,
           DartEntry::InvokeDynamic(instance, function, args, kNoArgumentNames));
     }
   }
   return Api::NewError("Object does not implement the 'List' interface");
 }
@@ skipping 43 matching lines @@
                               offset,
                               length);  }
   if (obj.IsGrowableObjectArray()) {
     GET_LIST_ELEMENT_AS_BYTES(isolate,
                               GrowableObjectArray,
                               obj,
                               native_array,
                               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));
   if (!instance.IsNull()) {
     String& name = String::Handle(isolate, String::New("[]"));
     const Function& function =
         Function::Handle(isolate,
                          Resolver::ResolveDynamic(instance, name, 2, 0));
     if (!function.IsNull()) {
       Object& result = Object::Handle(isolate);
@@ skipping 67 matching lines @@
                               length);
   }
   if (obj.IsGrowableObjectArray()) {
     SET_LIST_ELEMENT_AS_BYTES(isolate,
                               GrowableObjectArray,
                               obj,
                               native_array,
                               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));
   if (!instance.IsNull()) {
     String& name = String::Handle(isolate, String::New("[]="));
     const Function& function =
         Function::Handle(isolate,
                          Resolver::ResolveDynamic(instance, name, 3, 0));
     if (!function.IsNull()) {
       Integer& indexobj = Integer::Handle(isolate);
@@ skipping 260 matching lines @@
 
 
 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()) {
+    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());
 
   const Closure& closure_obj = Closure::Cast(obj);
   RawFunction* rf = closure_obj.function();
   return Api::NewHandle(isolate, rf);
 }
 
 
 DART_EXPORT Dart_Handle Dart_InvokeClosure(Dart_Handle closure,
                                            int number_of_arguments,
                                            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");
   }
   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());
 
   // 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]));

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

Passing errors as arguments.

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

We also need to check for illegal number_of_arguments too.

You can copy the code used in Dart_Invoke here...

  if (number_of_arguments < 0) {
    return Api::NewError(
        "%s expects argument 'number_of_arguments' to be non-negative.",
        CURRENT_FUNC);
  }

  // Check for malformed arguments in the arguments list.
  GrowableArray<const Object*> args(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()) {
      if (arg.IsError()) {
        return Api::NewHandle(isolate, arg.raw());
      } else {
        return Api::NewError(
            "%s expects arguments[%d] to be an Instance handle.",
            CURRENT_FUNC, i);
      }
    }
    args.Add(&arg);
  }

...perhaps the argument checking code could be pulled into a helper function.

     dart_arguments.Add(&arg);
   }
   const Array& kNoArgumentNames = Array::Handle(isolate);
   return Api::NewHandle(
       isolate,
       DartEntry::InvokeClosure(closure_obj, dart_arguments, kNoArgumentNames));
 }
 
 
 DART_EXPORT int64_t Dart_ClosureSmrck(Dart_Handle object) {
@@ skipping 656 matching lines @@
         CURRENT_FUNC);
   }
 
   // Get the class to instantiate.
   Class& cls = Class::Handle(
       isolate, Api::UnwrapClassHandle(isolate, clazz).raw());
   if (cls.IsNull()) {
     RETURN_TYPE_ERROR(isolate, clazz, Class);
   }
   String& base_constructor_name = String::Handle();
   base_constructor_name = cls.Name();

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

Not dealing with Error.

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

I don't understand.  cls has been checked for error, and Class::Name looks safe.

 
   // And get the name of the constructor to invoke.
   String& dot_name = String::Handle(isolate);
   const Object& name_obj =
       Object::Handle(isolate, Api::UnwrapHandle(constructor_name));
   if (name_obj.IsNull()) {
     dot_name = Symbols::Dot();
   } else if (name_obj.IsString()) {
     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);
   }
 
   const char* msg = CheckIsolateState(isolate);
   if (msg != NULL) {
     return Api::NewError(msg);
   }
 
@@ skipping 333 matching lines @@
       const Array& kNoArgNames = Array::Handle(isolate);
       return Api::NewHandle(
           isolate, DartEntry::InvokeStatic(getter, args, kNoArgNames));
     } else if (!field.IsNull()) {
       return Api::NewHandle(isolate, field.value());
     } else {
       return Api::NewError("%s: did not find top-level variable '%s'.",
                            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.",
         CURRENT_FUNC);
   }
 }
 
 
 DART_EXPORT Dart_Handle Dart_SetField(Dart_Handle container,
                                       Dart_Handle name,
@@ skipping 121 matching lines @@
                              CURRENT_FUNC, field_name.ToCString());
       } else {
         field.set_value(value_instance);
         return Api::Success(isolate);
       }
     } else {
       return Api::NewError("%s: did not find top-level variable '%s'.",
                            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.",
         CURRENT_FUNC);
   }
 }
 
 
 DART_EXPORT Dart_Handle Dart_CreateNativeWrapperClass(Dart_Handle library,
                                                       Dart_Handle name,
                                                       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) {
+    if (param.IsError()) {
+      return name;
+    }
     return Api::NewError(
         "Invalid arguments passed to Dart_CreateNativeWrapperClass");
   }
   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);
   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");
   }
   const Class& cls = Class::Handle(
       isolate, Class::NewNativeWrapper(&lib, cls_name, field_count));
   if (cls.IsNull()) {
     return Api::NewError(
         "Unable to create native wrapper class : already exists");
   }
   return Api::NewHandle(isolate, cls.raw());
@@ skipping 56 matching lines @@
 
 
 DART_EXPORT Dart_Handle Dart_ThrowException(Dart_Handle exception) {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
   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");
   }
+  // 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
   // exception.
   ApiState* state = isolate->api_state();
   ASSERT(state != NULL);
   state->UnwindScopes(isolate->top_exit_frame_info());
   Exceptions::Throw(excp);
   return Api::NewError("Exception was not thrown, internal error");
 }
 
 
 DART_EXPORT Dart_Handle Dart_ReThrowException(Dart_Handle exception,
                                               Dart_Handle stacktrace) {
   Isolate* isolate = Isolate::Current();
   CHECK_ISOLATE(isolate);
   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);
+  // Any error handles passed in lead to a fatal error here.
   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();
   ASSERT(state != NULL);
   state->UnwindScopes(isolate->top_exit_frame_info());
   Exceptions::ReThrow(excp, stk);
@@ skipping 429 matching lines @@
 DART_EXPORT void Dart_InitPerfEventsSupport(Dart_FileWriterFunction function) {
   Dart::set_perf_events_writer(function);
 }
 
 
 DART_EXPORT void Dart_InitFlowGraphPrinting(Dart_FileWriterFunction function) {
   Dart::set_flow_graph_writer(function);
 }
 
 }  // namespace dart