[Video player plugin] crashes with new pigeon version when engine is detached

The latest video player tries to set the message handler to nil on all the channels in the messages.m file when the FlutterEngine gets deallocated. The following code from the video player doe sthat:

- (void)detachFromEngineForRegistrar:(NSObject<FlutterPluginRegistrar>*)registrar {
  for (NSNumber* textureId in _players.allKeys) {
    FLTVideoPlayer* player = _players[textureId];
    [player disposeSansEventChannel];
  }
  [_players removeAllObjects];
  FLTVideoPlayerApiSetup(registrar.messenger, nil);
}

However, by this time, the engine is not running, and this causes an exception:

2020-05-12 08:51:58.023258-0400[10094:37996] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Setting a message handler before the FlutterEngine has been run.'
*** First throw call stack:
(
	0   CoreFoundation                      0x000000010dd416fb __exceptionPreprocess + 331
	1   libobjc.A.dylib                     0x000000010bf7bac5 objc_exception_throw + 48
	2   CoreFoundation                      0x000000010dd41482 +[NSException raise:format:arguments:] + 98
	3   Foundation                          0x000000010b9c9927 -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 194
	4   Flutter                             0x0000000110cdbb31 -[FlutterEngine setMessageHandlerOnChannel:binaryMessageHandler:] + 264
	5   MyApp                               0x000000010560b23a _Z22FLTVideoPlayerApiSetupPU33objcproto22FlutterBinaryMessenger11objc_objectPU28objcproto17FLTVideoPlayerApi11objc_object + 362
	6   MyApp                               0x00000001056dbd9e -[FLTVideoPlayerPlugin detachFromEngineForRegistrar:] + 766
	7   CoreFoundation                      0x000000010ddb19c2 -[__NSDictionaryM enumerateKeysAndObjectsWithOptions:usingBlock:] + 226
	8   Flutter                             0x0000000110cd8e56 -[FlutterEngine dealloc] + 92

[✓] 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)

@dannyvalentesonos

Hi @dannyvalentesonos
Pleas explain in detail, do you think this is a major issue? Can this reproduce when using code sample?
Thank you

I 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 the detachFromEngineForRegistrar on 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 detachFromEngineForRegistrar on all plugins when the "destroy" method is called, such as here: flutterEngine.destroy();

So, maybe on iOS, the engine should call detachFromEngineForRegistrar from all plugins at the time of destroyContext instead 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 detachFromEngineForRegistrar call 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's setup method doesn't actually check for whether api is null or not. So that isn't great either.

@gaaclarke

cc @gaaclarke as the pigeon documentation said to do so.

@dannyvalentesonos

Thanks 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

@gaaclarke Thanks for responding.
The reason we call destroyContext is 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

@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.

added a test to reproduce the crash from flutter/flutter#56996

@dannyvalentesonos

@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 -v and a minimal reproduction of the issue.

Navigation Menu

Repository navigation

[Video player plugin] crashes with new pigeon version when engine is detached #56996

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[Video player plugin] crashes with new pigeon version when engine is detached #56996

Description

Activity

dannyvalentesonos commented on May 12, 2020

TahaTesser commented on May 12, 2020

dannyvalentesonos commented on May 12, 2020

dannyvalentesonos commented on May 13, 2020

gaaclarke commented on May 13, 2020

dannyvalentesonos commented on May 13, 2020

dannyvalentesonos commented on May 13, 2020

gaaclarke commented on May 13, 2020

gaaclarke commented on May 13, 2020

github-actions commented on Aug 21, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions