[vm/ffi] Breaking change: Throw errors when returned as `Dart_Handle`

[dcharkes]

trafficstars

The following snippet throws an error instead of returning an error object:

Dart_Handle DartRuntimeHooks::GetCallbackFromHandle(int64_t handle) {
  Dart_Handle result = DartCallbackCache::GetCallback(handle);
  if (Dart_IsError(result)) {
    Dart_PropagateError(result);
  }
  return result;
}

(Example combined from https://github.com/flutter/flutter/issues/111243, https://github.com/flutter/engine/pull/36032, and flutter_engine/lib/ui/dart_runtime_hooks.cc.)

We should consider automatically throwing when Dart_IsError is true so that this pattern would not be needed.

Dart_IsError is defined in terms of a number of predefined class ids:

• Error
• ApiError
• LanguageError
• UnhandledError
• UnwindError

Throwing these would make it impossible to return objects with these types through dart:ffi. (If we ever want to do that, we could wrap the object in another object. Or if we're really concerned about not allocating another object, we could add another boolean flag to @FfiNative and asFunction.)

This is a breaking change, as code which currently returns these 5 objects types by Dart_Handle in dart:ffi will start throwing instead of returning the error object, requiring a try-catch as migration. However, I believe it might be unlikely that many users are relying on this.

cc @mraleph @dnfield @mkustermann

[dnfield]

I'm not sure I understand where you'd need a try catch - and some of these errors can end up happening purely when using the C API in dart_api.h (don't require directly invoking Dart code).

I have two main questions:

1. What happens to the C++ stack with this change? In particular will destructors still run and release resources? Will we basically be running into issues we try to avoid by not using Dart_ThrowException?
2. What happens to all of the API that currently returns error handles?

[dcharkes]

I'm not sure I understand where you'd need a try catch

You should not catch it, but fix the application instead. (Only exceptions should be caught, not errors.) I meant to say that if there is any code which relies on getting an Error back as object, the migration on the Dart end could be to wrap it in a try catch. (Though there are better migrations probably, see below.)

What happens to all of the API that currently returns error handles?

These would need to be migrated to return (a) an object wrapping the error handling, or (b) wrapped in a try-catch, or (c) we could add a ErrorHandle (native) <-> Object (Dart) to the native-to-Dart type mappings that does not throw errors (the current Handle <-> Object behavior.)

It probably should be (a) or (c). As Errors should in general not be caught.

Are there many such places in Flutters' ui implementation?

What happens to the C++ stack with this change? In particular will destructors still run and release resources? Will we basically be running into issues we try to avoid by not using Dart_ThrowException?

The exception would be thrown when returning from the FFI call. Not at the place where Dart_ThrowException or Dart_PropagateError is in the code. Conceptually, it is more of a Dart_PropagateErrorWhenReturningFromFfiCall anywhere in the C++. Or, in other words, it is using Dart_PropagateError just before the return in C++ that would return to Dart. So, all C++ code would still be run, including destructors.

[dnfield]

This SGTM

[dcharkes]

This SGTM

@dnfield do you prefer (a) or (c) for returning errors? Do you have calls that currently return errors?

[dnfield]

I think ideally this behaves just like other Dart code that throws an error.

I'm not quite sure I'm understanding which option would result in that, but basically it should behave similarly to a method that just does this:

void foo() {
  throw SomeError('This is an error');
}