From 06afdfe54ebef9168a90ca00a6721c2d36e6aafa Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Fri, 11 May 2018 20:11:02 -0700 Subject: [PATCH] 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 | 10 +++++----- content_handler/application.h | 11 ++++------- content_handler/application_runner.cc | 18 +++++++++++++++++- content_handler/application_runner.h | 6 ++---- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/content_handler/application.cc b/content_handler/application.cc index cb712cefe..cab1b6cee 100644 --- a/content_handler/application.cc +++ b/content_handler/application.cc @@ -20,7 +20,7 @@ namespace flutter { std::pair, std::unique_ptr> Application::Create( - Application::Delegate& delegate, + TerminationCallback termination_callback, component::ApplicationPackage package, component::ApplicationStartupInfo startup_info, fidl::InterfaceRequest 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 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. } diff --git a/content_handler/application.h b/content_handler/application.h index c2df2ff4b..8c958ef8d 100644 --- a/content_handler/application.h +++ b/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; // 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> - Create(Application::Delegate& delegate, + Create(TerminationCallback termination_callback, component::ApplicationPackage package, component::ApplicationStartupInfo startup_info, fidl::InterfaceRequest 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 last_return_code_; Application( - Application::Delegate& delegate, + TerminationCallback termination_callback, component::ApplicationPackage package, component::ApplicationStartupInfo startup_info, fidl::InterfaceRequest controller); diff --git a/content_handler/application_runner.cc b/content_handler/application_runner.cc index 7603234a3..baa9d2411 100644 --- a/content_handler/application_runner.cc +++ b/content_handler/application_runner.cc @@ -65,8 +65,24 @@ void ApplicationRunner::StartApplication( component::ApplicationPackage package, component::ApplicationStartupInfo startup_info, fidl::InterfaceRequest 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 diff --git a/content_handler/application_runner.h b/content_handler/application_runner.h index c8d87b1be..b2e7f3abb 100644 --- a/content_handler/application_runner.h +++ b/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();