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] Release all ObjC references on the main thread #1603

Merged
merged 37 commits into from
Sep 29, 2024
Merged

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Sep 26, 2024

Certain objects (typically the UI APIs) can only be destroyed on the main thread. Since there's no way of telling which objects have this restriction, we just dispatch all release calls to the main thread.

Fixes: #1470

Notes:

  • The core of this change is in pkgs/objective_c/src/objective_c.(c/h/m), and it just sends the objc_release(peer) call to the main thread using ObjC's dispatch_async(dispatch_get_main_queue(), ...) API, unless we're already on the main thread.
  • However, while the flutter runner does flush the main dispatch queue, the Dart VM doesn't (so the releases are never run). So we also have a C macro to disable this dispatching behavior, which builds targeted at the Dart VM will need to enable. This isn't a problem because those thread restricted APIs can't be used in the Dart VM anyway. In fact the only time we really use package:objective_c in the Dart VM is in tests, and that's just because it's faster and more reliable for doing GC tests.
  • Since running in the VM is the exception, not the rule, the default behavior (when no macro is defined) is to use the dispatcher, and it must be explicitly disabled. That way we won't accidentally forget to enable the macro in one of the plugin build flows.
  • For testing, these files are built using various setup.dart scripts, so I added a flag to those scripts to enable the macro. These flags default in the other direction (they do define the macro by default), since all of our tests run on the Dart VM, except for one github workflow.
  • Had to split arc_test.m into arc_test.h/m

@github-actions github-actions bot added type-infra A repository infrastructure change or enhancement package:ffigen labels Sep 26, 2024
Copy link

github-actions bot commented Sep 26, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

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

Coverage ✔️
File Coverage

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

License Headers ✔️
// 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/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/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/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_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart
Package publish validation ✔️
Package Version Status
package:ffi 2.1.3 already published at pub.dev
package:ffigen 15.0.0-wip WIP (no publish necessary)
package:jni 0.12.0-wip WIP (no publish necessary)
package:jnigen 0.12.0-wip WIP (no publish necessary)
package:native_assets_cli 0.9.0-wip WIP (no publish necessary)
package:objective_c 2.1.0-wip WIP (no publish necessary)
package:swift2objc 0.0.1-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.

@liamappelbe liamappelbe changed the title [ffigen] Release all ObjC references on the main thread WIP: [ffigen] Release all ObjC references on the main thread Sep 26, 2024
@coveralls
Copy link

coveralls commented Sep 26, 2024

Coverage Status

coverage: 90.713% (-0.4%) from 91.116%
when pulling bff5f1c on release_ui
into 8b260e6 on main.

@liamappelbe liamappelbe marked this pull request as ready for review September 27, 2024 00:10
@liamappelbe liamappelbe changed the title WIP: [ffigen] Release all ObjC references on the main thread [ffigen] Release all ObjC references on the main thread Sep 27, 2024
Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@liamappelbe liamappelbe merged commit f9a3888 into main Sep 29, 2024
21 checks passed
@liamappelbe liamappelbe deleted the release_ui branch September 29, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ffigen package:objective_c type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ffigen] Some objects must be destroyed on the platform thread
3 participants