Revert "[vm/ffi] Disallow Pointers and structs in finalizers and expandos"

This reverts commit 2376ab5.

Reason for revert: Importing dart:ffi from dart:core breaks the VM when using --enable-ffi=false

Original change's description:
> [vm/ffi] Disallow `Pointer`s and structs in finalizers and expandos
>
> `Dart_NewWeakPersistentHandle` and `Dart_NewFinalizableHandle` in
> `dart_api.h` do no longer accept `Pointer`s and subtypes of `Struct`s
> as values passed in to the `object` parameter.
>
> Expandos no longer accept `Pointer`s and subtypes of `Struct`s
> as values passed in to the `object` parameter.
>
> Cleans up unused object_store->ffi_struct_class.
>
> Closes: #45071
> Breaking change: #45072
>
> TEST=vm/cc/DartAPI_FinalizableHandleErrors
> TEST=vm/cc/DartAPI_WeakPersistentHandleErrors
> TEST=tests/ffi(_2)/expando_test.dart
>
> Change-Id: I1f11adfa073c7d2c979f3c2bb15c7444c7c767a0
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186280
> Commit-Queue: Daco Harkes <[email protected]>
> Reviewed-by: Martin Kustermann <[email protected]>

[email protected],[email protected],[email protected],[email protected]

Change-Id: Ib76d27e59391dc6107d3f8e8fba3d67640ea9a5c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195060
Reviewed-by: Daco Harkes <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>

Author: dcharkes

CHANGELOG.md

@@ -8,15 +8,6 @@
     daylight saving changes that are not precisely one hour.
     (No change on the Web which uses the JavaScript `Date` object.)
 
