Keep a copy of each engine's description that can be accessed outside…

… the engine's UI thread (#6885) The service protocol's ListViews method needs to return description data for each engine in the process. Previously ListViews would queue a task to each UI thread to gather this data. However, the UI thread might be blocked from executing tasks (e.g. if the Dart isolate is paused), resulting in a deadlock. This change provides a copy of the engine's description data to the ServiceProtocol's global list of engines, allowing ListViews to run without accessing any UI threads. Fixes flutter/flutter#24400
flutter · Nov 16, 2018 · 3978f07 · 3978f07
1 parent 0870e37
commit 3978f07
Show file tree

Hide file tree

Showing 13 changed files with 71 additions and 37 deletions.
diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc
@@ -54,7 +54,13 @@ void UIDartState::DidSetIsolate() {
   // main.dart$main-1234
   debug_name << advisory_script_uri_ << "$" << advisory_script_entrypoint_
              << "-" << main_port_;
-  debug_name_ = debug_name.str();
+  SetDebugName(debug_name.str());
+}
+
+void UIDartState::SetDebugName(const std::string debug_name) {
+  debug_name_ = debug_name;
+  if (window_)
+    window_->client()->UpdateIsolateDescription(debug_name_, main_port_);
 }
 
 UIDartState* UIDartState::Current() {
@@ -63,6 +69,8 @@ UIDartState* UIDartState::Current() {
 
 void UIDartState::SetWindow(std::unique_ptr<Window> window) {
   window_ = std::move(window);
+  if (window_)
+    window_->client()->UpdateIsolateDescription(debug_name_, main_port_);
 }
 
 const TaskRunners& UIDartState::GetTaskRunners() const {

diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h
@@ -32,7 +32,7 @@ class UIDartState : public tonic::DartState {
 
   Dart_Port main_port() const { return main_port_; }
 
-  void set_debug_name(const std::string name) { debug_name_ = name; }
+  void SetDebugName(const std::string name);
 
   const std::string& debug_name() const { return debug_name_; }
 

diff --git a/lib/ui/window/window.cc b/lib/ui/window/window.cc
@@ -62,7 +62,7 @@ void SetIsolateDebugName(Dart_NativeArguments args) {
     Dart_ThrowException(exception);
     return;
   }
-  UIDartState::Current()->window()->client()->SetIsolateDebugName(name);
+  UIDartState::Current()->SetDebugName(name);
 }
 
 Dart_Handle SendPlatformMessage(Dart_Handle window,

diff --git a/lib/ui/window/window.h b/lib/ui/window/window.h
@@ -43,8 +43,9 @@ class WindowClient {
   virtual void Render(Scene* scene) = 0;
   virtual void UpdateSemantics(SemanticsUpdate* update) = 0;
   virtual void HandlePlatformMessage(fml::RefPtr<PlatformMessage> message) = 0;
-  virtual void SetIsolateDebugName(const std::string isolateName) = 0;
   virtual FontCollection& GetFontCollection() = 0;
+  virtual void UpdateIsolateDescription(const std::string isolate_name,
+                                        int64_t isolate_port) = 0;
 
  protected:
   virtual ~WindowClient();

diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc
@@ -267,18 +267,15 @@ void RuntimeController::HandlePlatformMessage(
   client_.HandlePlatformMessage(std::move(message));
 }
 
-void RuntimeController::SetIsolateDebugName(const std::string name) {
-  std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
-  if (!root_isolate) {
-    return;
-  }
-  root_isolate->set_debug_name(name);
-}
-
 FontCollection& RuntimeController::GetFontCollection() {
   return client_.GetFontCollection();
 }
 
+void RuntimeController::UpdateIsolateDescription(const std::string isolate_name,
+                                                 int64_t isolate_port) {
+  client_.UpdateIsolateDescription(isolate_name, isolate_port);
+}
+
 Dart_Port RuntimeController::GetMainPort() {
   std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
   return root_isolate ? root_isolate->main_port() : ILLEGAL_PORT;

diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h
@@ -158,10 +158,11 @@ class RuntimeController final : public WindowClient {
   void HandlePlatformMessage(fml::RefPtr<PlatformMessage> message) override;
 
   // |blink::WindowClient|
-  void SetIsolateDebugName(const std::string name) override;
+  FontCollection& GetFontCollection() override;
 
   // |blink::WindowClient|
-  FontCollection& GetFontCollection() override;
+  void UpdateIsolateDescription(const std::string isolate_name,
+                                int64_t isolate_port) override;
 
   FML_DISALLOW_COPY_AND_ASSIGN(RuntimeController);
 };

diff --git a/runtime/runtime_delegate.h b/runtime/runtime_delegate.h
@@ -33,6 +33,9 @@ class RuntimeDelegate {
 
   virtual FontCollection& GetFontCollection() = 0;
 
+  virtual void UpdateIsolateDescription(const std::string isolate_name,
+                                        int64_t isolate_port) = 0;
+
  protected:
   virtual ~RuntimeDelegate();
 };

diff --git a/runtime/service_protocol.cc b/runtime/service_protocol.cc
@@ -51,16 +51,25 @@ ServiceProtocol::~ServiceProtocol() {
   ToggleHooks(false);
 }
 
-void ServiceProtocol::AddHandler(Handler* handler) {
+void ServiceProtocol::AddHandler(Handler* handler,
+                                 Handler::Description description) {
   std::lock_guard<std::mutex> lock(handlers_mutex_);
-  handlers_.emplace(handler);
+  handlers_.emplace(handler, description);
 }
 
 void ServiceProtocol::RemoveHandler(Handler* handler) {
   std::lock_guard<std::mutex> lock(handlers_mutex_);
   handlers_.erase(handler);
 }
 
+void ServiceProtocol::SetHandlerDescription(Handler* handler,
+                                            Handler::Description description) {
+  std::lock_guard<std::mutex> lock(handlers_mutex_);
+  auto it = handlers_.find(handler);
+  if (it != handlers_.end())
+    it->second = description;
+}
+
 void ServiceProtocol::ToggleHooks(bool set) {
   for (const auto& endpoint : endpoints_) {
     Dart_RegisterIsolateServiceRequestCallback(
@@ -191,7 +200,8 @@ bool ServiceProtocol::HandleMessage(fml::StringView method,
   if (method == kScreenshotExtensionName ||
       method == kScreenshotSkpExtensionName ||
       method == kFlushUIThreadTasksExtensionName) {
-    return HandleMessageOnHandler(*handlers_.begin(), method, params, response);
+    return HandleMessageOnHandler(handlers_.begin()->first, method, params,
+                                  response);
   }
 
   WriteServerErrorResponse(
@@ -239,23 +249,9 @@ bool ServiceProtocol::HandleListViewsMethod(
   // Collect handler descriptions on their respective task runners.
   std::lock_guard<std::mutex> lock(handlers_mutex_);
   std::vector<std::pair<intptr_t, Handler::Description>> descriptions;
-  for (auto* const handler : handlers_) {
-    fml::AutoResetWaitableEvent latch;
-    Handler::Description description;
-
-    fml::TaskRunner::RunNowOrPostTask(
-        handler->GetServiceProtocolHandlerTaskRunner(
-            kListViewsExtensionName),  // task runner
-        [&latch,                       //
-         &description,                 //
-         &handler                      //
-    ]() {
-          description = handler->GetServiceProtocolDescription();
-          latch.Signal();
-        });
-    latch.Wait();
-    descriptions.emplace_back(std::make_pair<intptr_t, Handler::Description>(
-        reinterpret_cast<intptr_t>(handler), std::move(description)));
+  for (const auto& handler : handlers_) {
+    descriptions.emplace_back(reinterpret_cast<intptr_t>(handler.first),
+                              handler.second);
   }
 
   auto& allocator = response.GetAllocator();

diff --git a/runtime/service_protocol.h b/runtime/service_protocol.h
@@ -63,14 +63,17 @@ class ServiceProtocol {
 
   void ToggleHooks(bool set);
 
-  void AddHandler(Handler* handler);
+  void AddHandler(Handler* handler, Handler::Description description);
 
   void RemoveHandler(Handler* handler);
 
+  void SetHandlerDescription(Handler* handler,
+                             Handler::Description description);
+
  private:
   const std::set<fml::StringView> endpoints_;
   mutable std::mutex handlers_mutex_;
-  std::set<Handler*> handlers_;
+  std::map<Handler*, Handler::Description> handlers_;
 
   FML_WARN_UNUSED_RESULT
   static bool HandleMessage(const char* method,

diff --git a/shell/common/engine.cc b/shell/common/engine.cc
@@ -416,6 +416,11 @@ void Engine::HandlePlatformMessage(
   }
 }
 
+void Engine::UpdateIsolateDescription(const std::string isolate_name,
+                                      int64_t isolate_port) {
+  delegate_.UpdateIsolateDescription(isolate_name, isolate_port);
+}
+
 blink::FontCollection& Engine::GetFontCollection() {
   return font_collection_;
 }

diff --git a/shell/common/engine.h b/shell/common/engine.h
@@ -48,6 +48,9 @@ class Engine final : public blink::RuntimeDelegate {
         fml::RefPtr<blink::PlatformMessage> message) = 0;
 
     virtual void OnPreEngineRestart() = 0;
+
+    virtual void UpdateIsolateDescription(const std::string isolate_name,
+                                          int64_t isolate_port) = 0;
   };
 
   Engine(Delegate& delegate,
@@ -142,6 +145,10 @@ class Engine final : public blink::RuntimeDelegate {
   void HandlePlatformMessage(
       fml::RefPtr<blink::PlatformMessage> message) override;
 
+  // |blink::RuntimeDelegate|
+  void UpdateIsolateDescription(const std::string isolate_name,
+                                int64_t isolate_port) override;
+
   void StopAnimator();
 
   void StartAnimatorIfPossible();

diff --git a/shell/common/shell.cc b/shell/common/shell.cc
@@ -373,7 +373,7 @@ bool Shell::Setup(std::unique_ptr<PlatformView> platform_view,
   is_setup_ = true;
 
   if (auto vm = blink::DartVM::ForProcessIfInitialized()) {
-    vm->GetServiceProtocol().AddHandler(this);
+    vm->GetServiceProtocol().AddHandler(this, GetServiceProtocolDescription());
   }
 
   PersistentCache::GetCacheForProcess()->AddWorkerTaskRunner(
@@ -749,6 +749,15 @@ void Shell::OnPreEngineRestart() {
   latch.Wait();
 }
 
+// |shell::Engine::Delegate|
+void Shell::UpdateIsolateDescription(const std::string isolate_name,
+                                     int64_t isolate_port) {
+  if (auto vm = blink::DartVM::ForProcessIfInitialized()) {
+    Handler::Description description(isolate_port, isolate_name);
+    vm->GetServiceProtocol().SetHandlerDescription(this, description);
+  }
+}
+
 // |blink::ServiceProtocol::Handler|
 fml::RefPtr<fml::TaskRunner> Shell::GetServiceProtocolHandlerTaskRunner(
     fml::StringView method) const {

diff --git a/shell/common/shell.h b/shell/common/shell.h
@@ -182,6 +182,10 @@ class Shell final : public PlatformView::Delegate,
   // |shell::Engine::Delegate|
   void OnPreEngineRestart() override;
 
+  // |shell::Engine::Delegate|
+  void UpdateIsolateDescription(const std::string isolate_name,
+                                int64_t isolate_port) override;
+
   // |blink::ServiceProtocol::Handler|
   fml::RefPtr<fml::TaskRunner> GetServiceProtocolHandlerTaskRunner(
       fml::StringView method) const override;