Repository navigation
[Video player plugin] crashes with new pigeon version when engine is detached #56996
Description
Activity
[✓] Flutter (Channel dev, 1.19.0-0.0.pre, on Mac OS X 10.14.6 18G4032, locale en-US)
• Flutter version 1.19.0-0.0.pre at /Users/danny.valente/Library/flutter/1.19.0-0.0.pre-dev
• Framework revision a849daf (4 days ago), 2020-05-07 18:59:02 -0700
• Engine revision 3953c3c
• Dart version 2.9.0 (build 2.9.0-5.0.dev 4da5b40fb6)Hi @dannyvalentesonos
Pleas explain in detail, do you think this is a major issue? Can this reproduce when using code sample?
Thank youI have made a local copy and modified the plugin, so not major for me, but this has been published and it crashes, so it's not great. Though I'm not sure how many people actually destroy a Flutter Engine at runtime, so it might not be a problem for many.
So, the issue is that when a Flutter Engine is destroyed, on iOS we call for example
[self.flutterEngine destroyContext];
Then, once the engine actually gets deallocated, it calls thedetachFromEngineForRegistraron all plugins. The video plugin tries to set message handlers to nil on its channels, but the flutter engine is dead at this point and causes a crash. The previous version of the video player plugin didn't try to set the message handlers to nil, so this was never a problem.I just debugged on Android, and Android's Flutter engine actually calls
detachFromEngineForRegistraron all plugins when the "destroy" method is called, such as here:flutterEngine.destroy();So, maybe on iOS, the engine should call
detachFromEngineForRegistrarfrom all plugins at the time ofdestroyContextinstead of destruction.Also, on android, the new version of the video plugin has the following code...
/** Sets up an instance of `VideoPlayerApi` to handle messages through the `binaryMessenger` */ public static void setup(BinaryMessenger binaryMessenger, VideoPlayerApi api) { { BasicMessageChannel<Object> channel = new BasicMessageChannel<Object>(binaryMessenger, "dev.flutter.pigeon.VideoPlayerApi.initialize", new StandardMessageCodec()); channel.setMessageHandler(new BasicMessageChannel.MessageHandler<Object>() { public void onMessage(Object message, BasicMessageChannel.Reply<Object> reply) { HashMap<String, HashMap> wrapped = new HashMap<String, HashMap>(); try { api.initialize(); wrapped.put("result", null); } catch (Exception exception) { wrapped.put("error", wrapError(exception)); } reply.reply(wrapped); } }); }And the video player plugin's
detachFromEngineForRegistrarcall looks like:@Override public void onDetachedFromEngine(FlutterPluginBinding binding) { if (flutterState == null) { Log.wtf(TAG, "Detached from the engine before registering to it."); } flutterState.stopListening(binding.getBinaryMessenger()); flutterState = null; }which calls
stopListening()below.void stopListening(BinaryMessenger messenger) { VideoPlayerApi.setup(messenger, null); }Notice in
VideoPlayerApi'ssetupmethod doesn't actually check for whetherapiis null or not. So that isn't great either.cc @gaaclarke as the pigeon documentation said to do so.
Reacted by gaaclarkeThanks for the report @dannyvalentesonos.
So, maybe on iOS, the engine should call detachFromEngineForRegistrar from all plugins at the time of destroyContext instead of destruction.
I think that's a good idea. I'm going to try to fix this without changing the engine though first since I want pigeon to be compatible with old versions of the engine.
Why do you call
destroyContext? Was this a workaround to a previous bug in the engine or is it to get more deterministic memory management to avoid OOM?@gaaclarke Thanks for responding.
The reason we calldestroyContextis actually because we're building an Add2App experience, so when we run our automation tests, we need to attach flutter driver to our process after the app has already launched, and so we have a message we send to our app that says to enable automation, which causes us to destroy the current Flutter engine so that we can create a new one with a different dart entry point from our main.dart file that enables flutter driver integration.@gaaclarke also, have you seen the issue I brought up for Android as well?
basically, pigeon generates this:
/** Sets up an instance of `VideoPlayerApi` to handle messages through the `binaryMessenger` */ public static void setup(BinaryMessenger binaryMessenger, VideoPlayerApi api) { { BasicMessageChannel<Object> channel = new BasicMessageChannel<Object>(binaryMessenger, "dev.flutter.pigeon.VideoPlayerApi.initialize", new StandardMessageCodec()); channel.setMessageHandler(new BasicMessageChannel.MessageHandler<Object>() { public void onMessage(Object message, BasicMessageChannel.Reply<Object> reply) { HashMap<String, HashMap> wrapped = new HashMap<String, HashMap>(); try { api.initialize(); wrapped.put("result", null); } catch (Exception exception) { wrapped.put("error", wrapError(exception)); } reply.reply(wrapped); } }); }However, if api is null, it still ends up subscribing a message handler instead of nulling it out.
@dannyvalentesonos Yep, added an issue for it, fixing it right now. I don't think I'll be able to get detaching from a running engine working without making changes to the engine code. Since it didn't work previously I'm not really taking functionality away but we should have it fixed by next stable release hopefully. This wasn't your problem anyway and no one has complained about it yet.
v0.10.11 should have the fix. Unfortunately I don't think any of the first-party plugins test plugins the way you use them, detaching from an engine was something that was added later in flutter so there is some bias in the testing. I started working on an automated test for pigeon, I'm not sure if we should do something similar for plugins. Let me know if you run into anything else.
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 -vand a minimal reproduction of the issue.- locked as resolved and limited conversation to collaborators
on Aug 21, 2021
The latest video player tries to set the message handler to nil on all the channels in the
messages.mfile when the FlutterEngine gets deallocated. The following code from the video player doe sthat:However, by this time, the engine is not running, and this causes an exception: