Repository navigation
Conversation
![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 updates the Android implementation of the shared_preferences plugin to track active plugin instances in a synchronized list, ensuring that detaching one Flutter engine does not unregister the plugin for other active engines. Feedback suggests wrapping the plugin registration and de-registration compound operations in synchronized blocks to ensure atomicity and prevent race conditions. Additionally, it is recommended to expose the attachedPlugins list as @VisibleForTesting so that unit tests can clear it to prevent memory leaks and ensure test isolation.
| attachedPlugins.add(this) | ||
| SharedPreferencesApiRegistrar().register(this) |
There was a problem hiding this comment.
To ensure thread safety and prevent race conditions when multiple engines are initialized or destroyed concurrently, the modification of attachedPlugins and the registration of the plugin should be performed atomically. Wrapping these operations in a synchronized block on attachedPlugins prevents other threads from registering an inconsistent state.
synchronized(attachedPlugins) {
attachedPlugins.add(this)
SharedPreferencesApiRegistrar().register(this)
}There was a problem hiding this comment.
Done. Attach and detach now each run under synchronized(attachedPlugins). Detach callbacks fire from FlutterEngineConnectionRegistry.remove (line 275) before FlutterJNI's main thread check runs in FlutterEngine.destroy, so I can't rely on a main thread guarantee there.
| attachedPlugins.remove(this) | ||
| SharedPreferencesApiRegistrar().register(attachedPlugins.lastOrNull()) |
There was a problem hiding this comment.
Since attachedPlugins is a synchronized list, individual operations like remove are thread-safe, but compound operations (such as remove followed by lastOrNull()) are not atomic. lastOrNull() is an extension function that checks if the list is empty and then retrieves the last element; if another thread modifies the list concurrently between these steps, it can throw an IndexOutOfBoundsException or register the wrong plugin instance. Wrapping this in a synchronized block on attachedPlugins ensures atomicity.
synchronized(attachedPlugins) {
attachedPlugins.remove(this)
SharedPreferencesApiRegistrar().register(attachedPlugins.lastOrNull())
}There was a problem hiding this comment.
Done. Attach and detach now each run under synchronized(attachedPlugins). Detach callbacks fire from FlutterEngineConnectionRegistry.remove (line 275) before FlutterJNI's main thread check runs in FlutterEngine.destroy, so I can't rely on a main thread guarantee there.
There was a problem hiding this comment.
In what context could those method run on anything but the main thread? That would violate the engine's threading contract.
There was a problem hiding this comment.
You're right, attach and detach run on the main thread. Removed the lock.
| private val attachedPlugins = | ||
| Collections.synchronizedList(mutableListOf<LegacySharedPreferencesPlugin>()) |
There was a problem hiding this comment.
The attachedPlugins list is a static companion object property, meaning it persists across the entire lifetime of the JVM process. In unit tests, plugins are attached but never detached (except for backgroundPlugin in registrationSurvivesOtherEngineDetaching), which causes plugin instances (and their mocked Context and Binding references) to leak and accumulate across tests. Consider exposing attachedPlugins as internal and @VisibleForTesting so that tests can clear it in an @After teardown block to maintain test isolation and prevent memory leaks.
| private val attachedPlugins = | |
| Collections.synchronizedList(mutableListOf<LegacySharedPreferencesPlugin>()) | |
| @VisibleForTesting | |
| internal val attachedPlugins = | |
| Collections.synchronizedList(mutableListOf<LegacySharedPreferencesPlugin>()) |
There was a problem hiding this comment.
Done. attachedPlugins is now @VisibleForTesting internal and LegacySharedPreferencesTest clears it in an @after.
…d reset test state
Plugin attach and detach run on the platform main thread: FlutterJNI.attachToNative and detachFromNativeAndReleaseResources both enforce it, so the lock guarded a case the engine contract rules out.
Doesn't this mean that if N instances are created, one of them is arbitrarily attached and the others are doing nothing? It seems like this is hacking around this issue rather than resolving the real problem. It might happen to currently work for this plugin since it is wrapping shared state, but the code still won't be doing what is intended. |
|
Agreed, this works around the registrar rather than fixing it. |
|
Keying Pigeon's JNI/FFI registrar by In the meantime, Pigeon's JNI/FFI registrar is a process-wide registry (not per-engine like Note that this PR currently only updates |
…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`.
|
I'll have a pr up that fixes this shortly by pulling in the pigeon fix. Thank you for reporting this bug and uploading a pr to fix it! |
Detaching one Flutter engine no longer removes the legacy
SharedPreferencesApiregistration that another attached engine still uses.SharedPreferencesApiInstances(Messages.g.kt:47) is a process-wide map.LegacySharedPreferencesPlugin.onDetachedFromEnginecalledSharedPreferencesApiRegistrar().register(null), which removes the default entry (Messages.g.kt:87). When a background engine (WorkManager, FCM background handler) detached, the main engine's nextSharedPreferences.getInstance()threwNo instance with no name has been registered(messages.g.dart:179).The plugin now keeps a list of attached instances. On detach it removes itself and re-registers the most recently attached one that is still attached, or unregisters if none remain.
SharedPreferencesPlugin.onDetachedFromEngineunregisters thedata_storeentry the same way (SharedPreferencesPlugin.kt:73). I left that out of this PR to keep it small and described it in the issue.Fixes flutter/flutter#194920
Test:
registrationSurvivesOtherEngineDetachinginLegacySharedPreferencesTest.ktattaches two plugins, detaches the later one, and checks the registered instance still serves the first plugin's data. It fails without the change (NoSuchElementException, nothing registered) and passes with it.Pre-Review Checklist
[shared_preferences]///).Footnotes
Version bumped to 2.5.3 (patch) with a CHANGELOG entry. ↩ ↩2