-### Dart VM
-
-*   **Breaking Change** [#45071][]: `Dart_NewWeakPersistentHandle`'s and
-    `Dart_NewFinalizableHandle`'s `object` parameter no longer accepts
-    `Pointer`s and subtypes of `Struct`. Expandos no longer accept
-    `Pointer`s and subtypes of `Struct`s.
-
-[#45071]: https://github.com/dart-lang/sdk/issues/45071
-
 ## 2.13.0
 
 ### Language

pkg/vm/lib/transformations/ffi_use_sites.dart

@@ -835,9 +835,7 @@ class _FfiUseSiteTransformer extends FfiTransformer {
           : null;
     }
 
-    if (!nativeTypesClasses.contains(klass) &&
-        klass != arrayClass &&
-        klass != arraySizeClass) {
+    if (!nativeTypesClasses.contains(klass) && klass != arrayClass) {
       for (final parent in nativeTypesClasses) {
         if (hierarchy.isSubtypeOf(klass, parent)) {
           return parent;

runtime/include/dart_api.h

@@ -475,7 +475,7 @@ DART_EXPORT void Dart_DeletePersistentHandle(Dart_PersistentHandle object);
  *
  * Requires there to be a current isolate.
  *
- * \param object An object with identity.
+ * \param object An object.
  * \param peer A pointer to a native object or NULL.  This value is
  *   provided to callback when it is invoked.
  * \param external_allocation_size The number of externally allocated
@@ -531,7 +531,7 @@ DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object,
  *
  * Requires there to be a current isolate.
  *
- * \param object An object with identity. Pointers do not have identity.
+ * \param object An object.
  * \param peer A pointer to a native object or NULL.  This value is
  *   provided to callback when it is invoked.
  * \param external_allocation_size The number of externally allocated

runtime/vm/dart_api_impl.cc

@@ -991,23 +991,6 @@ DART_EXPORT void Dart_SetPersistentHandle(Dart_PersistentHandle obj1,
   obj1_ref->set_ptr(obj2_ref);
 }
 
-// TODO(https://dartbug.com/38491): Reject Unions here as well.
-static bool IsFfiStruct(Thread* T, const Object& obj) {
-  if (obj.IsNull()) {
-    return false;
-  }
-
-  // CFE guarantees we can only have direct subclasses of `Struct`
-  // (no implementations or indirect subclasses are allowed).
-  const auto& klass = Class::Handle(Z, obj.clazz());
-  const auto& super_klass = Class::Handle(Z, klass.SuperClass());
-  if (super_klass.Name() != Symbols::Struct().ptr()) {
-    return false;
-  }
-  const auto& library = Library::Handle(Z, super_klass.library());
-  return library.url() == Symbols::DartFfi().ptr();
-}
-
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
     Thread* thread,
     const Object& ref,
@@ -1017,13 +1000,6 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
   if (!ref.ptr()->IsHeapObject()) {
     return NULL;
   }
-  if (ref.IsPointer()) {
-    return NULL;
-  }
-  if (IsFfiStruct(thread, ref)) {
-    return NULL;
-  }
-
   FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
                                        callback, external_allocation_size,
@@ -1032,14 +1008,16 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
 }
 
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
-    Thread* T,
+    Thread* thread,
     Dart_Handle object,
     void* peer,
     intptr_t external_allocation_size,
     Dart_HandleFinalizer callback) {
-  const auto& ref = Object::Handle(Z, Api::UnwrapHandle(object));
-  return AllocateWeakPersistentHandle(T, ref, peer, external_allocation_size,
-                                      callback);
+  REUSABLE_OBJECT_HANDLESCOPE(thread);
+  Object& ref = thread->ObjectHandle();
+  ref = Api::UnwrapHandle(object);
+  return AllocateWeakPersistentHandle(thread, ref, peer,
+                                      external_allocation_size, callback);
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(
@@ -1051,12 +1029,6 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
   if (!ref.ptr()->IsHeapObject()) {
     return NULL;
   }
-  if (ref.IsPointer()) {
-    return NULL;
-  }
-  if (IsFfiStruct(thread, ref)) {
-    return NULL;
-  }
 
   FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
@@ -1066,13 +1038,15 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(
-    Thread* T,
+    Thread* thread,
     Dart_Handle object,
     void* peer,
     intptr_t external_allocation_size,
     Dart_HandleFinalizer callback) {
-  const auto& ref = Object::Handle(Z, Api::UnwrapHandle(object));
-  return AllocateFinalizableHandle(T, ref, peer, external_allocation_size,
+  REUSABLE_OBJECT_HANDLESCOPE(thread);
+  Object& ref = thread->ObjectHandle();
+  ref = Api::UnwrapHandle(object);
+  return AllocateFinalizableHandle(thread, ref, peer, external_allocation_size,
                                    callback);
 }
 
@@ -1081,27 +1055,30 @@ Dart_NewWeakPersistentHandle(Dart_Handle object,
                              void* peer,
                              intptr_t external_allocation_size,
                              Dart_HandleFinalizer callback) {
-  DARTSCOPE(Thread::Current());
+  Thread* thread = Thread::Current();
+  CHECK_ISOLATE(thread->isolate());
   if (callback == NULL) {
     return NULL;
   }
+  TransitionNativeToVM transition(thread);
 
-  return AllocateWeakPersistentHandle(T, object, peer, external_allocation_size,
-                                      callback);
+  return AllocateWeakPersistentHandle(thread, object, peer,
+                                      external_allocation_size, callback);
 }
 
 DART_EXPORT Dart_FinalizableHandle
 Dart_NewFinalizableHandle(Dart_Handle object,
                           void* peer,
                           intptr_t external_allocation_size,
                           Dart_HandleFinalizer callback) {
-  DARTSCOPE(Thread::Current());
+  Thread* thread = Thread::Current();
+  CHECK_ISOLATE(thread->isolate());
   if (callback == nullptr) {
     return nullptr;
   }
-
-  return AllocateFinalizableHandle(T, object, peer, external_allocation_size,
-                                   callback);
+  TransitionNativeToVM transition(thread);
+  return AllocateFinalizableHandle(thread, object, peer,
+                                   external_allocation_size, callback);
 }
 
 DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object,

runtime/vm/dart_api_impl_test.cc

@@ -3368,35 +3368,6 @@ TEST_CASE(DartAPI_WeakPersistentHandleErrors) {
       Dart_NewWeakPersistentHandle(obj2, NULL, 0, NopCallback);
   EXPECT_EQ(ref2, static_cast<void*>(NULL));
 
-  // Pointer object.
-  Dart_Handle ffi_lib = Dart_LookupLibrary(NewString("dart:ffi"));
-  Dart_Handle pointer_type =
-      Dart_GetNonNullableType(ffi_lib, NewString("Pointer"), 0, NULL);
-  Dart_Handle obj3 = Dart_Allocate(pointer_type);
-  EXPECT_VALID(obj3);
-  Dart_WeakPersistentHandle ref3 =
-      Dart_NewWeakPersistentHandle(obj3, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref3, static_cast<void*>(nullptr));
-
-  // Subtype of Struct object.
-  const char* kScriptChars = R"(
-      import 'dart:ffi';
-
-      class MyStruct extends Struct {
-        external Pointer notEmpty;
-      }
-  )";
-  Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
-  Dart_Handle my_struct_type =
-      Dart_GetNonNullableType(lib, NewString("MyStruct"), 0, NULL);
-  Dart_Handle obj4 = Dart_Allocate(my_struct_type);
-  EXPECT_VALID(obj4);
-  Dart_WeakPersistentHandle ref4 =
-      Dart_NewWeakPersistentHandle(obj4, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref4, static_cast<void*>(nullptr));
-
-  // TODO(https://dartbug.com/38491): Reject Unions here as well.
-
   Dart_ExitScope();
 }
 
@@ -3417,33 +3388,6 @@ TEST_CASE(DartAPI_FinalizableHandleErrors) {
       Dart_NewFinalizableHandle(obj2, nullptr, 0, FinalizableHandleCallback);
   EXPECT_EQ(ref2, static_cast<void*>(nullptr));
 
-  // Pointer object.
-  Dart_Handle ffi_lib = Dart_LookupLibrary(NewString("dart:ffi"));
-  Dart_Handle pointer_type =
-      Dart_GetNonNullableType(ffi_lib, NewString("Pointer"), 0, NULL);
-  Dart_Handle obj3 = Dart_Allocate(pointer_type);
-  EXPECT_VALID(obj3);
-  Dart_FinalizableHandle ref3 =
-      Dart_NewFinalizableHandle(obj3, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref3, static_cast<void*>(nullptr));
-
-  // Subtype of Struct object.
-  const char* kScriptChars = R"(
-      import 'dart:ffi';
-
-      class MyStruct extends Struct {
-        external Pointer notEmpty;
-      }
-  )";
-  Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
-  Dart_Handle my_struct_type =
-      Dart_GetNonNullableType(lib, NewString("MyStruct"), 0, NULL);
-  Dart_Handle obj4 = Dart_Allocate(my_struct_type);
-  EXPECT_VALID(obj4);
-  Dart_FinalizableHandle ref4 =
-      Dart_NewFinalizableHandle(obj4, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref4, static_cast<void*>(nullptr));
-
   Dart_ExitScope();
 }
 

runtime/vm/object_store.h

@@ -236,6 +236,7 @@ class ObjectPointerVisitor;
   RW(GrowableObjectArray, ffi_callback_functions)                              \
   RW(Class, ffi_pointer_class)                                                 \
   RW(Class, ffi_native_type_class)                                             \
+  RW(Class, ffi_struct_class)                                                  \
   RW(Object, ffi_as_function_internal)                                         \
   // Please remember the last entry must be referred in the 'to' function below.
 

sdk/lib/_internal/vm/lib/core_patch.dart

@@ -49,8 +49,6 @@ import "dart:collection"
 
 import "dart:convert" show ascii, Encoding, json, latin1, utf8;
 
-import "dart:ffi" show Pointer, Struct;
-
 import "dart:isolate" show Isolate;
 
 import "dart:math" show Random;

sdk/lib/_internal/vm/lib/expando_patch.dart

@@ -140,12 +140,9 @@ class Expando<T> {
     if ((object == null) ||
         (object is bool) ||
         (object is num) ||
-        (object is String) ||
-        (object is Pointer) ||
-        (object is Struct)) {
-      // TODO(https://dartbug.com/38491): Reject Unions here as well.
+        (object is String)) {
       throw new ArgumentError.value(object,
-          "Expandos are not allowed on strings, numbers, booleans, null, Pointers or Structs.");
+          "Expandos are not allowed on strings, numbers, booleans or null");
     }
   }
 

sdk/lib/core/expando.dart

@@ -6,8 +6,7 @@ part of dart.core;
 
 /// An [Expando] allows adding new properties to objects.
 ///
-/// Does not work on numbers, strings, booleans, `null`, `dart:ffi` pointers
-/// or `dart:ffi` structs.
+/// Does not work on numbers, strings, booleans or `null`.
 ///
 /// An `Expando` does not hold on to the added property value after an object
 /// becomes inaccessible.
@@ -39,15 +38,13 @@ class Expando<T extends Object> {
   /// object. If the object hasn't been expanded, the method returns
   /// `null`.
   ///
-  /// The object must not be a number, a string, a boolean, `null`, a
-  /// `dart:ffi` pointer, or a `dart:ffi` struct.
+  /// The object must not be a number, a string or a boolean.
   external T? operator [](Object object);
 
   /// Sets the value of this [Expando]'s property on the given
   /// object. Properties can effectively be removed again by setting
   /// their value to `null`.
   ///
-  /// The object must not be a number, a string, a boolean, `null`, a
-  /// `dart:ffi` pointer, or a `dart:ffi` struct.
+  /// The object must not be a number, a string or a boolean.
   external void operator []=(Object object, T? value);
 }