Repository navigation
[beta] Expose engineId on native plugin bindings and registrars on Android, iOS, and macOS - #195991
Conversation
…iOS, and macOS (flutter#195881) Exposes the engine identifier (corresponding to `PlatformDispatcher.instance.engineId` in Dart) directly on native plugin bindings and registrars across Android, iOS, and macOS so that FFI/JNI plugins can associate and clean up per-engine native state during plugin registration and detachment without access to the underlying engine instance: - **Android**: Adds `FlutterPluginBinding.getEngineId()` returning `flutterEngine.getEngineId()`. - **iOS**: Adds `@property(readonly) int64_t engineIdentifier` to `FlutterBaseRegistrar` (inherited by `FlutterPluginRegistrar` and `FlutterApplicationRegistrar`), cached at registrar initialization so it remains valid during `detachFromEngineForRegistrar:` when the weak engine reference is already `nil`. - **macOS**: Adds `@property(readonly) int64_t engineIdentifier` to `FlutterPluginRegistrar`, cached at registrar initialization. work towards flutter#195876 (linux and windows will follow in another pr) ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/CONTRIBUTING.ai.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
@tarrinneal please fill out the PR description above, afterwards the release team will review this request. |
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
github-actions
Bot
added
platform-android
tarrinneal
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request exposes the unique identifier of the FlutterEngine (engineId or engineIdentifier) to plugin registrars and bindings across Android, iOS, and macOS platforms, and adds corresponding unit tests. The review feedback points out that adding engineIdentifier as a required property to the public FlutterBaseRegistrar (iOS) and FlutterPluginRegistrar (macOS) protocols is a breaking change for third-party implementations. It is recommended to mark these properties as @optional to maintain backward compatibility.
|
|
||
| /** | ||
| * The unique identifier for the `FlutterEngine` associated with this registrar. | ||
| * | ||
| * This corresponds to `PlatformDispatcher.instance.engineId` in Dart. | ||
| */ | ||
| @property(nonatomic, readonly) int64_t engineIdentifier; |
There was a problem hiding this comment.
Adding a new required property to the public FlutterBaseRegistrar protocol is a breaking change for any third-party plugins or applications that implement custom or mock registrars in their unit tests. To maintain backward compatibility, please mark this property as @optional and restore the @required directive for the subsequent properties/methods.
@optional
/**
* The unique identifier for the FlutterEngine associated with this registrar.
*
* This corresponds to PlatformDispatcher.instance.engineId in Dart.
*/
@property(nonatomic, readonly) int64_t engineIdentifier;
@requiredReferences
- Check for potential regressions: Look for changes that might break existing functionality or introduce unexpected behavior in related areas. (link)
There was a problem hiding this comment.
There was a problem hiding this comment.
The possible difference is that this is a minor release cherry pick though.
There was a problem hiding this comment.
Maybe make it optional in the cherry pick and keep required in master?
|
|
||
| /** | ||
| * The unique identifier for the `FlutterEngine` associated with this registrar. | ||
| * | ||
| * This corresponds to `PlatformDispatcher.instance.engineId` in Dart. | ||
| */ | ||
| @property(nonatomic, readonly) int64_t engineIdentifier; |
There was a problem hiding this comment.
Adding a new required property to the public FlutterPluginRegistrar protocol is a breaking change for any third-party plugins or applications that implement custom or mock registrars in their unit tests. To maintain backward compatibility, please mark this property as @optional and restore the @required directive for the subsequent properties/methods.
@optional
/**
* The unique identifier for the FlutterEngine associated with this registrar.
*
* This corresponds to PlatformDispatcher.instance.engineId in Dart.
*/
@property(nonatomic, readonly) int64_t engineIdentifier;
@requiredReferences
- Check for potential regressions: Look for changes that might break existing functionality or introduce unexpected behavior in related areas. (link)
zanderso
left a comment
zanderso
left a comment
There was a problem hiding this comment.
This CP is under discussion today.
|
I'm going to go ahead and close this. A breaking change to support feature work is not suitable for a cherry-pick: https://github.com/flutter/flutter/blob/master/docs/releases/Flutter-Cherrypick-Process.md A breaking change should also follow our breaking change processes: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes In terms of making this breaking on master and non-breaking on stable by making the new property |
|
FWIW I can't remember any case where additions to the plugin API surface were considered a breaking change before. There's no plausible use case for non-test code to implement these surfaces, and things that only break the tests of plugin developers (and of that very small subset of people, only those that have native unit tests, which anecdotally is a significantly smaller subset, and of those only those who unit test the parts that require mocking this surface) have an extremely small impact since there's no ecosystem-level effect (unlike changes that break package code that is consumed by clients). In the past we just adapted to the changes in Flutter's own rollers, and didn't consider it formally breaking. |
|
Thanks @stuartmorgan, I apologize that it wasn't clear to me from the discussion that the change would only affect rarely-written unit tests and not production code. A quick decision was needed and in the presence of tests broken internally, we made a conservative choice. If this change is safer than our coarse-grained signals indicated, then we should consider it for a hotfix release in this cycle. cc @tarrinneal |
…droid, iOS, and macOS (#196145) ### Issue Link: #195876 ### Impact Description: Native plugins using JNI/FFI (such as plugins generated with Pigeon's native interop support) register directly in native memory rather than on a per-engine `BinaryMessenger`, and have no non-deprecated public API on `FlutterPluginBinding` (Android) or `FlutterPluginRegistrar` (iOS/macOS) to obtain the engine identifier during plugin attachment and detachment. In apps running multiple `FlutterEngine` instances in the same process (such as background engines alongside the main UI engine), one engine attaching or detaching can overwrite or unregister another engine's JNI/FFI host API instance (#194920). This blocks shipping multi-engine-safe FFI/JNI plugins (flutter/packages#13359) against the 3.50 stable release. **Scope of the API break:** This supersedes #195991, which was closed as a breaking change. As [@stuartmorgan noted](#195991 (comment)), the `@required` `engineIdentifier` property on `FlutterBaseRegistrar` (iOS) and `FlutterPluginRegistrar` (macOS) only affects hand-written native unit-test fakes that conform to those protocols directly. Production plugin and app code only consumes registrars provided by the engine and is unaffected. Following that, [@zanderso commented](#195991 (comment)) that if the change is safer than the earlier signals indicated, it should be considered for a hotfix release in this cycle. Affected test fakes can add `var engineIdentifier: Int64 = 0` (Swift) or `@synthesize engineIdentifier;` (Objective-C), which compiles both before and after this change. First-party fakes were already updated in flutter/packages#13574. On Android, `FlutterPluginBinding` is a concrete class, so adding `getEngineId()` does not affect mocks. ### Changelog Description: [flutter/195876] When running multiple `FlutterEngine`s on Android, iOS, or macOS, FFI/JNI plugins could not determine which engine a `FlutterPluginBinding` or `FlutterPluginRegistrar` belonged to. ### Workaround: On Android, plugins can call the deprecated `FlutterPluginBinding.getFlutterEngine().getEngineId()`. On iOS and macOS, there is no public API workaround (`engineIdentifier` is only declared in internal headers, and the registrar's weak engine reference is already `nil` during `detachFromEngineForRegistrar:`). ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: 1. On Android, verify `FlutterPluginBinding.getEngineId()` returns the associated `FlutterEngine.getEngineId()`. 2. On iOS, verify `registrar.engineIdentifier` on `FlutterPluginRegistrar` and `FlutterApplicationRegistrar` matches the owning `FlutterEngine.engineIdentifier`, both during registration and during `detachFromEngineForRegistrar:`. 3. On macOS, verify `registrar.engineIdentifier` on `FlutterPluginRegistrar` matches the owning `FlutterEngine.engineIdentifier`.
This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.
Issue Link:
#195876
Impact Description:
Native plugins using JNI/FFI (such as plugins generated with Pigeon's native interop support) register directly in native memory rather than on a per-engine
BinaryMessenger, and previously had no non-deprecated public API onFlutterPluginBinding(Android) orFlutterPluginRegistrar(iOS/macOS) to obtain the engine identifier during plugin attachment and detachment. In production apps running multipleFlutterEngineinstances in the same process (such as background engines alongside the main UI engine), one engine attaching or detaching can overwrite or unregister another engine's JNI/FFI host API instance (#194920). ExposingengineId/engineIdentifieron the native plugin bindings and registrars unblocks per-engine scoping of native FFI/JNI plugin registrations.Changelog Description:
flutter/195876 Exposes the engine identifier on
FlutterPluginBinding(Android) andFlutterPluginRegistrar(iOS and macOS) so native FFI/JNI plugins can scope per-engine registrations in multi-engine apps.Workaround:
On Android, plugins had to call the deprecated
FlutterPluginBinding.getFlutterEngine().getEngineId(). On iOS and macOS, there was no public API workaround onFlutterPluginRegistrar(engineIdentifierwas only declared on internal headers, and the weak engine reference on the registrar is alreadynilduringdetachFromEngineForRegistrar:).Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
FlutterPluginBinding.getEngineId()returns the associatedFlutterEngine.getEngineId().registrar.engineIdentifieronFlutterPluginRegistrarandFlutterApplicationRegistrarmatches the owningFlutterEngine.engineIdentifierboth during registration and duringdetachFromEngineForRegistrar:after the engine is deallocated.registrar.engineIdentifieronFlutterPluginRegistrarmatches the owningFlutterEngine.engineIdentifier.