Ensure that application termination callbacks are serviced on the runner thread. (#5247)

This removes the Application::Delegate interface and instead uses a closure that captures the task runner. The interface was expected to be more complicated than it turned out to be.

content_handler/application.cc

@@ -20,7 +20,7 @@ namespace flutter {
 
 std::pair<std::unique_ptr<fsl::Thread>, std::unique_ptr<Application>>
 Application::Create(
-    Application::Delegate& delegate,
+    TerminationCallback termination_callback,
     component::ApplicationPackage package,
     component::ApplicationStartupInfo startup_info,
     fidl::InterfaceRequest<component::ApplicationController> controller) {
@@ -29,7 +29,7 @@ Application::Create(
 
   fxl::AutoResetWaitableEvent latch;
   thread->TaskRunner()->PostTask([&]() mutable {
-    application.reset(new Application(delegate,                 //
+    application.reset(new Application(termination_callback,     //
                                       std::move(package),       //
                                       std::move(startup_info),  //
                                       std::move(controller)     //
@@ -51,12 +51,12 @@ static std::string DebugLabelForURL(const std::string& url) {
 }
 
 Application::Application(
-    Application::Delegate& delegate,
+    TerminationCallback termination_callback,
     component::ApplicationPackage,
     component::ApplicationStartupInfo startup_info,
     fidl::InterfaceRequest<component::ApplicationController>
         application_controller_request)
-    : delegate_(delegate),
+    : termination_callback_(termination_callback),
       debug_label_(DebugLabelForURL(startup_info.launch_info.url)),
       application_controller_(this) {
   application_controller_.set_error_handler([this]() { Kill(); });
@@ -234,7 +234,7 @@ void Application::Kill() {
   }
   wait_callbacks_.clear();
 
-  delegate_.OnApplicationTerminate(this);
+  termination_callback_(this);
   // WARNING: Don't do anything past this point as this instance may have been
   // collected.
 }

content_handler/application.h

@@ -31,16 +31,13 @@ class Application final : public Engine::Delegate,
                           public component::ApplicationController,
                           public views_v1::ViewProvider {
  public:
-  class Delegate {
-   public:
-    virtual void OnApplicationTerminate(const Application* application) = 0;
-  };
+  using TerminationCallback = std::function<void(const Application*)>;
 
   // Creates a dedicated thread to run the application and constructions the
   // application on it. The application can be accessed only on this thread.
   // This is a synchronous operation.
   static std::pair<std::unique_ptr<fsl::Thread>, std::unique_ptr<Application>>
-  Create(Application::Delegate& delegate,
+  Create(TerminationCallback termination_callback,
          component::ApplicationPackage package,
          component::ApplicationStartupInfo startup_info,
          fidl::InterfaceRequest<component::ApplicationController> controller);
@@ -53,7 +50,7 @@ class Application final : public Engine::Delegate,
 
  private:
   blink::Settings settings_;
-  Delegate& delegate_;
+  TerminationCallback termination_callback_;
   const std::string debug_label_;
   UniqueFDIONS fdio_ns_ = UniqueFDIONSCreate();
   fxl::UniqueFD application_directory_;
@@ -69,7 +66,7 @@ class Application final : public Engine::Delegate,
   std::pair<bool, uint32_t> last_return_code_;
 
   Application(
-      Application::Delegate& delegate,
+      TerminationCallback termination_callback,
       component::ApplicationPackage package,
       component::ApplicationStartupInfo startup_info,
       fidl::InterfaceRequest<component::ApplicationController> controller);

content_handler/application_runner.cc

@@ -65,8 +65,24 @@ void ApplicationRunner::StartApplication(
     component::ApplicationPackage package,
     component::ApplicationStartupInfo startup_info,
     fidl::InterfaceRequest<component::ApplicationController> controller) {
+  // Notes on application termination: Application typically terminate on the
+  // thread on which they were created. This usually means the thread was
+  // specifically created to host the application. But we want to ensure that
+  // access to the active applications collection is made on the same thread. So
+  // we capture the runner in the termination callback. There is no risk of
+  // there being multiple application runner instance in the process at the same
+  // time. So it is safe to use the raw pointer.
+  Application::TerminationCallback termination_callback =
+      [task_runner = fsl::MessageLoop::GetCurrent()->task_runner(),  //
+       application_runner = this                                     //
+  ](const Application* application) {
+        task_runner->PostTask([application_runner, application]() {
+          application_runner->OnApplicationTerminate(application);
+        });
+      };
+
   auto thread_application_pair =
-      Application::Create(*this,                    // delegate
+      Application::Create(termination_callback,     // termination callback
                           std::move(package),       // application pacakge
                           std::move(startup_info),  // startup info
                           std::move(controller)     // controller request

content_handler/application_runner.h

@@ -20,8 +20,7 @@ namespace flutter {
 
 // Publishes the |component::ApplicationRunner| service and runs applications on
 // their own threads.
-class ApplicationRunner final : public Application::Delegate,
-                                public component::ApplicationRunner {
+class ApplicationRunner final : public component::ApplicationRunner {
  public:
   ApplicationRunner();
 
@@ -64,8 +63,7 @@ class ApplicationRunner final : public Application::Delegate,
 
   void UnregisterApplication(const Application* application);
 
-  // |Application::Delegate|
-  void OnApplicationTerminate(const Application* application) override;
+  void OnApplicationTerminate(const Application* application);
 
   void SetupICU();