Was this easy to debug...

This was hard to track, it took me >2 days to find the issue. Debugging was harder since this only affected optimized builds (values are optimized away, debugger skips large code paths, etc...). The crashes happened well after the stack was corrupted, making it difficult to track the source. Also, adding std::couts would fix the issue (likely because stack was changed in such a way to no longer crash).

I wonder if there's a way to prevent this from happening.

Agreed. I added this bit to the PR description: In the future, we should reconsider this API to prevent lifetime issues. Perhaps destroying MethodChannels/EventChannels should deregister the handlers. Thoughts @cbracken or @stuartmorgan?

I'd like to hold off on landing this until we have established whether engine changes are needed, because this isn't supposed to be necessary. As pointed out above, lots of actual plugins use this structure, so if there's an engine-level problem this is a band-aid that masks it, rather than a fix.

Once the FlutterWindow::OnCreate method exits, the MethodChannel and EventChannel are destroyed.

This is expected; the behavior here was modeled on the existing iOS behavior (I forget whether Android behaves the same way) where the channel serves as a convenience to set up the handler, but doesn't own it. (I think that's a slightly odd structure, but I explicitly wanted to minimize unnecessary divergence between platform APIs for ease of understanding by clients working on plugins across platforms.)

However, the desktop embedder keeps pointers to these stack-allocated objects' handlers. These handlers would then corrupt the stack if they attempted to modify the deallocated channel.

I had a reply mostly written up confused about this because I was looking at the message/method channel code since I'm more familiar with that, and the PR description suggests this is a problem with both, but now I see the problem with event channels. I'm not sure what the problem with message/method channels is though; are you sure you saw corruption from those? If so, I'm still missing a piece of this.

The context here is that I wrote the message/method channel code to match iOS, and the design is explicitly that the handlers are not in any conceptual way owned by the channel objects; they shouldn't ever have references to them (so modifying them later shouldn't be possible). The channel code was a community contribution quite a while later, and I apparently didn't have enough of that context paged back in when I reviewed it. I was comparing the PR to iOS, apparently without thinking through the original constraint of not having self-referencing handlers (which doesn't apply to iOS due to ref counting), so missed the problematic this usage.

Can we fix this in the engine by just replacing is_listening_ with a stateful mutable lambda variable? We're using a new enough version of C++ now (which might not have been true at the time? I forget), but I'm not sure without trying it if there's something else about how we're using the lambda that prevents that.

Perhaps destroying MethodChannels/EventChannels should deregister the handlers.

We should avoid this if at all possible. It would make the Window API confusingly inconsistent with other platforms, and would silently break a ton of plugins.

Thanks for the background Stuart, that's very helpful! I agree that structure is odd, but I also agree we should minimize unnecessary divergence.

Given the EventChannel structure, I'm worried that years from now we'll forget about these constraints again and reintroduce a similar bug. Is there a clean way to enforce that handlers don't reference the channel? Perhaps a test that fails if EventChannel or MethodChannel had such a bug? I'll experiment making a test that fails if stack corruption is detected. In any case, I'll add comments to MethodChannel::SetMethodCallHandler and EventChannel::SetStreamHandler to call out these constraints.

I'm not sure what the problem with message/method channels is though; are you sure you saw corruption from those?

The crash only happens with EventChannels today. This change included both EventChannels and MethodChannels as their implementations are very similar, the mistake in EventChannel could easily be made in MethodChannel as well.

Can we fix this in the engine by just replacing is_listening_ with a stateful mutable lambda variable?

Interesting idea, this would fix the ecosystem without needing any changes. I'll close this off and try that instead!

I agree that structure is odd, but I also agree we should minimize unnecessary divergence.

I remembered a potential problem with auto-unregistration, which may be why iOS channels were designed that way: registration for a channel name in the engine is last-wins, so if for some reason you ever had two channel objects for the same channel name, you'd get surprising results. E.g., create A, create B, destroy A would result in B's handler being unregistered.

(If I were designing these APIs from scratch I would probably make them lifetime-bound, and use an identifier for registration/unregisterion to avoid that problem, but I don't think the oddities here are with the churn of introducing a replacement API surface across all platforms.)