From 756b95743ddd877d6fb6d6ae0462bcc6eb31c0dc Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Thu, 30 Jan 2020 11:44:51 -0800 Subject: [PATCH 1/3] [fuchsia] SceneHostBindings are no longer thread locals Prior to this change SceneHostBindinds was a ThreadLocal but the intention was for it to be IsolateLocal. Given that dart could collect this map on a non-UI thread this caused use-after-free issues. This change fixes it by making it keyed on isolate and koid this is not the ideal solution, this would exist on dart isolate group data struct. Given that Fuchsia is moving to use the embedder API, the decision to use this temporary work around was made. fixes https://github.com/flutter/flutter/issues/49738 --- lib/ui/compositing/scene_host.cc | 53 +++++++++++++++++++------------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/lib/ui/compositing/scene_host.cc b/lib/ui/compositing/scene_host.cc index e4f83cc3ff0e8..05971e87b6372 100644 --- a/lib/ui/compositing/scene_host.cc +++ b/lib/ui/compositing/scene_host.cc @@ -10,33 +10,48 @@ #include #include +#include "dart/runtime/include/dart_api.h" #include "flutter/flow/view_holder.h" #include "flutter/fml/thread_local.h" #include "flutter/lib/ui/ui_dart_state.h" namespace { -using SceneHostBindings = std::unordered_map; +struct SceneHostBindingKey { + std::string isolate_service_id; + zx_koid_t koid; -FML_THREAD_LOCAL fml::ThreadLocalUniquePtr - tls_scene_host_bindings; + SceneHostBindingKey(const zx_koid_t koid) { + this->koid = koid; + this->isolate_service_id = Dart_IsolateServiceId(Dart_CurrentIsolate()); + } -void SceneHost_constructor(Dart_NativeArguments args) { - // This UI thread / Isolate contains at least 1 SceneHost. Initialize the - // per-Isolate bindings. - if (tls_scene_host_bindings.get() == nullptr) { - tls_scene_host_bindings.reset(new SceneHostBindings()); + bool operator==(const SceneHostBindingKey& other) const { + return isolate_service_id == other.isolate_service_id && koid == other.koid; } +}; + +struct SceneHostBindingKeyHasher { + std::size_t operator()(const SceneHostBindingKey& key) const { + std::size_t koid_hash = std::hash()(key.koid); + std::size_t isolate_hash = std::hash()(key.isolate_service_id); + return koid_hash ^ isolate_hash; + } +}; + +using SceneHostBindings = std::unordered_map; +static SceneHostBindings scene_host_bindings; + +void SceneHost_constructor(Dart_NativeArguments args) { tonic::DartCallConstructor(&flutter::SceneHost::Create, args); } flutter::SceneHost* GetSceneHost(scenic::ResourceId id) { - auto* bindings = tls_scene_host_bindings.get(); - FML_DCHECK(bindings); - - auto binding = bindings->find(id); - if (binding != bindings->end()) { + auto binding = scene_host_bindings.find(id); + if (binding != scene_host_bindings.end()) { return binding->second; } @@ -153,11 +168,9 @@ SceneHost::SceneHost(fml::RefPtr viewHolderToken, // This callback will be posted as a task when the |scenic::ViewHolder| // resource is created and given an id by the GPU thread. auto bind_callback = [scene_host = this](scenic::ResourceId id) { - auto* bindings = tls_scene_host_bindings.get(); - FML_DCHECK(bindings); - FML_DCHECK(bindings->find(id) == bindings->end()); - - bindings->emplace(std::make_pair(id, scene_host)); + FML_DCHECK(scene_host_bindings.find(id) == scene_host_bindings.end()); + const auto key = SceneHostBindingKey(id); + scene_host_bindings.emplace(std::make_pair(key, scene_host)); }; // Pass the raw handle to the GPU thead; destroying a |zircon::dart::Handle| @@ -175,9 +188,7 @@ SceneHost::SceneHost(fml::RefPtr viewHolderToken, } SceneHost::~SceneHost() { - auto* bindings = tls_scene_host_bindings.get(); - FML_DCHECK(bindings); - bindings->erase(koid_); + scene_host_bindings.erase(koid_); gpu_task_runner_->PostTask( [id = koid_]() { flutter::ViewHolder::Destroy(id); }); From b3423ed912d7b70f317ea00e762915e43769de5e Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Thu, 30 Jan 2020 14:06:03 -0800 Subject: [PATCH 2/3] use isolate service id --- lib/ui/compositing/scene_host.cc | 43 ++++++++++++++++++++++---------- lib/ui/compositing/scene_host.h | 1 + 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/lib/ui/compositing/scene_host.cc b/lib/ui/compositing/scene_host.cc index 05971e87b6372..aff7f6a320d6a 100644 --- a/lib/ui/compositing/scene_host.cc +++ b/lib/ui/compositing/scene_host.cc @@ -21,9 +21,12 @@ struct SceneHostBindingKey { std::string isolate_service_id; zx_koid_t koid; - SceneHostBindingKey(const zx_koid_t koid) { + SceneHostBindingKey(const zx_koid_t koid, + const std::string isolate_service_id) { this->koid = koid; - this->isolate_service_id = Dart_IsolateServiceId(Dart_CurrentIsolate()); + this->isolate_service_id = isolate_service_id; + FML_LOG(ERROR) << "SceneHostBindingKey koid:: " << koid + << ", isolate: " << isolate_service_id; } bool operator==(const SceneHostBindingKey& other) const { @@ -49,13 +52,25 @@ void SceneHost_constructor(Dart_NativeArguments args) { tonic::DartCallConstructor(&flutter::SceneHost::Create, args); } -flutter::SceneHost* GetSceneHost(scenic::ResourceId id) { - auto binding = scene_host_bindings.find(id); - if (binding != scene_host_bindings.end()) { +flutter::SceneHost* GetSceneHost(scenic::ResourceId id, + std::string isolate_service_id) { + auto binding = + scene_host_bindings.find(SceneHostBindingKey(id, isolate_service_id)); + if (binding == scene_host_bindings.end()) { + return nullptr; + } else { return binding->second; } +} - return nullptr; +flutter::SceneHost* GetSceneHostForCurrentIsolate(scenic::ResourceId id) { + auto isolate = Dart_CurrentIsolate(); + if (!isolate) { + return nullptr; + } else { + std::string isolate_service_id = Dart_IsolateServiceId(isolate); + return GetSceneHost(id, isolate_service_id); + } } void InvokeDartClosure(const tonic::DartPersistentValue& closure) { @@ -122,7 +137,7 @@ fml::RefPtr SceneHost::Create( } void SceneHost::OnViewConnected(scenic::ResourceId id) { - auto* scene_host = GetSceneHost(id); + auto* scene_host = GetSceneHostForCurrentIsolate(id); if (scene_host && !scene_host->view_connected_callback_.is_empty()) { InvokeDartClosure(scene_host->view_connected_callback_); @@ -130,7 +145,7 @@ void SceneHost::OnViewConnected(scenic::ResourceId id) { } void SceneHost::OnViewDisconnected(scenic::ResourceId id) { - auto* scene_host = GetSceneHost(id); + auto* scene_host = GetSceneHostForCurrentIsolate(id); if (scene_host && !scene_host->view_disconnected_callback_.is_empty()) { InvokeDartClosure(scene_host->view_disconnected_callback_); @@ -138,7 +153,7 @@ void SceneHost::OnViewDisconnected(scenic::ResourceId id) { } void SceneHost::OnViewStateChanged(scenic::ResourceId id, bool state) { - auto* scene_host = GetSceneHost(id); + auto* scene_host = GetSceneHostForCurrentIsolate(id); if (scene_host && !scene_host->view_state_changed_callback_.is_empty()) { InvokeDartFunction(scene_host->view_state_changed_callback_, state); @@ -153,6 +168,7 @@ SceneHost::SceneHost(fml::RefPtr viewHolderToken, UIDartState::Current()->GetTaskRunners().GetGPUTaskRunner()), koid_(GetKoid(viewHolderToken->handle())) { auto dart_state = UIDartState::Current(); + isolate_service_id_ = Dart_IsolateServiceId(Dart_CurrentIsolate()); // Initialize callbacks it they are non-null in Dart. if (!Dart_IsNull(viewConnectedCallback)) { @@ -167,9 +183,10 @@ SceneHost::SceneHost(fml::RefPtr viewHolderToken, // This callback will be posted as a task when the |scenic::ViewHolder| // resource is created and given an id by the GPU thread. - auto bind_callback = [scene_host = this](scenic::ResourceId id) { - FML_DCHECK(scene_host_bindings.find(id) == scene_host_bindings.end()); - const auto key = SceneHostBindingKey(id); + auto bind_callback = [scene_host = this, + isolate_service_id = + isolate_service_id_](scenic::ResourceId id) { + const auto key = SceneHostBindingKey(id, isolate_service_id); scene_host_bindings.emplace(std::make_pair(key, scene_host)); }; @@ -188,7 +205,7 @@ SceneHost::SceneHost(fml::RefPtr viewHolderToken, } SceneHost::~SceneHost() { - scene_host_bindings.erase(koid_); + scene_host_bindings.erase(SceneHostBindingKey(koid_, isolate_service_id_)); gpu_task_runner_->PostTask( [id = koid_]() { flutter::ViewHolder::Destroy(id); }); diff --git a/lib/ui/compositing/scene_host.h b/lib/ui/compositing/scene_host.h index 05c36e3c4f0cf..bf501a128ea2f 100644 --- a/lib/ui/compositing/scene_host.h +++ b/lib/ui/compositing/scene_host.h @@ -57,6 +57,7 @@ class SceneHost : public RefCountedDartWrappable { tonic::DartPersistentValue view_connected_callback_; tonic::DartPersistentValue view_disconnected_callback_; tonic::DartPersistentValue view_state_changed_callback_; + std::string isolate_service_id_; zx_koid_t koid_ = ZX_KOID_INVALID; }; From ce5c51b2501f5f944f7c7f6c4ccf46b8e7e15d18 Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Thu, 30 Jan 2020 14:08:50 -0800 Subject: [PATCH 3/3] remove log line --- lib/ui/compositing/scene_host.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ui/compositing/scene_host.cc b/lib/ui/compositing/scene_host.cc index aff7f6a320d6a..0f09ac66ea8be 100644 --- a/lib/ui/compositing/scene_host.cc +++ b/lib/ui/compositing/scene_host.cc @@ -25,8 +25,6 @@ struct SceneHostBindingKey { const std::string isolate_service_id) { this->koid = koid; this->isolate_service_id = isolate_service_id; - FML_LOG(ERROR) << "SceneHostBindingKey koid:: " << koid - << ", isolate: " << isolate_service_id; } bool operator==(const SceneHostBindingKey& other) const {