Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ffigen] ObjCBlock.listener refcounting issues #835

Closed
liamappelbe opened this issue Nov 27, 2023 · 5 comments
Closed

[ffigen] ObjCBlock.listener refcounting issues #835

liamappelbe opened this issue Nov 27, 2023 · 5 comments

Comments

@liamappelbe
Copy link
Contributor

ObjCBlock.listener lets ObjC code invoke Dart callbacks that live in other isolates. If they pass an object arg, it's possible that it gets refcounted and destroyed before the Dart callback runs (the Dart code calls retain, but it may be too late). So while these listener callbacks let us get rid of a lot of NativePort boilerplate, they don't totally eliminate the need for native code.

These trampolines are super simple and can be automatically generated by ffigen. But automatically compiling them relies on native_assets.

@liamappelbe liamappelbe added package:ffigen lang-objective_c Related to Objective C support labels Nov 27, 2023
domesticmouse pushed a commit to flutter/samples that referenced this issue Dec 6, 2023
Since ffigen added support for
[`NativeCallable.listener`](https://api.flutter.dev/flutter/dart-ffi/NativeCallable/NativeCallable.listener.html)
to its ObjC bindings, this example can be simplified. We can replace the
`Dart_Port` logic with `ObjCBlock.listener`, which lets us get rid of
most of the native code.

We still need a small bit of native code to `retain` a reference to the
callback's arguments before invoking the listener, otherwise the
arguments may be ref counted and deleted before the Dart side of the
callback is invoked. See dart-lang/native#835
@liamappelbe liamappelbe moved this from Todo to In progress in ObjC/Swift interop Apr 18, 2024
@liamappelbe liamappelbe moved this from In progress to Todo in ObjC/Swift interop Apr 18, 2024
@liamappelbe liamappelbe moved this from Todo to In progress in ObjC/Swift interop May 3, 2024
@liamappelbe liamappelbe moved this from In progress to Todo in ObjC/Swift interop Jun 3, 2024
@liamappelbe liamappelbe moved this from Todo to In progress in ObjC/Swift interop Jun 6, 2024
@liamappelbe liamappelbe mentioned this issue Jun 11, 2024
@dcharkes
Copy link
Collaborator

Right, so the problem this is solving is that we don't call retain on the temp isolate that does argument marshalling, but we do the retain calls on the target Dart isolate, which might be arbitrarily delayed.

I see multiple solutions to this issue:

  1. Introduce NativeCallable.blocking.
  2. Allow writing isolate independent code in the temp isolate (e.g. doing an FFI call to retain objc blocks).
  3. Generate some native code that does a retain before calling a NativeCallable.listener, and subsequently not increasing the refcount in Dart code in the target isolate but decreasing the refcount in that target isolate once the ObjCObject is GCed. (Fix #835 #1203).

Option 3 currently has the drawback that the users need to manually include that generated native code in their build. In the future, once we have native assets, users will need to write a build hook that refers to generated files (likely some trivial call to a helper in package:objc).

The downside of option 3 is also that it is not a general solution. @HosseinYousefi How do we deal with this in JNIgen? When is the JavaGlobalReference constructed in callbacks via native ports?

cc @mkustermann @mraleph

@dcharkes
Copy link
Collaborator

dcharkes commented Jun 12, 2024

Also, if increasing the refcount in native code or on the marshaller isolate. How do we ensure the refcount is decreased if the message is never delivered to the target isolate?

  • With some isolate independent code, we could also write some isolate independent code to decrease the refcount if the message is not delivered.

Another alternative not involving isolate independent code:

  • Add two optional native function pointers to NativeCallable.listener one for onMarshalling and one for onFailedDelivery. This would still require bundling native code, but would make increasing and decreasing the refcount symmetric. (And solve the releasing when a message is not delivered.)

@HosseinYousefi
Copy link
Member

The downside of option 3 is also that it is not a general solution. @HosseinYousefi How do we deal with this in JNIgen? When is the JavaGlobalReference constructed in callbacks via native ports?

We block and clean up the global references after invocation.

@dcharkes
Copy link
Collaborator

The downside of option 3 is also that it is not a general solution. @HosseinYousefi How do we deal with this in JNIgen? When is the JavaGlobalReference constructed in callbacks via native ports?

We block and clean up the global references after invocation.

Right, we don't have listener functionality in JNIgen, all calls are as if we had a NativeCallable.blocking. If we do want listener style in JNIgen, we'll run into the same issues I suppose.

@dcharkes
Copy link
Collaborator

From an offline discussion with @mkustermann @liamappelbe et al.

Option:

  1. NativeCallable.onAnyThread. Then we can synchronously increase the refcount. Temporary isolates or shared memory multithreading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

3 participants