[vm] Native API: Make Dart_NewWeakPersistentHandle not auto delete

Changes Dart_NewWeakPersistentHandle to no longer auto delete the
weak persistent handle.

Changes the signatures of WeakPersistentHandleFinalizers to no longer
have access to the handle.

Flutter PR: flutter/engine#19843
g3 presubmit: cl/318028238

Issue: #42312

TEST=runtime/vm/dart_api_impl_test.cc

Change-Id: I3f77db9954d9486759f903b78c03a494f73c68ba
Cq-Include-Trybots:dart/try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-msan-linux-release-x64-try,vm-kernel-precomp-msan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-mac-debug-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-nnbd-linux-debug-x64-try,analyzer-nnbd-linux-release-try,front-end-nnbd-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151525
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: dcharkes

CHANGELOG.md

@@ -27,6 +27,16 @@ opted out of null safety by adding `// @dart=2.9` to the beginning of the file.
 
 ### Dart VM
 
+*   **Breaking Change** [#42312][]: `Dart_WeakPersistentHandle`s will no longer
+    auto-delete themselves when the referenced object is garbage collected to
+    avoid race conditions, but they are still automatically deleted when the
+    isolate group shuts down.
+*   **Breaking Change** [#42312][]: `Dart_WeakPersistentHandleFinalizer`
+    is renamed to `Dart_HandleFinalizer` and had its `handle` argument removed.
+    All api functions using that type have been updated.
+
+[#42312]: https://github.com/dart-lang/sdk/issues/42312
+
 ### Dart2JS
 
 * Removed `--no-defer-class-types` and `--no-new-deferred-split`.

runtime/bin/dartutils.cc

@@ -905,11 +905,10 @@ Dart_CObject* CObject::NewUint32Array(intptr_t length) {
   return cobject;
 }
 
-Dart_CObject* CObject::NewExternalUint8Array(
-    intptr_t length,
-    uint8_t* data,
-    void* peer,
-    Dart_WeakPersistentHandleFinalizer callback) {
+Dart_CObject* CObject::NewExternalUint8Array(intptr_t length,
+                                             uint8_t* data,
+                                             void* peer,
+                                             Dart_HandleFinalizer callback) {
   Dart_CObject* cobject = New(Dart_CObject_kExternalTypedData);
   cobject->value.as_external_typed_data.type = Dart_TypedData_kUint8;
   cobject->value.as_external_typed_data.length = length;
@@ -937,7 +936,7 @@ Dart_CObject* CObject::NewIOBuffer(int64_t length) {
 void CObject::FreeIOBufferData(Dart_CObject* cobject) {
   ASSERT(cobject->type == Dart_CObject_kExternalTypedData);
   cobject->value.as_external_typed_data.callback(
-      NULL, NULL, cobject->value.as_external_typed_data.peer);
+      NULL, cobject->value.as_external_typed_data.peer);
   cobject->value.as_external_typed_data.data = NULL;
 }
 

runtime/bin/dartutils.h

@@ -344,11 +344,10 @@ class CObject {
   static Dart_CObject* NewArray(intptr_t length);
   static Dart_CObject* NewUint8Array(intptr_t length);
   static Dart_CObject* NewUint32Array(intptr_t length);
-  static Dart_CObject* NewExternalUint8Array(
-      intptr_t length,
-      uint8_t* data,
-      void* peer,
-      Dart_WeakPersistentHandleFinalizer callback);
+  static Dart_CObject* NewExternalUint8Array(intptr_t length,
+                                             uint8_t* data,
+                                             void* peer,
+                                             Dart_HandleFinalizer callback);
 
   static Dart_CObject* NewIOBuffer(int64_t length);
   static void FreeIOBufferData(Dart_CObject* object);
@@ -571,7 +570,7 @@ class CObjectExternalUint8Array : public CObject {
   }
   uint8_t* Data() const { return cobject_->value.as_external_typed_data.data; }
   void* Peer() const { return cobject_->value.as_external_typed_data.peer; }
-  Dart_WeakPersistentHandleFinalizer Callback() const {
+  Dart_HandleFinalizer Callback() const {
     return cobject_->value.as_external_typed_data.callback;
   }
 

runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc

@@ -475,7 +475,7 @@ DART_EXPORT void ExecuteCallback(Work* work_ptr) {
 
 Dart_Port send_port_;
 
-static void FreeFinalizer(void*, Dart_WeakPersistentHandle, void* value) {
+static void FreeFinalizer(void*, void* value) {
   free(value);
 }
 
@@ -776,9 +776,7 @@ DART_EXPORT void ThreadPoolTest_BarrierSync(
 // vmspecific_handle_test.dart (statically linked).
 // vmspecific_handle_dynamically_linked_test.dart (dynamically linked).
 
-static void RunFinalizer(void* isolate_callback_data,
-                         Dart_WeakPersistentHandle handle,
-                         void* peer) {
+static void RunFinalizer(void* isolate_callback_data, void* peer) {
   printf("Running finalizer for weak handle.\n");
 }
 

runtime/bin/io_buffer.h

@@ -25,9 +25,7 @@ class IOBuffer {
   static void Free(void* buffer) { free(buffer); }
 
   // Function for finalizing external byte arrays used as IO buffers.
-  static void Finalizer(void* isolate_callback_data,
-                        Dart_WeakPersistentHandle handle,
-                        void* buffer) {
+  static void Finalizer(void* isolate_callback_data, void* buffer) {
     Free(buffer);
   }
 

runtime/include/dart_api.h

@@ -244,8 +244,9 @@ typedef struct _Dart_IsolateGroup* Dart_IsolateGroup;
  * the object is garbage collected. It is never safe to use these handles
  * unless you know the object is still reachable.
  *
- * WeakPersistentHandles are persistent handles which are auto deleted
- * when the object is garbage collected.
+ * WeakPersistentHandles are persistent handles which are automatically set
+ * to point Dart_Null when the object is garbage collected. They are not auto
+ * deleted, so it is safe to use them after the object has become unreachable.
  */
 typedef struct _Dart_Handle* Dart_Handle;
 typedef Dart_Handle Dart_PersistentHandle;
@@ -254,10 +255,6 @@ typedef struct _Dart_FinalizableHandle* Dart_FinalizableHandle;
 // These structs are versioned by DART_API_DL_MAJOR_VERSION, bump the
 // version when changing this struct.
 
-typedef void (*Dart_WeakPersistentHandleFinalizer)(
-    void* isolate_callback_data,
-    Dart_WeakPersistentHandle handle,
-    void* peer);
 typedef void (*Dart_HandleFinalizer)(void* isolate_callback_data, void* peer);
 
 /**
@@ -423,6 +420,8 @@ DART_EXPORT Dart_Handle Dart_HandleFromPersistent(Dart_PersistentHandle object);
 
 /**
  * Allocates a handle in the current scope from a weak persistent handle.
+ *
+ * This will be a handle to Dart_Null if the object has been garbage collected.
  */
 DART_EXPORT Dart_Handle
 Dart_HandleFromWeakPersistent(Dart_WeakPersistentHandle object);
@@ -461,24 +460,18 @@ DART_EXPORT void Dart_DeletePersistentHandle(Dart_PersistentHandle object);
 /**
  * Allocates a weak persistent handle for an object.
  *
- * This handle has the lifetime of the current isolate unless the object
- * pointed to by the handle is garbage collected, in this case the VM
- * automatically deletes the handle after invoking the callback associated
- * with the handle. The handle can also be explicitly deallocated by
- * calling Dart_DeleteWeakPersistentHandle.
- *
- * If the object becomes unreachable the callback is invoked with the weak
- * persistent handle and the peer as arguments. The callback can be executed on
- * any thread, will have an isolate group, but will not have a current isolate.
- * The callback can only call Dart_DeletePersistentHandle or
- * Dart_DeleteWeakPersistentHandle. The callback must not call
- * Dart_DeleteWeakPersistentHandle for the handle being finalized, as it is
- * automatically deleted by the VM after the callback returns. This gives the
- * embedder the ability to cleanup data associated with the object and clear
- * out any cached references to the handle. All references to this handle after
- * the callback will be invalid. It is illegal to call into the VM with any
- * other Dart_* functions from the callback. If the handle is deleted before
- * the object becomes unreachable, the callback is never invoked.
+ * This handle has the lifetime of the current isolate. The handle can also be
+ * explicitly deallocated by calling Dart_DeleteWeakPersistentHandle.
+ *
+ * If the object becomes unreachable the callback is invoked with the peer as
+ * argument. The callback can be executed on any thread, will have a current
+ * isolate group, but will not have a current isolate. The callback can only
+ * call Dart_DeletePersistentHandle or Dart_DeleteWeakPersistentHandle. This
+ * gives the embedder the ability to cleanup data associated with the object.
+ * The handle will point to the Dart_Null object after the finalizer has been
+ * run. It is illegal to call into the VM with any other Dart_* functions from
+ * the callback. If the handle is deleted before the object becomes
+ * unreachable, the callback is never invoked.
  *
  * Requires there to be a current isolate.
  *
@@ -498,7 +491,7 @@ DART_EXPORT Dart_WeakPersistentHandle
 Dart_NewWeakPersistentHandle(Dart_Handle object,
                              void* peer,
                              intptr_t external_allocation_size,
-                             Dart_WeakPersistentHandleFinalizer callback);
+                             Dart_HandleFinalizer callback);
 
 /**
  * Deletes the given weak persistent [object] handle.
@@ -2046,7 +2039,7 @@ Dart_NewExternalLatin1String(const uint8_t* latin1_array,
                              intptr_t length,
                              void* peer,
                              intptr_t external_allocation_size,
-                             Dart_WeakPersistentHandleFinalizer callback);
+                             Dart_HandleFinalizer callback);
 
 /**
  * Returns a String which references an external array of UTF-16 encoded
@@ -2067,7 +2060,7 @@ Dart_NewExternalUTF16String(const uint16_t* utf16_array,
                             intptr_t length,
                             void* peer,
                             intptr_t external_allocation_size,
-                            Dart_WeakPersistentHandleFinalizer callback);
+                            Dart_HandleFinalizer callback);
 
 /**
  * Gets the C string representation of a String.
@@ -2440,13 +2433,13 @@ DART_EXPORT Dart_Handle Dart_NewExternalTypedData(Dart_TypedData_Type type,
  * \return The TypedData object if no error occurs. Otherwise returns
  *   an error handle.
  */
-DART_EXPORT Dart_Handle Dart_NewExternalTypedDataWithFinalizer(
-    Dart_TypedData_Type type,
-    void* data,
-    intptr_t length,
-    void* peer,
-    intptr_t external_allocation_size,
-    Dart_WeakPersistentHandleFinalizer callback);
+DART_EXPORT Dart_Handle
+Dart_NewExternalTypedDataWithFinalizer(Dart_TypedData_Type type,
+                                       void* data,
+                                       intptr_t length,
+                                       void* peer,
+                                       intptr_t external_allocation_size,
+                                       Dart_HandleFinalizer callback);
 
 /**
  * Returns a ByteBuffer object for the typed data.

runtime/include/dart_api_dl.h

@@ -84,7 +84,7 @@ typedef void (*Dart_NativeMessageHandler_DL)(Dart_Port_DL dest_port_id,
   F(Dart_DeletePersistentHandle, void, (Dart_PersistentHandle object))         \
   F(Dart_NewWeakPersistentHandle, Dart_WeakPersistentHandle,                   \
     (Dart_Handle object, void* peer, intptr_t external_allocation_size,        \
-     Dart_WeakPersistentHandleFinalizer callback))                             \
+     Dart_HandleFinalizer callback))                                           \
   F(Dart_DeleteWeakPersistentHandle, void, (Dart_WeakPersistentHandle object)) \
   F(Dart_UpdateExternalSize, void,                                             \
     (Dart_WeakPersistentHandle object, intptr_t external_allocation_size))     \

runtime/include/dart_native_api.h

@@ -31,8 +31,7 @@
  * The data for kTypedData is copied on message send and ownership remains with
  * the caller. The ownership of data for kExternalTyped is passed to the VM on
  * message send and returned when the VM invokes the
- * Dart_WeakPersistentHandleFinalizer callback; a non-NULL callback must be
- * provided.
+ * Dart_HandleFinalizer callback; a non-NULL callback must be provided.
  */
 typedef enum {
   Dart_CObject_kNull = 0,
@@ -79,7 +78,7 @@ typedef struct _Dart_CObject {
       intptr_t length;
       uint8_t* data;
       void* peer;
-      Dart_WeakPersistentHandleFinalizer callback;
+      Dart_HandleFinalizer callback;
     } as_external_typed_data;
   } value;
 } Dart_CObject;

runtime/include/dart_version.h

@@ -10,7 +10,7 @@
 // On breaking changes the major version is increased.
 // On backwards compatible changes the minor version is increased.
 // The versioning covers the symbols exposed in dart_api_dl.h
-#define DART_API_DL_MAJOR_VERSION 1
-#define DART_API_DL_MINOR_VERSION 1
+#define DART_API_DL_MAJOR_VERSION 2
+#define DART_API_DL_MINOR_VERSION 0
 
 #endif /* RUNTIME_INCLUDE_DART_VERSION_H_ */ /* NOLINT */

runtime/lib/vmservice.cc

@@ -72,7 +72,7 @@ DEFINE_NATIVE_ENTRY(VMService_SendRootServiceMessage, 0, 1) {
 DEFINE_NATIVE_ENTRY(VMService_SendObjectRootServiceMessage, 0, 1) {
 #ifndef PRODUCT
   GET_NON_NULL_NATIVE_ARGUMENT(Array, message, arguments->NativeArgAt(0));
-    return Service::HandleObjectRootMessage(message);
+  return Service::HandleObjectRootMessage(message);
 #endif
   return Object::null();
 }
@@ -307,16 +307,12 @@ class TarArchive {
   DISALLOW_COPY_AND_ASSIGN(TarArchive);
 };
 
-static void ContentsFinalizer(void* isolate_callback_data,
-                              Dart_WeakPersistentHandle handle,
-                              void* peer) {
+static void ContentsFinalizer(void* isolate_callback_data, void* peer) {
   uint8_t* data = reinterpret_cast<uint8_t*>(peer);
   delete[] data;
 }
 
-static void FilenameFinalizer(void* isolate_callback_data,
-                              Dart_WeakPersistentHandle handle,
-                              void* peer) {
+static void FilenameFinalizer(void* isolate_callback_data, void* peer) {
   char* filename = reinterpret_cast<char*>(peer);
   delete[] filename;
 }