-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Description
Steps to Reproduce
There are two distinct but related breakages here: double registration and too early registration when automatic registration is relied on. I'm going to describe them both below.
Double registration
Reproduction Steps
- Run
flutter create -t appwith flutter stable (v.12.13) - Include any plugin using the v1 embedding that doesn't require an activity (for example,
firebase_messaging) flutter runon an Android device.
Expected results: Plugin should be registered and function normally.
Actual results: The plugin is registered twice (visible with a debugger), leading to plugin-specific bugs caused by multiple instances of the same plugin class responding to single MethodChannel calls. For example, see firebase/flutterfire#1669: a particular plugin method is always called twice.
Early registration
Reproduction Steps
- Run
flutter create -t appwith flutter stable (v1.12.13) - Remove the template
MainActivityin favor ofFlutterActivityas recommended by our official migration docs. - Include any plugin using the v1 embedding that requires an activity (for example,
google_maps_flutter). flutter runon an Android device.
Expected results: Plugin should be registered and function normally.
Actual results: The plugin is registered before the activity is set on the shim registrar (visible with a debugger), leading to plugin-specific bugs caused by the null activity. For example, see #47921: the plugin appears to never be registered and is completely unusable.
Background
Automatic plugin registration was proposed in this doc and merged in flutter/engine#13455. The goal stated in the doc is to remove boilerplate code in user apps, but I think it may also have been to support background registration. (@bkonyi @matthew-carroll to confirm)
The v1 registration flow expected plugins to be registered once, and the registration call to contain any information the plugin may need (like an Activity). The v2 registration flow in contrast is done over a series of lifecycle callbacks and never propagates all the necessary information for a plugin at a single call. The v2 registration flow is supposed to still be compatible with v1-only plugins through a ShimPluginRegistry, which internally listens to all the v2 callbacks.
Status
Edited 1/27/2020: This has been fixed in the framework as of 4fbb85e, but still hasn't rolled to any channels or versions other than master.
Suggested fix
We disable the current automatic registration call in FlutterEngine when it's constructed via FlutterActivity and add an automatic registration call into the configureFlutterEngine of the FlutterActivity superclass. That way it will get called at the appropriate time in both the foreground and background case automatically and will still be under Flutter source code instead of user code. As a bonus the generated template doesn't call super() so this should automatically fix the problem in existing user apps without the need for a migration. The "ugly" part here is that from now on we're going to expect users to call super() when they override configureFlutterEngine if they're not registering plugins themselves, and it's been a no-op before. So people who have been deliberately overriding it will be impacted. I'm unaware of any such cases today.
Edit: The purpose is to avoid having critical code in a user template, not just removing boilerplate. This is a stronger case to avoid reverting. If the automatic registration was only to remove boilerplate code, I suggest we revert that change and change all our official docs to recommend registering plugins in a subclass of FlutterActivity, like the template. Knowing the "right" place to correctly automatically register v1 plugins with the v2 embedding so that they have all their required state is difficult since the embedding can be completely decoupled from any kind of activity or service at all. The boilerplate is regrettable but preferable to bugs.
Edit: This won't work for all background cases, figured out in an offline discussion. Assuming the automatic registration is necessary to support background cases better, @amirh suggested a plan where at a high level we can basically avoid this problem by still automatically registering eventually, but delaying the automatic registration until the plugin registry is either attached to an activity (for foreground cases) or a service (for background cases). Then we should change the tool's template and urge users to migrate away from the currently MainActivity registration call.
Related Issues
- "Upgrading pre 1.12" Android instructions causes Android crashes as written #47921
- Inconsistency in the documentation about MainActivity content for v2 embedding enabled #49346
- [firebase_messaging] onMessage/onResume is called twice on Android/iOS firebase/flutterfire#1669
/cc @amirh @bkonyi @matthew-carroll @xster @blasten @workerbee22 @woprandi