Fix #835 #1203

liamappelbe · 2024-06-11T01:07:40Z

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 · 2024-06-11T02:59:42Z

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 · 2024-06-11T03:05:54Z

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

coveralls · 2024-06-11T03:32:07Z

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

coveralls · 2024-06-11T04:02:32Z

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

coveralls · 2024-06-11T04:51:27Z

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

coveralls · 2024-06-11T04:54:14Z

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

dcharkes · 2024-06-12T09:20:32Z

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 · 2024-06-17T11:51:04Z

pkgs/ffigen/lib/src/code_generator/func_type.dart

+  @override
+  String getNativeType(String varName) {
+    final arg = dartTypeParameters.map<String>((p) => p.type.getNativeType(''));
+    return '${returnType.getNativeType('')} (*$varName)(${arg.join(', ')})';


should varName param be optional instead of passing ''?

coveralls · 2024-06-18T00:24:05Z

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

coveralls · 2024-06-18T00:33:27Z

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

liamappelbe added 3 commits June 7, 2024 11:20

Fix some block ref counting tests

570955d

Add a test that reproduces the bug.

98239b6

Generate the trampoline

225b8b3

github-actions bot added the package:ffigen label Jun 11, 2024

fixes

c1247ac

github-actions bot added the package:objective_c label Jun 11, 2024

Merge branch 'main' into gh835

f90097d

Update json schema

37289db

liamappelbe marked this pull request as ready for review June 11, 2024 03:26

liamappelbe requested a review from dcharkes June 11, 2024 03:26

Fix NPE

e1877f5

Fix swift test setup

42c8637

dcharkes mentioned this pull request Jun 12, 2024

[ffigen] ObjCBlock.listener refcounting issues #835

Closed

dcharkes approved these changes Jun 17, 2024

View reviewed changes

liamappelbe added 2 commits June 18, 2024 10:44

Merge branch 'main' into gh835

270ee56

Make varName optional

13a4469

liamappelbe merged commit 9329b80 into main Jun 18, 2024
19 checks passed

liamappelbe deleted the gh835 branch June 18, 2024 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #835 #1203

Fix #835 #1203

liamappelbe commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

dcharkes commented Jun 12, 2024

dcharkes left a comment

dcharkes Jun 17, 2024

liamappelbe Jun 18, 2024

coveralls commented Jun 18, 2024

coveralls commented Jun 18, 2024

Fix #835 #1203

Fix #835 #1203

Conversation

liamappelbe commented Jun 11, 2024 • edited Loading

github-actions bot commented Jun 11, 2024 • edited Loading

PR Health

Changelog Entry ✔️

Coverage ⚠️

License Headers ✔️

Package publish validation ✔️

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

coveralls commented Jun 11, 2024

dcharkes commented Jun 12, 2024

dcharkes left a comment

Choose a reason for hiding this comment

dcharkes Jun 17, 2024

Choose a reason for hiding this comment

liamappelbe Jun 18, 2024

Choose a reason for hiding this comment

coveralls commented Jun 18, 2024

coveralls commented Jun 18, 2024

liamappelbe commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading