[fuchsia] SceneHostBindings are no longer thread locals (#16262)

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 flutter/flutter#49738

Author: iskakaushik

lib/ui/compositing/scene_host.cc

@@ -10,37 +10,65 @@
 #include <third_party/tonic/dart_binding_macros.h>
 #include <third_party/tonic/logging/dart_invoke.h>
 
+#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<zx_koid_t, flutter::SceneHost*>;
+struct SceneHostBindingKey {
+  std::string isolate_service_id;
+  zx_koid_t koid;
 
-FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<SceneHostBindings>
-    tls_scene_host_bindings;
+  SceneHostBindingKey(const zx_koid_t koid,
+                      const std::string isolate_service_id) {
+    this->koid = koid;
+    this->isolate_service_id = isolate_service_id;
+  }
 
-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<zx_koid_t>()(key.koid);
+    std::size_t isolate_hash = std::hash<std::string>()(key.isolate_service_id);
+    return koid_hash ^ isolate_hash;
   }
+};
+
+using SceneHostBindings = std::unordered_map<SceneHostBindingKey,
+                                             flutter::SceneHost*,
+                                             SceneHostBindingKeyHasher>;
+
+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()) {
+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) {
@@ -107,23 +135,23 @@ fml::RefPtr<SceneHost> 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_);
   }
 }
 
 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_);
   }
 }
 
 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);
@@ -138,6 +166,7 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> 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)) {
@@ -152,12 +181,11 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> 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));
+  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));
   };
 
   // Pass the raw handle to the GPU thead; destroying a |zircon::dart::Handle|
@@ -175,9 +203,7 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> viewHolderToken,
 }
 
 SceneHost::~SceneHost() {
-  auto* bindings = tls_scene_host_bindings.get();
-  FML_DCHECK(bindings);
-  bindings->erase(koid_);
+  scene_host_bindings.erase(SceneHostBindingKey(koid_, isolate_service_id_));
 
   gpu_task_runner_->PostTask(
       [id = koid_]() { flutter::ViewHolder::Destroy(id); });

lib/ui/compositing/scene_host.h

@@ -57,6 +57,7 @@ class SceneHost : public RefCountedDartWrappable<SceneHost> {
   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;
 };