From 3e245ae8127fce99f00923bd24aecd1a233d9b0a Mon Sep 17 00:00:00 2001 From: mikejurka Date: Wed, 23 Aug 2017 17:32:19 -0700 Subject: [PATCH] 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 | 58 +++++++++++++++++++++++++++-- flow/export_node.h | 48 +++++++++++++++++++----- flow/layers/child_scene_layer.cc | 5 ++- flow/layers/child_scene_layer.h | 8 ++-- flow/layers/layer.h | 2 + flow/scene_update_context.cc | 23 +++++++++++- flow/scene_update_context.h | 12 ++++++ lib/ui/compositing/scene_builder.cc | 2 +- lib/ui/compositing/scene_host.cc | 3 +- lib/ui/compositing/scene_host.h | 6 +-- 10 files changed, 143 insertions(+), 24 deletions(-) diff --git a/flow/export_node.cc b/flow/export_node.cc index 5a2ab94fe..055de7e59 100644 --- a/flow/export_node.cc +++ b/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 export_token_handle) + : export_node_(std::make_unique(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 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 diff --git a/flow/export_node.h b/flow/export_node.h index 31bfaf7d3..0056732e4 100644 --- a/flow/export_node.h +++ b/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 { + public: + ExportNodeHolder(ftl::RefPtr 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 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 { +class ExportNode { public: ExportNode(ftl::RefPtr 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 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 node_; - FRIEND_MAKE_REF_COUNTED(ExportNode); - FRIEND_REF_COUNTED_THREAD_SAFE(ExportNode); FTL_DISALLOW_COPY_AND_ASSIGN(ExportNode); }; diff --git a/flow/layers/child_scene_layer.cc b/flow/layers/child_scene_layer.cc index b64875d76..37fbd16e4 100644 --- a/flow/layers/child_scene_layer.cc +++ b/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_); } } diff --git a/flow/layers/child_scene_layer.h b/flow/layers/child_scene_layer.h index 3ae15fa69..dbca89359 100644 --- a/flow/layers/child_scene_layer.h +++ b/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 export_node) { - export_node_ = std::move(export_node); + void set_export_node_holder( + ftl::RefPtr 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 export_node_; + ftl::RefPtr export_node_holder_; bool hit_testable_ = true; FTL_DISALLOW_COPY_AND_ASSIGN(ChildSceneLayer); diff --git a/flow/layers/layer.h b/flow/layers/layer.h index 7bcecd40c..e374f5e9f 100644 --- a/flow/layers/layer.h +++ b/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(); diff --git a/flow/scene_update_context.cc b/flow/scene_update_context.cc index 1a741e19a..5264db36e 100644 --- a/flow/scene_update_context.cc +++ b/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, diff --git a/flow/scene_update_context.h b/flow/scene_update_context.h index bfcd78540..0606c0e25 100644 --- a/flow/scene_update_context.h +++ b/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 paint_tasks_; + // Save ExportNodes so we can dispose them in our destructor. + std::set export_nodes_; + FTL_DISALLOW_COPY_AND_ASSIGN(SceneUpdateContext); }; diff --git a/lib/ui/compositing/scene_builder.cc b/lib/ui/compositing/scene_builder.cc index b5961c8bf..00ba21a08 100644 --- a/lib/ui/compositing/scene_builder.cc +++ b/lib/ui/compositing/scene_builder.cc @@ -239,7 +239,7 @@ void SceneBuilder::addChildScene(double dx, std::unique_ptr 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 diff --git a/lib/ui/compositing/scene_host.cc b/lib/ui/compositing/scene_host.cc index b40d23ab0..886514571 100644 --- a/lib/ui/compositing/scene_host.cc +++ b/lib/ui/compositing/scene_host.cc @@ -37,7 +37,8 @@ ftl::RefPtr SceneHost::create( } SceneHost::SceneHost(ftl::RefPtr export_token_handle) { - export_node_ = ftl::MakeRefCounted(export_token_handle); + export_node_holder_ = + ftl::MakeRefCounted(export_token_handle); } #else ftl::RefPtr SceneHost::create(Dart_Handle export_token_handle) { diff --git a/lib/ui/compositing/scene_host.h b/lib/ui/compositing/scene_host.h index f2962a392..a738aba7d 100644 --- a/lib/ui/compositing/scene_host.h +++ b/lib/ui/compositing/scene_host.h @@ -35,8 +35,8 @@ class SceneHost : public ftl::RefCountedThreadSafe, ~SceneHost() override; #if defined(OS_FUCHSIA) - const ftl::RefPtr& exportNode() const { - return export_node_; + const ftl::RefPtr& export_node_holder() const { + return export_node_holder_; } #endif @@ -46,7 +46,7 @@ class SceneHost : public ftl::RefCountedThreadSafe, private: #if defined(OS_FUCHSIA) - ftl::RefPtr export_node_; + ftl::RefPtr export_node_holder_; #endif #if defined(OS_FUCHSIA) -- GitLab