Fix #835

[liamappelbe]

There's a bug where args passed to listener blocks are GC'd before the Dart function is invoked. This PR fixes that bug by giving each listener block a small ObjC trampoline that increments the ref count of any ref countable args before invoking the real block.

Here's an example trampoline from the tests (with added comments). This function is invoked at listener construction time. We pass in the block created in Dart, and that block is wrapped in native trampoline block that does the ref counting.

typedef void  (^ListenerBlock)(int32_t  (^)(int32_t ));
ListenerBlock wrapListenerBlock_ObjCBlock_ffiVoid_IntBlock(ListenerBlock block) {
  ListenerBlock wrapper = [^void(int32_t  (^arg0)(int32_t )) {
    block([arg0 copy]);
  } copy];
  // ^ the block literal is allocated on the stack, so we have to
  // call copy to copy it to the heap.

  // The wrapper block now owns a reference to the original
  // block, so release the reference to the original block held
  // by this function scope.
  [block release];

  return wrapper;
}

Details:

• Added infrastructure to the configs and the writer to generate a .m file if there are any bindings that need to generate ObjC code.
• Added Type.getNativeType, which returns the C/ObjC type, as seen in C/ObjC source code.
  • The naming conflicts a bit with Type.getCType, which is the Dart representation of the C type.
  • It's necessary to pass the varName to this function, because there are some C types where the variable name (if any) is embedded in the type: int foo[123], int (*foo)().
• Added Type.generateRetain, which returns a snippet of ObjC code that increments the ref count of the given object. Returns null if the type isn't refcounted.
  • I'm ignoring compound types like structs, unions, and arrays here. It's possible that the correct thing to do is to recursively invoke retain on all of a struct's fields etc. But that will require some more research about what the ObjC runtime does in these cases. The best way to do that research is with a real use case, so I'll defer that until we have one.
• The interesting part of the PR is in objc_block.dart.
  • In addDependencies we decide if a native trampoline is needed. We need a trampoline if there's a listener constructor, and any of the arguments are ref counted. If so, we create _wrapListenerBlock, which is the binding for the native wrapper function we're about to code gen.
  • toObjCBindingString generates the native wrapper function, if necessary. See above for an example.
  • In toBindingString, we still construct the listener block in the same way as before, but if we have a _wrapListenerBlock we pass the constructed block through this function to get a wrapped block. The other thing that changed here is we swapped out convFn for listenerConvFn, which doesn't retain the received args (since the trampoline already retained them).
• Had to update native_objc_test/setup.dart to compile the generated .m file if it exists.
• Note that if the listener is never invoked (eg because the isolate has shut down), the args will leak. This should be a rare occurance.
• I also noticed some old block ref counting tests that should have been fixed by Delete the Dart functions associated with closure blocks #1100, but needed some clean up.

[github-actions]

PR Health

Changelog Entry ✔️

Details

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️

Details

File	Coverage
pkgs/ffigen/lib/src/code_generator/binding.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/compound.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/enum_class.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/func_type.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/handle.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/imports.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/library.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/native_type.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_block.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_interface.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_nullable.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/pointer.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/type.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/typealias.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/writer.dart	💔 Not covered
pkgs/ffigen/lib/src/config_provider/config.dart	💔 Not covered
pkgs/ffigen/lib/src/config_provider/config_types.dart	💔 Not covered
pkgs/ffigen/lib/src/config_provider/spec_utils.dart	💔 Not covered
pkgs/ffigen/lib/src/executables/ffigen.dart	💔 Not covered
pkgs/ffigen/lib/src/strings.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️

Details

// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffi/example/main.dart
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_cli/lib/src/api/builder.dart
pkgs/native_assets_cli/lib/src/api/linker.dart
pkgs/native_assets_cli/test/model/checksum_test.dart

Package publish validation ✔️

Details

Package	Version	Status
package:ffi	2.1.2	already published at pub.dev
package:ffigen	13.0.0-wip	WIP (no publish necessary)
package:jni	0.9.3-wip	WIP (no publish necessary)
package:jnigen	0.9.2	already published at pub.dev
package:native_assets_cli	0.6.1-wip	WIP (no publish necessary)
package:objective_c	1.1.0-wip	WIP (no publish necessary)
package:swiftgen	0.0.1-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[coveralls]

coverage: 92.183% (+1.0%) from 91.215%
when pulling f90097d on gh835
into ecb8b7e on main.

[coveralls]

coverage: 92.183% (+1.0%) from 91.215%
when pulling 37289db on gh835
into ecb8b7e on main.

[coveralls]

coverage: 92.223% (+1.0%) from 91.215%
when pulling e1877f5 on gh835
into ecb8b7e on main.

[coveralls]

coverage: 92.223% (+1.0%) from 91.215%
when pulling 42c8637 on gh835
into ecb8b7e on main.

[coveralls]

coverage: 92.179% (+1.0%) from 91.215%
when pulling 42c8637 on gh835
into ecb8b7e on main.

[dcharkes]

Let's explore more general options to this problem first. I think this might be an issue for more than just ObjectiveC refcounts.

[dcharkes]

LGTM!

[dcharkes]

should varName param be optional instead of passing ''?

[liamappelbe]

Done

[coveralls]

coverage: 92.282% (+0.2%) from 92.123%
when pulling 13a4469 on gh835
into a44ef95 on main.

[coveralls]

coverage: 92.669% (+0.5%) from 92.123%
when pulling 13a4469 on gh835
into a44ef95 on main.