fuchsia: Handle multiple views in platformViews path

Author: arbreng

flow/layers/fuchsia_layer_unittests.cc

@@ -20,8 +20,6 @@
 #include "flutter/flow/layers/physical_shape_layer.h"
 #include "flutter/flow/layers/transform_layer.h"
 #include "flutter/flow/view_holder.h"
-#include "flutter/fml/platform/fuchsia/message_loop_fuchsia.h"
-#include "flutter/fml/task_runner.h"
 #include "gtest/gtest.h"
 
 namespace flutter {
@@ -251,10 +249,6 @@ class MockSessionWrapper : public flutter::SessionWrapper {
 };
 
 struct TestContext {
-  // Message loop.
-  fml::RefPtr<fml::MessageLoopFuchsia> loop;
-  fml::RefPtr<fml::TaskRunner> task_runner;
-
   // Session.
   fidl::InterfaceRequest<fuchsia::ui::scenic::SessionListener> listener_request;
   MockSession mock_session;
@@ -273,10 +267,6 @@ struct TestContext {
 std::unique_ptr<TestContext> InitTest() {
   std::unique_ptr<TestContext> context = std::make_unique<TestContext>();
 
-  // Init message loop.
-  context->loop = fml::MakeRefCounted<fml::MessageLoopFuchsia>();
-  context->task_runner = fml::MakeRefCounted<fml::TaskRunner>(context->loop);
-
   // Init Session.
   fuchsia::ui::scenic::SessionPtr session_ptr;
   fuchsia::ui::scenic::SessionListenerPtr listener;
@@ -318,7 +308,7 @@ zx_koid_t GetChildLayerId() {
 class AutoDestroyChildLayerId {
  public:
   AutoDestroyChildLayerId(zx_koid_t id) : id_(id) {}
-  ~AutoDestroyChildLayerId() { ViewHolder::Destroy(id_); }
+  ~AutoDestroyChildLayerId() { ViewHolder::Destroy(id_, nullptr); }
 
  private:
   zx_koid_t id_;
@@ -360,9 +350,10 @@ TEST_F(FuchsiaLayerTest, DISABLED_PhysicalShapeLayersAndChildSceneLayers) {
   const zx_koid_t kChildLayerId1 = GetChildLayerId();
   auto [unused_view_token1, unused_view_holder_token1] =
       scenic::ViewTokenPair::New();
-  ViewHolder::Create(kChildLayerId1, test_context->task_runner,
-                     std::move(unused_view_holder_token1),
-                     /*bind_callback=*/[](scenic::ResourceId id) {});
+  ViewHolder::Create(
+      kChildLayerId1,
+      /*bind_callback=*/[](scenic::ResourceId id) {},
+      std::move(unused_view_holder_token1));
   // Will destroy only when we go out of scope (i.e. end of the test).
   AutoDestroyChildLayerId auto_destroy1(kChildLayerId1);
   auto child_view1 = std::make_shared<ChildSceneLayer>(
@@ -388,9 +379,10 @@ TEST_F(FuchsiaLayerTest, DISABLED_PhysicalShapeLayersAndChildSceneLayers) {
   const zx_koid_t kChildLayerId2 = GetChildLayerId();
   auto [unused_view_token2, unused_view_holder_token2] =
       scenic::ViewTokenPair::New();
-  ViewHolder::Create(kChildLayerId2, test_context->task_runner,
-                     std::move(unused_view_holder_token2),
-                     /*bind_callback=*/[](scenic::ResourceId id) {});
+  ViewHolder::Create(
+      kChildLayerId2,
+      /*bind_callback=*/[](scenic::ResourceId id) {},
+      std::move(unused_view_holder_token2));
   // Will destroy only when we go out of scope (i.e. end of the test).
   AutoDestroyChildLayerId auto_destroy2(kChildLayerId2);
   auto child_view2 = std::make_shared<ChildSceneLayer>(
@@ -604,9 +596,10 @@ TEST_F(FuchsiaLayerTest, DISABLED_OpacityAndTransformLayer) {
   auto [unused_view_token1, unused_view_holder_token1] =
       scenic::ViewTokenPair::New();
 
-  ViewHolder::Create(kChildLayerId1, test_context->task_runner,
-                     std::move(unused_view_holder_token1),
-                     /*bind_callback=*/[](scenic::ResourceId id) {});
+  ViewHolder::Create(
+      kChildLayerId1,
+      /*bind_callback=*/[](scenic::ResourceId id) {},
+      std::move(unused_view_holder_token1));
   // Will destroy only when we go out of scope (i.e. end of the test).
   AutoDestroyChildLayerId auto_destroy1(kChildLayerId1);
   auto child_view1 = std::make_shared<ChildSceneLayer>(

flow/scene_update_context.cc

@@ -223,13 +223,13 @@ void SceneUpdateContext::UpdateView(int64_t view_id,
 }
 
 void SceneUpdateContext::CreateView(int64_t view_id,
+                                    ViewHolder::ViewIdCallback on_view_created,
                                     bool hit_testable,
                                     bool focusable) {
   FML_LOG(INFO) << "CreateView for view holder: " << view_id;
   zx_handle_t handle = (zx_handle_t)view_id;
-  flutter::ViewHolder::Create(handle, nullptr,
-                              scenic::ToViewHolderToken(zx::eventpair(handle)),
-                              nullptr);
+  flutter::ViewHolder::Create(handle, std::move(on_view_created),
+                              scenic::ToViewHolderToken(zx::eventpair(handle)));
   auto* view_holder = ViewHolder::FromId(view_id);
   FML_DCHECK(view_holder);
 
@@ -250,8 +250,10 @@ void SceneUpdateContext::UpdateView(int64_t view_id,
   view_holder->set_focusable(focusable);
 }
 
-void SceneUpdateContext::DestroyView(int64_t view_id) {
-  ViewHolder::Destroy(view_id);
+void SceneUpdateContext::DestroyView(
+    int64_t view_id,
+    ViewHolder::ViewIdCallback on_view_destroyed) {
+  ViewHolder::Destroy(view_id, std::move(on_view_destroyed));
 }
 
 SceneUpdateContext::Entity::Entity(std::shared_ptr<SceneUpdateContext> context)

flow/scene_update_context.h

@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "flutter/flow/embedded_views.h"
+#include "flutter/flow/view_holder.h"
 #include "flutter/fml/logging.h"
 #include "flutter/fml/macros.h"
 #include "third_party/skia/include/core/SkRect.h"
@@ -167,9 +168,14 @@ class SceneUpdateContext : public flutter::ExternalViewEmbedder {
     return nullptr;
   }
 
-  void CreateView(int64_t view_id, bool hit_testable, bool focusable);
+  // View manipulation.
+  void CreateView(int64_t view_id,
+                  ViewHolder::ViewIdCallback on_view_created,
+                  bool hit_testable,
+                  bool focusable);
+  void DestroyView(int64_t view_id,
+                   ViewHolder::ViewIdCallback on_view_destroyed);
   void UpdateView(int64_t view_id, bool hit_testable, bool focusable);
-  void DestroyView(int64_t view_id);
   void UpdateView(int64_t view_id,
                   const SkPoint& offset,
                   const SkSize& size,

flow/view_holder.cc

@@ -51,9 +51,8 @@ fuchsia::ui::gfx::ViewProperties ToViewProperties(float width,
 namespace flutter {
 
 void ViewHolder::Create(zx_koid_t id,
-                        fml::RefPtr<fml::TaskRunner> ui_task_runner,
-                        fuchsia::ui::views::ViewHolderToken view_holder_token,
-                        const BindCallback& on_bind_callback) {
+                        ViewIdCallback on_view_created,
+                        fuchsia::ui::views::ViewHolderToken view_holder_token) {
   // This raster thread contains at least 1 ViewHolder.  Initialize the
   // per-thread bindings.
   if (tls_view_holder_bindings.get() == nullptr) {
@@ -64,16 +63,20 @@ void ViewHolder::Create(zx_koid_t id,
   FML_DCHECK(bindings);
   FML_DCHECK(bindings->find(id) == bindings->end());
 
-  auto view_holder = std::make_unique<ViewHolder>(std::move(ui_task_runner),
-                                                  std::move(view_holder_token),
-                                                  on_bind_callback);
+  auto view_holder = std::unique_ptr<ViewHolder>(
+      new ViewHolder(std::move(view_holder_token), std::move(on_view_created)));
   bindings->emplace(id, std::move(view_holder));
 }
 
-void ViewHolder::Destroy(zx_koid_t id) {
+void ViewHolder::Destroy(zx_koid_t id, ViewIdCallback on_view_destroyed) {
   auto* bindings = tls_view_holder_bindings.get();
   FML_DCHECK(bindings);
+  auto binding = bindings->find(id);
+  FML_DCHECK(binding != bindings->end());
 
+  if (binding->second->view_holder_) {
+    on_view_destroyed(binding->second->view_holder_->id());
+  }
   bindings->erase(id);
 }
 
@@ -91,12 +94,10 @@ ViewHolder* ViewHolder::FromId(zx_koid_t id) {
   return binding->second.get();
 }
 
-ViewHolder::ViewHolder(fml::RefPtr<fml::TaskRunner> ui_task_runner,
-                       fuchsia::ui::views::ViewHolderToken view_holder_token,
-                       const BindCallback& on_bind_callback)
-    : ui_task_runner_(std::move(ui_task_runner)),
-      pending_view_holder_token_(std::move(view_holder_token)),
-      pending_bind_callback_(on_bind_callback) {
+ViewHolder::ViewHolder(fuchsia::ui::views::ViewHolderToken view_holder_token,
+                       ViewIdCallback on_view_created)
+    : pending_view_holder_token_(std::move(view_holder_token)),
+      on_view_created_(std::move(on_view_created)) {
   FML_DCHECK(pending_view_holder_token_.value);
 }
 
@@ -114,12 +115,13 @@ void ViewHolder::UpdateScene(scenic::Session* session,
     opacity_node_->AddChild(*entity_node_);
     opacity_node_->SetLabel("flutter::ViewHolder");
     entity_node_->Attach(*view_holder_);
-    if (ui_task_runner_ && pending_view_holder_token_.value) {
-      ui_task_runner_->PostTask(
-          [bind_callback = std::move(pending_bind_callback_),
-           view_holder_id = view_holder_->id()]() {
-            bind_callback(view_holder_id);
-          });
+
+    // Inform the rest of Flutter about the view being created.
+    // As long as we do this before calling `Present` on the session,
+    // View-related messages sent to the UI thread will never be processed
+    // before this internal message is delivered to the UI thread.
+    if (on_view_created_) {
+      on_view_created_(view_holder_->id());
     }
   }
   FML_DCHECK(entity_node_);

flow/view_holder.h

@@ -29,18 +29,20 @@ namespace flutter {
 // This object is created and destroyed on the |Rasterizer|'s thread.
 class ViewHolder {
  public:
-  using BindCallback = std::function<void(scenic::ResourceId)>;
-
+  using ViewIdCallback = std::function<void(scenic::ResourceId)>;
+
+  // `Create`, `Destroy`, and `FromId` must be executed on the raster thread or
+  // errors will occur.
+  //
+  // `on_bind_callback` and `on_unbind_callback` will likewise execute on the
+  // raster thread; clients are responsible for re-threading the callback if
+  // needed.
   static void Create(zx_koid_t id,
-                     fml::RefPtr<fml::TaskRunner> ui_task_runner,
-                     fuchsia::ui::views::ViewHolderToken view_holder_token,
-                     const BindCallback& on_bind_callback);
-  static void Destroy(zx_koid_t id);
+                     ViewIdCallback on_view_created,
+                     fuchsia::ui::views::ViewHolderToken view_holder_token);
+  static void Destroy(zx_koid_t id, ViewIdCallback on_view_destroyed);
   static ViewHolder* FromId(zx_koid_t id);
 
-  ViewHolder(fml::RefPtr<fml::TaskRunner> ui_task_runner,
-             fuchsia::ui::views::ViewHolderToken view_holder_token,
-             const BindCallback& on_bind_callback);
   ~ViewHolder() = default;
 
   // Sets the properties/opacity of the child view by issuing a Scenic command.
@@ -68,18 +70,20 @@ class ViewHolder {
   void set_focusable(bool value) { focusable_ = value; }
 
  private:
-  fml::RefPtr<fml::TaskRunner> ui_task_runner_;
+  ViewHolder(fuchsia::ui::views::ViewHolderToken view_holder_token,
+             ViewIdCallback on_view_created);
 
   std::unique_ptr<scenic::EntityNode> entity_node_;
   std::unique_ptr<scenic::OpacityNodeHACK> opacity_node_;
   std::unique_ptr<scenic::ViewHolder> view_holder_;
 
   fuchsia::ui::views::ViewHolderToken pending_view_holder_token_;
-  BindCallback pending_bind_callback_;
 
   bool hit_testable_ = true;
   bool focusable_ = true;
 
+  ViewIdCallback on_view_created_;
+
   fuchsia::ui::gfx::ViewProperties pending_properties_;
   bool has_pending_properties_ = false;
 

lib/ui/compositing/scene_host.cc

@@ -19,24 +19,26 @@ namespace {
 
 struct SceneHostBindingKey {
   std::string isolate_service_id;
-  zx_koid_t koid;
+  scenic::ResourceId resource_id;
 
-  SceneHostBindingKey(const zx_koid_t koid,
-                      const std::string isolate_service_id) {
-    this->koid = koid;
+  SceneHostBindingKey(const std::string isolate_service_id,
+                      scenic::ResourceId resource_id) {
     this->isolate_service_id = isolate_service_id;
+    this->resource_id = resource_id;
   }
 
   bool operator==(const SceneHostBindingKey& other) const {
-    return isolate_service_id == other.isolate_service_id && koid == other.koid;
+    return isolate_service_id == other.isolate_service_id &&
+           resource_id == other.resource_id;
   }
 };
 
 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;
+    std::size_t resource_id_hash =
+        std::hash<scenic::ResourceId>()(key.resource_id);
+    return isolate_hash ^ resource_id_hash;
   }
 };
 
@@ -54,7 +56,7 @@ void SceneHost_constructor(Dart_NativeArguments args) {
 flutter::SceneHost* GetSceneHost(scenic::ResourceId id,
                                  std::string isolate_service_id) {
   auto binding =
-      scene_host_bindings.find(SceneHostBindingKey(id, isolate_service_id));
+      scene_host_bindings.find(SceneHostBindingKey(isolate_service_id, id));
   if (binding == scene_host_bindings.end()) {
     return nullptr;
   } else {
@@ -165,7 +167,8 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> viewHolderToken,
                      Dart_Handle viewStateChangedCallback)
     : raster_task_runner_(
           UIDartState::Current()->GetTaskRunners().GetRasterTaskRunner()),
-      koid_(GetKoid(viewHolderToken->handle())) {
+      koid_(GetKoid(viewHolderToken->handle())),
+      weak_factory_(this) {
   auto dart_state = UIDartState::Current();
   isolate_service_id_ = Dart_IsolateServiceId(Dart_CurrentIsolate());
 
@@ -180,34 +183,47 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> viewHolderToken,
     view_state_changed_callback_.Set(dart_state, viewStateChangedCallback);
   }
 
-  // This callback will be posted as a task  when the |scenic::ViewHolder|
-  // resource is created and given an id by the raster thread.
-  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));
+  // This callback is invoked on the raster thread when the |scenic::ViewHolder|
+  // resource is created and given an id.
+  auto bind_callback = [weak = weak_factory_.GetWeakPtr(),
+                        ui_task_runner =
+                            dart_state->GetTaskRunners().GetUITaskRunner()](
+                           scenic::ResourceId id) {
+    ui_task_runner->PostTask([weak, id]() {
+      if (!weak) {
+        FML_LOG(WARNING) << "ViewHolder bound to SceneHost after SceneHost was "
+                            "destroyed; ignoring.";
+        return;
+      }
+
+      FML_DCHECK(weak->resource_id_ == 0);
+      weak->resource_id_ = id;
+
+      const auto key =
+          SceneHostBindingKey(weak->isolate_service_id_, weak->resource_id_);
+      scene_host_bindings.emplace(std::make_pair(key, weak.get()));
+    });
   };
 
   // Pass the raw handle to the raster thread; destroying a
   // |zircon::dart::Handle| on that thread can cause a race condition.
-  raster_task_runner_->PostTask(
-      [id = koid_,
-       ui_task_runner =
-           UIDartState::Current()->GetTaskRunners().GetUITaskRunner(),
-       raw_handle = viewHolderToken->ReleaseHandle(), bind_callback]() {
-        flutter::ViewHolder::Create(
-            id, std::move(ui_task_runner),
-            scenic::ToViewHolderToken(zx::eventpair(raw_handle)),
-            std::move(bind_callback));
-      });
+  raster_task_runner_->PostTask([koid = koid_,
+                                 raw_handle = viewHolderToken->ReleaseHandle(),
+                                 bind_callback = std::move(bind_callback)]() {
+    flutter::ViewHolder::Create(
+        koid, std::move(bind_callback),
+        scenic::ToViewHolderToken(zx::eventpair(raw_handle)));
+  });
 }
 
 SceneHost::~SceneHost() {
-  scene_host_bindings.erase(SceneHostBindingKey(koid_, isolate_service_id_));
+  if (resource_id_ != 0) {
+    scene_host_bindings.erase(
+        SceneHostBindingKey(isolate_service_id_, resource_id_));
+  }
 
   raster_task_runner_->PostTask(
-      [id = koid_]() { flutter::ViewHolder::Destroy(id); });
+      [koid = koid_]() { flutter::ViewHolder::Destroy(koid, nullptr); });
 }
 
 void SceneHost::dispose() {