[windows] Fix crash when calling FlutterDesktopMessengerSetCallback after engine shutdown

[jnschulze]

Fixes flutter/flutter#118611

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the [CLA].
• All existing and new tests are passing.

[loic-sharma]

If we move forward with this approach, we should also update FlutterDesktopMessengerSendWithReply and FlutterDesktopMessengerSendResponse.

[gaaclarke]

I think there is a difference between setting the handler and sending a message. I think the OP was trying to set the handler to nullptr so that they know the proper cleanup is happening instead of relying on the engine to do so. They wanted to make that not an error. I think it would be acceptable to make that not a crash, but to keep sending messages in the destructors a crash (we can improve the crash messaging with an assert if we don't already).

[loic-sharma]

Thanks for reporting this and sending a pull request! Here are some potential alternatives we should consider before moving forward with this:

1. Improve but keep the crashes. Assert that FlutterDesktopMessengerIsAvailable before doing messenger operations. Document which messenger operations are illegal if the engine is shutdown.
2. Destroy plugins before shutting down the engine, and, make sure the engine is available during plugin shutdown.
3. Move this logic up to C++ wrapper around the desktop APIs.

Let me do some more digging to see which approach would best. /cc @gaaclarke @cbracken in case you have thoughts already here.

[loic-sharma]

@gaaclarke The engine's shutdown invalidates the messenger before plugins are shutdown:

engine/shell/platform/windows/flutter_windows_engine.cc

Lines 217 to 220 in 5932bad

	FlutterWindowsEngine::~FlutterWindowsEngine() {
	messenger_->SetEngine(nullptr);
	Stop();
	}

Could that be swapped so that plugins have access to a valid messenger during shutdown? That would prevent these kinds of issues.

Here's the call stack to shut down plugins:

app.dll!camera_windows::CameraPlugin::~CameraPlugin()
...
app.dll!flutter::PluginRegistrar::ClearPlugins()
app.dll!flutter::PluginRegistrarWindows::~PluginRegistrarWindows()
...
app.dll!flutter::PluginRegistrarManager::OnRegistrarDestroyed(FlutterDesktopPluginRegistrar * registrar)
flutter_windows.dll!flutter::FlutterWindowsEngine::Stop() 
flutter_windows.dll!flutter::FlutterWindowsEngine::~FlutterWindowsEngine()
flutter_windows.dll!flutter::FlutterWindowsEngine::~FlutterWindowsEngine()

[gaaclarke]

Could that be swapped so that plugins have access to a valid messenger during shutdown? That would prevent these kinds of issues.

We could look into it, but I don't think the end goal is worth the risk or investment to research. I think sending messages in the destructor should be an error. I guess it's debatable if we want it to be a crash or just a log message.

[jnschulze]

1. Improve but keep the crashes. Assert that FlutterDesktopMessengerIsAvailable before doing messenger operations. Document which messenger operations are illegal if the engine is shutdown.

I think the C++ client wrapper should implicitly handle this scenario. According to the documentation, "the caller is responsible for unregistering the handler if it should no longer be called". Why should there be a restriction that forbids calling it from a plugin destructor?

My use case is a video player plugin which has one event channel per player instance. Each event channel is owned by the corresponding player instance. Hence, it's not just about removing the SetStreamHandler(...) in the plugin destructor, but introducing a second destruction path for players which gets used upon plugin destruction.

3. Move this logic up to C++ wrapper around the desktop APIs.

Yes, moving the check to BinaryMessengerImpl::SetMessageHandler (and using FlutterDesktopMessengerIsAvailable) is certainly an option as well.

[loic-sharma]

1. Improve but keep the crashes. Assert that FlutterDesktopMessengerIsAvailable before doing messenger operations. Document which messenger operations are illegal if the engine is shutdown.

I think the C++ client wrapper should implicitly handle this scenario. According to the documentation, "the caller is responsible for unregistering the handler if it should no longer be called". Why should there be a restriction that forbids calling it from a plugin destructor?

In hindsight, that comment is easily misunderstood. It's supposed to mean:

Destroying the `MethodChannel` does not deregister the handler as
the lifetimes of the `MethodChannel` and its handler are fully decoupled.
The handler is destroyed when it is deregistered or when the engine is shut down.

In other words, it shouldn't be necessary to set a nullptr handler when the plugin is destroyed. We should update that comment :)

Hence, it's not just about removing the SetStreamHandler(...) in the plugin destructor, but introducing a second destruction path for players which gets used upon plugin destruction.

Interesting, could you tell us more about how players get used upon plugin destruction?

[jnschulze]

In other words, it shouldn't be necessary to set a nullptr handler when the plugin is destroyed. We should update that comment :)

I see. But what about destructing event channels in general? Should the stream handler be unset or not?

Interesting, could you tell us more about how players get used upon plugin destruction?

The left players just get destroyed (and so do their event channels).
However, I don't want to distinguish between destroying them explicitly (i.e. by invoking some "destroy" method channel method) and destroying them when the app gets shut down.

[loic-sharma]

But what about destructing event channels in general? Should the stream handler be unset or not?

For most cases, you shouldn't need to worry about deregistering handlers manually. You definitely don't need to do it during plugin destruction as the engine will take care of destroying all platform/event channel handlers.

[loic-sharma]

@gaaclarke and I chatted and our plan is:

Short-term: add asserts to the Flutter desktop API to ensure the messenger is valid before doing operations on it. Also, improve relevant comments. This will continue to be a crash, however, the developer experience should be improved.

Long-term: consider making the messenger valid during engine shut down, however, disallow sending messages. In other words, engine shutdown would have two separate phases:

1. Notify components of engine shutdown. Allow things like plugins to run shutdown logic
2. Once shutdown logic is completed, cleanup resources like the messenger

This requires investigation to see if it's a good idea.

@jnschulze I'll go ahead and close this as we'll be taking a slightly different approach. Thank you for reporting this issue and jumpstarting this discussion 😄