Fix crash when removing stories. (#4003)

Ensure that a Mozart EntityNode (that corresponds
to an ExportNode) is always released on the
Rasterizer thread.

MZ-259

flow/export_node.cc

@@ -4,25 +4,57 @@
 
 #include "flutter/flow/export_node.h"
 
+#include "flutter/common/threads.h"
+#include "lib/ftl/functional/make_copyable.h"
+
 namespace flow {
 
+ExportNodeHolder::ExportNodeHolder(
+    ftl::RefPtr<fidl::dart::Handle> export_token_handle)
+    : export_node_(std::make_unique<ExportNode>(export_token_handle)) {
+  ASSERT_IS_UI_THREAD;
+}
+
+void ExportNodeHolder::Bind(SceneUpdateContext& context,
+                            mozart::client::ContainerNode& container,
+                            const SkPoint& offset,
+                            bool hit_testable) {
+  ASSERT_IS_GPU_THREAD;
+  export_node_->Bind(context, container, offset, hit_testable);
+}
+
+ExportNodeHolder::~ExportNodeHolder() {
+  ASSERT_IS_UI_THREAD;
+  blink::Threads::Gpu()->PostTask(
+      ftl::MakeCopyable([export_node = std::move(export_node_)]() {
+        export_node->Dispose(true);
+      }));
+}
+
 ExportNode::ExportNode(ftl::RefPtr<fidl::dart::Handle> export_token_handle)
     : export_token_(export_token_handle->ReleaseHandle()) {}
 
 ExportNode::~ExportNode() {
-  // TODO(MZ-190): Ensure the node is properly unregistered on the rasterizer
-  // thread by attaching it to the update context in some way.
+  // Ensure that we properly released the node.
+  FTL_DCHECK(!node_);
+  FTL_DCHECK(scene_update_context_ == nullptr);
 }
 
 void ExportNode::Bind(SceneUpdateContext& context,
                       mozart::client::ContainerNode& container,
                       const SkPoint& offset,
                       bool hit_testable) {
-  ftl::MutexLocker lock(&mutex_);
+  ASSERT_IS_GPU_THREAD;
 
   if (export_token_) {
+    // Happens first time we bind.
     node_.reset(new mozart::client::EntityNode(container.session()));
     node_->Export(std::move(export_token_));
+
+    // Add ourselves to the context so it can call Dispose() on us if the Mozart
+    // session is closed.
+    context.AddExportNode(this);
+    scene_update_context_ = &context;
   }
 
   if (node_) {
@@ -34,4 +66,24 @@ void ExportNode::Bind(SceneUpdateContext& context,
   }
 }
 
+void ExportNode::Dispose(bool remove_from_scene_update_context) {
+  ASSERT_IS_GPU_THREAD;
+
+  // If scene_update_context_ is set, then we should still have a node left to
+  // dereference.
+  // If scene_update_context_ is null, then either:
+  // 1. A node was never created, or
+  // 2. A node was created but was already dereferenced (i.e. Dispose has
+  // already been called).
+  FTL_DCHECK(scene_update_context_ || !node_);
+
+  if (remove_from_scene_update_context && scene_update_context_) {
+    scene_update_context_->RemoveExportNode(this);
+  }
+
+  scene_update_context_ = nullptr;
+  export_token_.reset();
+  node_ = nullptr;
+}
+
 }  // namespace flow

flow/export_node.h

@@ -21,31 +21,59 @@
 
 namespace flow {
 
+// Wrapper class for ExportNode to use on UI Thread. When ExportNodeHolder is
+// destroyed, a task is posted on the Rasterizer thread to dispose the resources
+// held by the ExportNode.
+class ExportNodeHolder : public ftl::RefCountedThreadSafe<ExportNodeHolder> {
+ public:
+  ExportNodeHolder(ftl::RefPtr<fidl::dart::Handle> export_token_handle);
+  ~ExportNodeHolder();
+
+  // Calls Bind() on the wrapped ExportNode.
+  void Bind(SceneUpdateContext& context,
+            mozart::client::ContainerNode& container,
+            const SkPoint& offset,
+            bool hit_testable);
+
+  ExportNode* export_node() { return export_node_.get(); }
+
+ private:
+  std::unique_ptr<ExportNode> export_node_;
+
+  FRIEND_MAKE_REF_COUNTED(ExportNodeHolder);
+  FRIEND_REF_COUNTED_THREAD_SAFE(ExportNodeHolder);
+  FTL_DISALLOW_COPY_AND_ASSIGN(ExportNodeHolder);
+};
+
 // Represents a node which is being exported from the session.
 // This object is created on the UI thread but the entity node it contains
 // must be created and destroyed by the rasterizer thread.
-//
-// Therefore this object is thread-safe.
-class ExportNode : public ftl::RefCountedThreadSafe<ExportNode> {
+class ExportNode {
  public:
   ExportNode(ftl::RefPtr<fidl::dart::Handle> export_token_handle);
 
+  ~ExportNode();
+
   // Binds the export token to the entity node and adds it as a child of
-  // the specified container.
+  // the specified container. Must be called on the Rasterizer thread.
   void Bind(SceneUpdateContext& context,
             mozart::client::ContainerNode& container,
             const SkPoint& offset,
             bool hit_testable);
 
  private:
-  ~ExportNode();
+  friend class SceneUpdateContext;
+  friend class ExportNodeHolder;
+
+  // Cleans up resources held and removes this ExportNode from
+  // SceneUpdateContext. Must be called on the Rasterizer thread.
+  void Dispose(bool remove_from_scene_update_context);
 
-  ftl::Mutex mutex_;
-  mx::eventpair export_token_ FTL_GUARDED_BY(mutex_);
-  std::unique_ptr<mozart::client::EntityNode> node_ FTL_GUARDED_BY(mutex_);
+  // Member variables can only be read or modified on Rasterizer thread.
+  SceneUpdateContext* scene_update_context_ = nullptr;
+  mx::eventpair export_token_;
+  std::unique_ptr<mozart::client::EntityNode> node_;
 
-  FRIEND_MAKE_REF_COUNTED(ExportNode);
-  FRIEND_REF_COUNTED_THREAD_SAFE(ExportNode);
   FTL_DISALLOW_COPY_AND_ASSIGN(ExportNode);
 };
 

flow/layers/child_scene_layer.cc

@@ -26,8 +26,9 @@ void ChildSceneLayer::UpdateScene(SceneUpdateContext& context) {
   // or whether we should leave this up to the Flutter application to decide.
   // In some situations, it might be useful to allow children to draw
   // outside of their layout bounds.
-  if (export_node_) {
-    context.AddChildScene(export_node_.get(), offset_, hit_testable_);
+  if (export_node_holder_) {
+    context.AddChildScene(export_node_holder_->export_node(), offset_,
+                          hit_testable_);
   }
 }
 

flow/layers/child_scene_layer.h

@@ -10,6 +10,7 @@
 
 namespace flow {
 
+// Layer that represents an embedded child.
 class ChildSceneLayer : public Layer {
  public:
   ChildSceneLayer();
@@ -19,8 +20,9 @@ class ChildSceneLayer : public Layer {
 
   void set_size(const SkSize& size) { size_ = size; }
 
-  void set_export_node(ftl::RefPtr<ExportNode> export_node) {
-    export_node_ = std::move(export_node);
+  void set_export_node_holder(
+      ftl::RefPtr<ExportNodeHolder> export_node_holder) {
+    export_node_holder_ = std::move(export_node_holder);
   }
 
   void set_hit_testable(bool hit_testable) { hit_testable_ = hit_testable; }
@@ -34,7 +36,7 @@ class ChildSceneLayer : public Layer {
  private:
   SkPoint offset_;
   SkSize size_;
-  ftl::RefPtr<ExportNode> export_node_;
+  ftl::RefPtr<ExportNodeHolder> export_node_holder_;
   bool hit_testable_ = true;
 
   FTL_DISALLOW_COPY_AND_ASSIGN(ChildSceneLayer);

flow/layers/layer.h

@@ -35,6 +35,8 @@ namespace flow {
 
 class ContainerLayer;
 
+// Represents a single composited layer. Created on the UI thread but then
+// subquently used on the Rasterizer thread.
 class Layer {
  public:
   Layer();

flow/scene_update_context.cc

@@ -4,6 +4,7 @@
 
 #include "flutter/flow/scene_update_context.h"
 
+#include "flutter/common/threads.h"
 #include "flutter/flow/export_node.h"
 #include "flutter/flow/layers/layer.h"
 #include "flutter/flow/matrix_decomposition.h"
@@ -17,16 +18,36 @@ SceneUpdateContext::SceneUpdateContext(mozart::client::Session* session,
   FTL_DCHECK(surface_producer_ != nullptr);
 }
 
-SceneUpdateContext::~SceneUpdateContext() = default;
+SceneUpdateContext::~SceneUpdateContext() {
+  ASSERT_IS_GPU_THREAD;
+
+  // Release Mozart session resources for all ExportNodes.
+  for (auto export_node : export_nodes_) {
+    export_node->Dispose(false);
+  }
+};
 
 void SceneUpdateContext::AddChildScene(ExportNode* export_node,
                                        SkPoint offset,
                                        bool hit_testable) {
+  ASSERT_IS_GPU_THREAD;
   FTL_DCHECK(top_entity_);
 
   export_node->Bind(*this, top_entity_->entity_node(), offset, hit_testable);
 }
 
+void SceneUpdateContext::AddExportNode(ExportNode* export_node) {
+  ASSERT_IS_GPU_THREAD;
+
+  export_nodes_.insert(export_node);  // Might already have been added.
+}
+
+void SceneUpdateContext::RemoveExportNode(ExportNode* export_node) {
+  ASSERT_IS_GPU_THREAD;
+
+  export_nodes_.erase(export_node);
+}
+
 void SceneUpdateContext::CreateFrame(mozart::client::EntityNode& entity_node,
                                      const SkRRect& rrect,
                                      SkColor color,

flow/scene_update_context.h

@@ -19,6 +19,7 @@
 namespace flow {
 
 class Layer;
+class ExportNodeHolder;
 class ExportNode;
 
 class SceneUpdateContext {
@@ -123,6 +124,14 @@ class SceneUpdateContext {
                      SkPoint offset,
                      bool hit_testable);
 
+  // Adds reference to |export_node| so we can call export_node->Dispose() in
+  // our destructor. Caller is responsible for calling RemoveExportNode() before
+  // |export_node| is destroyed.
+  void AddExportNode(ExportNode* export_node);
+
+  // Removes reference to |export_node|.
+  void RemoveExportNode(ExportNode* export_node);
+
   // TODO(chinmaygarde): This method must submit the surfaces as soon as paint
   // tasks are done. However, given that there is no support currently for
   // Vulkan semaphores, we need to submit all the surfaces after an explicit
@@ -174,6 +183,9 @@ class SceneUpdateContext {
 
   std::vector<PaintTask> paint_tasks_;
 
+  // Save ExportNodes so we can dispose them in our destructor.
+  std::set<ExportNode*> export_nodes_;
+
   FTL_DISALLOW_COPY_AND_ASSIGN(SceneUpdateContext);
 };
 

lib/ui/compositing/scene_builder.cc

@@ -239,7 +239,7 @@ void SceneBuilder::addChildScene(double dx,
   std::unique_ptr<flow::ChildSceneLayer> layer(new flow::ChildSceneLayer());
   layer->set_offset(SkPoint::Make(dx, dy));
   layer->set_size(SkSize::Make(width, height));
-  layer->set_export_node(sceneHost->exportNode());
+  layer->set_export_node_holder(sceneHost->export_node_holder());
   layer->set_hit_testable(hitTestable);
   m_currentLayer->Add(std::move(layer));
 #endif

lib/ui/compositing/scene_host.cc

@@ -37,7 +37,8 @@ ftl::RefPtr<SceneHost> SceneHost::create(
 }
 
 SceneHost::SceneHost(ftl::RefPtr<fidl::dart::Handle> export_token_handle) {
-  export_node_ = ftl::MakeRefCounted<flow::ExportNode>(export_token_handle);
+  export_node_holder_ =
+      ftl::MakeRefCounted<flow::ExportNodeHolder>(export_token_handle);
 }
 #else
 ftl::RefPtr<SceneHost> SceneHost::create(Dart_Handle export_token_handle) {

lib/ui/compositing/scene_host.h

@@ -35,8 +35,8 @@ class SceneHost : public ftl::RefCountedThreadSafe<SceneHost>,
   ~SceneHost() override;
 
 #if defined(OS_FUCHSIA)
-  const ftl::RefPtr<flow::ExportNode>& exportNode() const {
-    return export_node_;
+  const ftl::RefPtr<flow::ExportNodeHolder>& export_node_holder() const {
+    return export_node_holder_;
   }
 #endif
 
@@ -46,7 +46,7 @@ class SceneHost : public ftl::RefCountedThreadSafe<SceneHost>,
 
  private:
 #if defined(OS_FUCHSIA)
-  ftl::RefPtr<flow::ExportNode> export_node_;
+  ftl::RefPtr<flow::ExportNodeHolder> export_node_holder_;
 #endif
 
 #if defined(OS_FUCHSIA)