Automatic plugin registration breaks some cases

[mklim]

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

1. Run flutter create -t app with flutter stable (v.12.13)
2. Include any plugin using the v1 embedding that doesn't require an activity (for example, firebase_messaging)
3. flutter run on 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

1. Run flutter create -t app with flutter stable (v1.12.13)
2. Remove the template MainActivity in favor of FlutterActivity as recommended by our official migration docs.
3. Include any plugin using the v1 embedding that requires an activity (for example, google_maps_flutter).
4. flutter run on 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

[mklim]

Talked about this a little bit offline with @matthew-carroll.

• The ActivityAware/ServiceAware approach for a fix won't work because right now the ServiceAware callbacks aren't consistently called in every background case.
• The main reason isn't boilerplate, but a separation of concerns. We're trying to avoid another case where we want to migrate code that's been generated into a user's app. I agree that this is important enough to try to work around instead of just revert.
• I thought of a third idea that I think is viable. 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.

[matthew-carroll]

Thanks @mklim for capturing that info. I'll take a look at your 3rd proposal as soon as I get a minute. I'm juggling a few things, but I know this is a high priority. At first glance, I'm not too bothered with needing to call super() given that we're dealing with Android, and given the downsides of alternatives. I'll look deeper at it as soon as I can.

[matthew-carroll]

The possibility of duplicate plugin registration within FlutterEngine has now been fixed here: flutter/engine#15956

That's one half of the resolution to this ticket.

[mklim]

flutter/engine#15979 is currently waiting for CI and should fix the rest of this once it lands.

Unfortunately these fixes are in the engine, so there's still going to be a delay after they land before they're widely rolled out and usable.

[mklim]

The patches to fix this have landed in flutter/engine, but I'm going to keep the issue open for now since they still haven't rolled into the framework.

[mikemaksymowych]

It seems like after flutter/engine#15979, which rolled into flutter here I can no longer initialize plugins prior to runApp()?

[mklim]

So my earlier assessment was wrong, sorry about that. I could "reproduce" the problem because I hadn't fully modified the camera example and v2 registration was broken in general. Right now it's using the v1 embedding GeneratedPluginRegistrant and a custom v1 path in its MainActivity. Updating it to fully use the v2 GeneratedPluginRegistrant and rely on the v2 FlutterActivity at the same time fixes the problem I was seeing. (#47153 has more context on why the plugin example is out of date.) @mikemaksymowych would you be able to share a small reproducible sample case that causes this exception for you?

[mikemaksymowych]

I think my issue is a missing call to super.configureFlutterEngine(flutterEngine) in my overrode method?

I'm generally confused about plugin registration now. A new flutter app template shows the following:

  @Override
  public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) {
    GeneratedPluginRegistrant.registerWith(flutterEngine);
  }

I had removed the call to registerWith at some point because I was seeing duplicate registration on plugins, and all was well prior to flutter/engine#15979. So my plugins were registered correctly without the call to super (essentially an empty configureFlutterEngine)

After the change, it seems like I need the call to super (or just remove my empty override), or I can restore the call to registerWith, and my problem is solved (double registration also appears to be fixed).

[mklim]

Got it, thanks for clarifying the situation. So the confusing thing is our official instructions say not to override that method, but our template generates an override (#49346). Up until the fixes for this issue landed both configurations were buggy in opposing ways. It sounds like you manually fixed the duplicate registration problem in your app, but then our fix removed the "extra" registration call you were relying on so your app ended up never registering at all.

As of 4fbb85e, having an app with either the flutter template MainActivity (specifically the configureFlutterEngine manual registration override and no super()) or not having a configureFlutterEngine override or even a subclassed MainActivity at all should both function without bugs. However out of the two, we recommend not overriding MainActivity and relying on the base FlutterActivity's configureFlutterEngine call instead to do the plugin registration. We still need to update all the docs to recommend this though since the patch hasn't rolled to stable.

The reason either works now is that the one and only one registration call is in FlutterActivity#configureFlutterEngine. This runs after an engine has been constructed and attached to a Flutter activity. For the template, it manually overrides the class and registers the plugins in that method without calling super(), and that ends up having the same effect as the method in the base class.

Before the fix there was a bit more going on:

• Plugins were unintentionally being registered in two different places in the Android engine embedding.
• The first registration call happened as soon as FlutterEngine.java was instantiated. This class does a lot, but basically you can think of it as spinning up the rendering engine for Flutter (among other things). This class is instantiated and then attached to an actual View in the Flutter tree, so this initial plugin registration call didn't include an Activity with it.
• Then the activity would be created. The MainActivity generated by flutter create had a manual registration call in configureFlutterEngine, which would register the plugins for a second time and include the activity when it did so.
• Some plugins would break from duplicate calls, while others would error internally from receiving a first (and sometimes only) registration call without a Flutter activity attached.

[mikemaksymowych]

Thanks, that clears up a lot :)

[mklim]

The fix for this has rolled into Flutter v1.14.6 (currently contained in the beta, dev, and master channels). Unfortunately it doesn't look like we'll be able to hotfix it so I'm going to close this since there isn't much more action to take here in Flutter itself except wait for the fix to roll everywhere. Plugins that are individually impacted may be able to partially work around this in the meantime by safeguarding against duplicate registration.

[kbrmimbyl]

@mklim, we discovered this bug in 1.12.13+hotfix.7 just before going to production on our app. I'm considering migrating to Flutter 1.14.6 on beta... where I can get the release notes for 1.14.6? I'd like to know if it's safe to migrate...

Here on github I just see one string "Revert string interp (#49602)" here: https://github.com/flutter/flutter/releases/tag/v1.14.6

[droidluv]

@mklim thanks for all the work you put into this, was pulling my hair out on what to do here, because a couple of plugins were failing because of a NULL activity and some like the firebase messaging was having two registered states, and was calling onMessage twice, switching to 1.14.6 beta resolved all the issues I was facing with different weirdly behaving plugins.

As an extra question please do help me understand this, one of the steps state that, I have to,
"Remove the reference to FlutterApplication from the application tag" but in FirebaseMessaging for background message one of the steps is extending the FlutterApplication class and replacing the reference with our custom class

The step is mentioned right after step 4 https://pub.dev/packages/firebase_messaging

What would you suggest here? Can we still extend the FlutterApplication class or is there a new best practice?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.