Prevent stack corruption when using C++ EventChannel

[loic-sharma]

Background

Updating the engine to Visual Studio 2019 was reverted as it broke the framework tree. The root cause is that the EventChannel's handler stores and mutates data on the EventChannel itself. This is problematic as the EventChannel's and the handler's lifetimes are decoupled (the EventChannel does not own its handler). If the EventChannel is destroyed, the handler reads/mutates deallocated memory. See this comment for more info: flutter/flutter#113135 (comment)

This change decouples the lifetimes of the EventChannel and its handler by making the handler own its own state.

Addresses flutter/flutter#113728

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

[gaaclarke]

LGTM, two nits.

[gaaclarke]

Instead of mutating a captured variable, don't you think it would be more clear to capture a reference to a bool on the heap?

[gaaclarke]

I didn't mean literally a C++ reference obviously, a std::unique_ptr<bool> just to be clear =)

[loic-sharma]

I agree mutating a captured variable is very subtle. It looks like I'll need to use std::shared_ptr per this comment:

engine/shell/platform/common/client_wrapper/include/flutter/event_channel.h

Lines 57 to 60 in d079196

	// std::function requires a copyable lambda, so convert to a shared pointer.
	// This is safe since only one copy of the shared_pointer will ever be
	// accessed.
	std::shared_ptr<StreamHandler<T>> shared_handler(handler.release());

[loic-sharma]

Updated using shared_ptr, let me know if you have additional feedback! :)

[stuartmorgan-g]

Did the mutable lands not end up working then?

[jason-simmons]

With fml::MakeCopyable there will only be one instance of the bool.

MakeCopyable wraps a single instance of the lambda in a reference-counted object.

[stuartmorgan-g]

The client wrapper code is external to the engine; it's an artifact that is copied into projects, to provide a more usable API than the C API we have to provide at the library level. So we're can't use FML, we'd have to duplicate that code into the wrapper if we wanted to use it.

[gaaclarke]

Here's a more simple and portable implementation of MakeCopyable:

#include <functional>
#include <iostream>

namespace {
  template <typename T>
  class MakeCopyable {
  public:
    MakeCopyable(T&& val) : ptr_(std::make_shared<T>(std::move(val))) {}

    template <typename... ArgType>
    auto operator()(ArgType&&... args) const {
      return ptr_->operator()(std::forward<ArgType>(args)...);
    }
   private:
    std::shared_ptr<T> ptr_;
  };
}

void RunTwice(const std::function<void()>& func) {
  func();
  func();
}

int main() {
  int32_t adder = 10;
  RunTwice(MakeCopyable([adder, tally = std::make_unique<int32_t>(0)]() {
    std::cout << adder + *tally << std::endl;
    *tally += 1;
  }));
}

[loic-sharma]

Thanks Jason for the info on fml::MakeCopyable, I wasn't aware of that!

Like Stuart mentioned, the client wrapper is code that's copied into the user's project, which the user can freely reference. I lean towards being conservative in adding new types to the client wrapper (though I realize MakeCopyable could likely be hidden as an implementation detail).

It seems folks are leaning more towards the mutable lambda now, I'll switch back to that 😄

[stuartmorgan-g]

though I realize MakeCopyable could likely be hidden as an implementation detail

It's non-trivial to hide most things in the wrapper since it's heavily templated.

[yaakovschectman]

LGTM

[cbracken]

Thanks for fixing!