Make weak handles created via Dart_NewWeakPersistentHandle not auto-delete.

[mkustermann]

Right now the VM's weak handle API exposed to embedders is a bit fragile to use: After creation of a weak persistent handle via Dart_NewWeakPersistentHandle it is only save to use it (e.g. via Dart_HandleFromWeakPersistent, Dart_DeleteWeakPersistentHandle) if one knows that the object itself is still alive.

Once the object in the weak persistent handle is unreachable any following Garbage Collection can reclaim the object and in doing so will delete the weak persistent handle as well.

See the code in scavenger.cc:

  void VisitHandle(uword addr) {
    auto handle = reinterpret_cast<FinalizablePersistentHandle*>(addr);
    ObjectPtr* p = handle->raw_addr();
    if (scavenger_->IsUnreachable(p)) {
      handle->UpdateUnreachable(thread()->isolate_group());
    }
    ...
  }

which eventually calls dart_api_impl.cc:FinalizablePersistentHandle::Finalize

  ...
  state->FreeWeakPersistentHandle(handle);

=> The only usage pattern when this is safe is when the weak handle is only accessed if the code is guaranteed to have a strong reference to it (i.e. the object in the weak handle is still alive).

If the object is being deleted by the GC and native code tries to delete the weak handle on another thread (or get the object out of the weak handle) by calling the Dart API then the two threads are racing and it might lead to undefined behavior. It is not possible to prevent this via simple locking because that can lead to a deadlock.

Safe usage pattern

One usage pattern which is safe is e.g. this:

1. A resource class inherits from a native field wrapper class, e.g. class _File extends NativeFieldWrapperClass1 { ... }
2. The constructor of such an object will make a new weak handle via Dart_NewWeakPersistentHandle from the file object handle and set the native field of the file to be this weak handle via Dart_SetNativeInstanceField.
3. An explicit file.close() will call native code and get the weak handle from the file object and deletes it via Dart_DeleteWeakPersistentHandle.

Notice that step 3 guarantees that the file object is still alive when the weak handle gets deleted.

In case a user forgets to explicitly close the file, the GC will invoke the weak handle callback and automatically deletes the handle afterwards.

Proposed changes

We should consider changing our existing weak handle API to not auto-delete and migrate our embedders.

We can then introduce a new API to support finalization and safe auto-delete of finalizable handles by requiring the user to proof he/she has access to the object being eagerly finalized, e.g.

void Finalizer(int fd) {
   close(fd);
}

void FUNCTION_NAME(File_File)(Dart_NativeArguments args) {
  Dart_Handle file_object = ...;
  int fd = open(...);
  auto finalizable_handle = Dart_NewFinalizableHandle(file_object, Finalizer, fd);
  Dart_SetNativeInstanceField(file_object, 0, fd);
  Dart_SetNativeInstanceField(file_object, 1, finalizable_handle);
}

void FUNCTION_NAME(File_Close)(Dart_NativeArguments args) {
  Dart_Handle file_object = ...;
  auto fd = Dart_GetNativeInstanceField(file_object, 1);
  auto finalizable_handle = Dart_GetNativeInstanceField(file_object, 1);
  Dart_DeleteFinalizableHandle(finalizable_handle, file_object);
  close(fd);
}

By forcing the programmer to provide the original object to Dart_DeleteFinalizableHandle we know it's still alive.

/cc @rmacnak-google @mraleph @dcharkes @alexmarkov

[alexmarkov]

There is still a danger that Dart_HandleFromWeakPersistent could be used to get the original object from a handle and pass it to Dart_DeleteFinalizableHandle. That would work most of the time but would still suffer from the same race. So, in addition to passing original object into Dart_DeleteFinalizableHandle we might need to disallow getting an object out of finalizable handles.

[dcharkes]

Because we have both use cases that require weak handles (being able to dereference them) and finalizers we should then have two types in the API distinguishing these handles.

• Dart_FinalizableHandle that gets returned from attaching a finalizer, and that requires a Dart_Handle to delete. These are auto-deleted on running the finalizer. Dereferencing is disallowed.
• Dart_WeakPersistentHandle that gets returned from getting a weak handle without a finalizer, that one can safely dereference, and delete without having a Dart_Handle to it. This one does not auto-delete itself.

[mkustermann]

There is still a danger that Dart_HandleFromWeakPersistent could be used to get the original
object from a handle and pass it to Dart_DeleteFinalizableHandle. That would work most of the
time but would still suffer from the same race. So, in addition to passing original object into
Dart_DeleteFinalizableHandle we might need to disallow getting an object out of finalizable
handles.

Yes, the Dart_*FinalizableHandle* api functions would not include getting the object out of the finalizable handle. All of them would take the object as explicit parameter to ensure the auto-deleting finalizable handle is still alive.

[dcharkes]

Progress update on exploring splitting the API between Dart_FinalizableHandle (auto deleting) and Dart_WeakPersistenHandle (not auto deleting). The API would look something like this:

There are a couple of caveats with the split:

1. In order to aid migration of Dart embedders and g3, we cannot change the semantics of Dart_NewWeakPersistentHandle to become non-deleting. Instead we need to deprecate it but support it for a while. That means we need to introduce Dart_NewWeakPersistentHandleV2 with the correct semantics. Any naming suggestions?

2. Splitting Dart_FinalizableHandle and Dart_WeakPersistenHandle also creates a split between Dart_WeakPersistentHandleFinalizer and Dart_FinalizableHandleFinalizer. Which in turn prompts us to migrate Dart_NewExternalLatin1String, Dart_NewExternalUTF16String, and Dart_NewExternalTypedDataWithFinalizer, which would also create V2 versions of those. (The weak handles of those fuctions are not exposed, so we could skip them, but they are auto deleting so it leaves a naming inconsistency).

3. Splitting Dart_FinalizableHandle and Dart_WeakPersistenHandle requires us to duplicate Dart_UpdateExternalSize.

Alternatives:

A. typedef Dart_WeakPersistentHandle Dart_FinalizableHandle;

• Prevents caveat 2 and 3.
• Provides only documentation, no static checking for API users.

B. Add an auto_delete param to Dart_NewWeakPersistentHandle (see old CL). This enables using the existing api with both auto deleting and non auto deleting semantics.

• prevents caveat 1, 2, and 3.
• Provides no documentation, and no static checking.

Both alternatives don't give any static checking for the problem described in this issue. So, IMO, they are not good alternatives.

[mkustermann]

In order to aid migration of Dart embedders and g3, we cannot change the semantics of
Dart_NewWeakPersistentHandle to become non-deleting.

What about: We introduce Dart_*FinalizableHandle* API as first step. Then migrate our existing embedders to use it. Then we do the breaking change to the Dart_*WeakHandle* APIs.

[dcharkes]

In order to aid migration of Dart embedders and g3, we cannot change the semantics of
Dart_NewWeakPersistentHandle to become non-deleting.

What about: We introduce Dart_*FinalizableHandle* API as first step. Then migrate our existing embedders to use it. Then we do the breaking change to the Dart_*WeakHandle* APIs.

DartWrappable in Flutter/engine requires migration to non-auto-deleting WPHs, because it features being able to get a handle out of a weak handle (see Flutter/engine patch). We'll have to roll manually to avoid the V2 symbols.

[mkustermann]

I believe everything we wanted to do here has been implemented. Closing.