diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 87c6208ebcec6..89cadedbc7d91 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -170,7 +170,6 @@ FILE: ../../../flutter/flow/raster_cache_unittests.cc FILE: ../../../flutter/flow/rtree.cc FILE: ../../../flutter/flow/rtree.h FILE: ../../../flutter/flow/rtree_unittests.cc -FILE: ../../../flutter/flow/skia_gpu_object.cc FILE: ../../../flutter/flow/skia_gpu_object.h FILE: ../../../flutter/flow/skia_gpu_object_unittests.cc FILE: ../../../flutter/flow/surface.cc @@ -1050,6 +1049,7 @@ FILE: ../../../flutter/shell/common/display_manager.h FILE: ../../../flutter/shell/common/engine.cc FILE: ../../../flutter/shell/common/engine.h FILE: ../../../flutter/shell/common/engine_unittests.cc +FILE: ../../../flutter/shell/common/fixtures/hello_loop_2.gif FILE: ../../../flutter/shell/common/fixtures/shell_test.dart FILE: ../../../flutter/shell/common/fixtures/shelltest_screenshot.png FILE: ../../../flutter/shell/common/input_events_unittests.cc @@ -1075,6 +1075,7 @@ FILE: ../../../flutter/shell/common/shell_benchmarks.cc FILE: ../../../flutter/shell/common/shell_fuchsia_unittests.cc FILE: ../../../flutter/shell/common/shell_io_manager.cc FILE: ../../../flutter/shell/common/shell_io_manager.h +FILE: ../../../flutter/shell/common/shell_io_manager_unittests.cc FILE: ../../../flutter/shell/common/shell_test.cc FILE: ../../../flutter/shell/common/shell_test.h FILE: ../../../flutter/shell/common/shell_test_external_view_embedder.cc diff --git a/flow/BUILD.gn b/flow/BUILD.gn index def9e48d62c76..57164dfb13085 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -64,7 +64,6 @@ source_set("flow") { "raster_cache_key.h", "rtree.cc", "rtree.h", - "skia_gpu_object.cc", "skia_gpu_object.h", "surface.cc", "surface.h", diff --git a/flow/skia_gpu_object.cc b/flow/skia_gpu_object.cc deleted file mode 100644 index 7415916d77954..0000000000000 --- a/flow/skia_gpu_object.cc +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "flutter/flow/skia_gpu_object.h" - -#include "flutter/fml/message_loop.h" -#include "flutter/fml/trace_event.h" - -namespace flutter { - -SkiaUnrefQueue::SkiaUnrefQueue(fml::RefPtr task_runner, - fml::TimeDelta delay, - fml::WeakPtr context) - : task_runner_(std::move(task_runner)), - drain_delay_(delay), - drain_pending_(false), - context_(context) {} - -SkiaUnrefQueue::~SkiaUnrefQueue() { - FML_DCHECK(objects_.empty()); -} - -void SkiaUnrefQueue::Unref(SkRefCnt* object) { - std::scoped_lock lock(mutex_); - objects_.push_back(object); - if (!drain_pending_) { - drain_pending_ = true; - task_runner_->PostDelayedTask( - [strong = fml::Ref(this)]() { strong->Drain(); }, drain_delay_); - } -} - -void SkiaUnrefQueue::Drain() { - TRACE_EVENT0("flutter", "SkiaUnrefQueue::Drain"); - std::deque skia_objects; - { - std::scoped_lock lock(mutex_); - objects_.swap(skia_objects); - drain_pending_ = false; - } - - for (SkRefCnt* skia_object : skia_objects) { - skia_object->unref(); - } - - if (context_ && skia_objects.size() > 0) { - context_->performDeferredCleanup(std::chrono::milliseconds(0)); - } -} - -} // namespace flutter diff --git a/flow/skia_gpu_object.h b/flow/skia_gpu_object.h index 38d29dcacdaf5..7eb067d7dd48e 100644 --- a/flow/skia_gpu_object.h +++ b/flow/skia_gpu_object.h @@ -11,6 +11,7 @@ #include "flutter/fml/memory/ref_counted.h" #include "flutter/fml/memory/weak_ptr.h" #include "flutter/fml/task_runner.h" +#include "flutter/fml/trace_event.h" #include "third_party/skia/include/core/SkRefCnt.h" #include "third_party/skia/include/gpu/GrDirectContext.h" @@ -18,16 +19,40 @@ namespace flutter { // A queue that holds Skia objects that must be destructed on the given task // runner. -class SkiaUnrefQueue : public fml::RefCountedThreadSafe { +template +class UnrefQueue : public fml::RefCountedThreadSafe> { public: - void Unref(SkRefCnt* object); + using ResourceContext = T; + + void Unref(SkRefCnt* object) { + std::scoped_lock lock(mutex_); + objects_.push_back(object); + if (!drain_pending_) { + drain_pending_ = true; + task_runner_->PostDelayedTask( + [strong = fml::Ref(this)]() { strong->Drain(); }, drain_delay_); + } + } // Usually, the drain is called automatically. However, during IO manager // shutdown (when the platform side reference to the OpenGL context is about // to go away), we may need to pre-emptively drain the unref queue. It is the // responsibility of the caller to ensure that no further unrefs are queued // after this call. - void Drain(); + void Drain() { + TRACE_EVENT0("flutter", "SkiaUnrefQueue::Drain"); + std::deque skia_objects; + { + std::scoped_lock lock(mutex_); + objects_.swap(skia_objects); + drain_pending_ = false; + } + DoDrain(skia_objects, context_); + } + + void UpdateResourceContext(sk_sp context) { + context_ = context; + } private: const fml::RefPtr task_runner_; @@ -35,22 +60,47 @@ class SkiaUnrefQueue : public fml::RefCountedThreadSafe { std::mutex mutex_; std::deque objects_; bool drain_pending_; - fml::WeakPtr context_; + sk_sp context_; // The `GrDirectContext* context` is only used for signaling Skia to // performDeferredCleanup. It can be nullptr when such signaling is not needed // (e.g., in unit tests). - SkiaUnrefQueue(fml::RefPtr task_runner, - fml::TimeDelta delay, - fml::WeakPtr context = {}); + UnrefQueue(fml::RefPtr task_runner, + fml::TimeDelta delay, + sk_sp context = nullptr) + : task_runner_(std::move(task_runner)), + drain_delay_(delay), + drain_pending_(false), + context_(context) {} + + ~UnrefQueue() { + fml::TaskRunner::RunNowOrPostTask( + task_runner_, [objects = std::move(objects_), + context = std::move(context_)]() mutable { + DoDrain(objects, context); + context.reset(); + }); + } + + // static + static void DoDrain(const std::deque& skia_objects, + sk_sp context) { + for (SkRefCnt* skia_object : skia_objects) { + skia_object->unref(); + } - ~SkiaUnrefQueue(); + if (context && skia_objects.size() > 0) { + context->performDeferredCleanup(std::chrono::milliseconds(0)); + } + } - FML_FRIEND_REF_COUNTED_THREAD_SAFE(SkiaUnrefQueue); - FML_FRIEND_MAKE_REF_COUNTED(SkiaUnrefQueue); - FML_DISALLOW_COPY_AND_ASSIGN(SkiaUnrefQueue); + FML_FRIEND_REF_COUNTED_THREAD_SAFE(UnrefQueue); + FML_FRIEND_MAKE_REF_COUNTED(UnrefQueue); + FML_DISALLOW_COPY_AND_ASSIGN(UnrefQueue); }; +using SkiaUnrefQueue = UnrefQueue; + /// An object whose deallocation needs to be performed on an specific unref /// queue. The template argument U need to have a call operator that returns /// that unref queue. diff --git a/flow/skia_gpu_object_unittests.cc b/flow/skia_gpu_object_unittests.cc index faf0f3acec0ab..cda91dd3f6e2b 100644 --- a/flow/skia_gpu_object_unittests.cc +++ b/flow/skia_gpu_object_unittests.cc @@ -22,7 +22,7 @@ class TestSkObject : public SkRefCnt { fml::TaskQueueId* dtor_task_queue_id) : latch_(latch), dtor_task_queue_id_(dtor_task_queue_id) {} - ~TestSkObject() { + virtual ~TestSkObject() { if (dtor_task_queue_id_) { *dtor_task_queue_id_ = fml::MessageLoop::GetCurrentTaskQueueId(); } @@ -34,6 +34,15 @@ class TestSkObject : public SkRefCnt { fml::TaskQueueId* dtor_task_queue_id_; }; +class TestResourceContext : public TestSkObject { + public: + TestResourceContext(std::shared_ptr latch, + fml::TaskQueueId* dtor_task_queue_id) + : TestSkObject(latch, dtor_task_queue_id) {} + ~TestResourceContext() = default; + void performDeferredCleanup(std::chrono::milliseconds msNotUsed) {} +}; + class SkiaGpuObjectTest : public ThreadTest { public: SkiaGpuObjectTest() @@ -127,5 +136,27 @@ TEST_F(SkiaGpuObjectTest, ObjectResetTwice) { ASSERT_EQ(dtor_task_queue_id, unref_task_runner()->GetTaskQueueId()); } +TEST_F(SkiaGpuObjectTest, UnrefResourceContextInTaskRunnerThread) { + std::shared_ptr latch = + std::make_shared(); + fml::RefPtr> unref_queue; + fml::TaskQueueId dtor_task_queue_id(0); + unref_task_runner()->PostTask([&]() { + auto resource_context = + sk_make_sp(latch, &dtor_task_queue_id); + unref_queue = fml::MakeRefCounted>( + unref_task_runner(), fml::TimeDelta::FromSeconds(0), resource_context); + latch->Signal(); + }); + latch->Wait(); + + // Delete the unref queue, it will schedule a task to unref the resource + // context in the task runner's thread. + unref_queue = nullptr; + latch->Wait(); + // Verify that the resource context was destroyed in the task runner's thread. + ASSERT_EQ(dtor_task_queue_id, unref_task_runner()->GetTaskQueueId()); +} + } // namespace testing } // namespace flutter diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index 50a2881ec0877..d3bc759817a40 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -159,7 +159,10 @@ if (enable_unittests) { test_fixtures("shell_unittests_fixtures") { dart_main = "fixtures/shell_test.dart" - fixtures = [ "fixtures/shelltest_screenshot.png" ] + fixtures = [ + "fixtures/shelltest_screenshot.png", + "fixtures/hello_loop_2.gif", + ] } shell_host_executable("shell_benchmarks") { @@ -293,6 +296,9 @@ if (enable_unittests) { "$fuchsia_sdk_root/pkg:fidl_cpp", "$fuchsia_sdk_root/pkg:sys_cpp", ] + } else { + # TODO(63837): This test is hard-coded to use a TestGLSurface so it cannot run on fuchsia. + sources += [ "shell_io_manager_unittests.cc" ] } } } diff --git a/shell/common/fixtures/hello_loop_2.gif b/shell/common/fixtures/hello_loop_2.gif new file mode 100644 index 0000000000000..e9ac36d917d22 Binary files /dev/null and b/shell/common/fixtures/hello_loop_2.gif differ diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 85c406291c942..e2e9c2f08a0fd 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -270,3 +270,10 @@ void onBeginFrameWithNotifyNativeMain() { }; notifyNative(); } + +@pragma('vm:entry-point') +void frameCallback(_Image, int) { + // It is used as the frame callback of 'MultiFrameCodec' in the test + // 'ItDoesNotCrashThatSkiaUnrefQueueDrainAfterIOManagerReset'. + // The test is a regression test and doesn't care about images, so it is empty. +} diff --git a/shell/common/shell_io_manager.cc b/shell/common/shell_io_manager.cc index 115b5efaf7c4d..38ab99d7bce77 100644 --- a/shell/common/shell_io_manager.cc +++ b/shell/common/shell_io_manager.cc @@ -34,7 +34,8 @@ sk_sp ShellIOManager::CreateCompatibleResourceLoadingContext( ShellIOManager::ShellIOManager( sk_sp resource_context, std::shared_ptr is_gpu_disabled_sync_switch, - fml::RefPtr unref_queue_task_runner) + fml::RefPtr unref_queue_task_runner, + fml::TimeDelta unref_queue_drain_delay) : resource_context_(std::move(resource_context)), resource_context_weak_factory_( resource_context_ @@ -43,8 +44,8 @@ ShellIOManager::ShellIOManager( : nullptr), unref_queue_(fml::MakeRefCounted( std::move(unref_queue_task_runner), - fml::TimeDelta::FromMilliseconds(8), - GetResourceContext())), + unref_queue_drain_delay, + resource_context_)), is_gpu_disabled_sync_switch_(is_gpu_disabled_sync_switch), weak_factory_(this) { if (!resource_context_) { @@ -81,6 +82,7 @@ void ShellIOManager::UpdateResourceContext( ? std::make_unique>( resource_context_.get()) : nullptr; + unref_queue_->UpdateResourceContext(resource_context_); } fml::WeakPtr ShellIOManager::GetWeakPtr() { diff --git a/shell/common/shell_io_manager.h b/shell/common/shell_io_manager.h index a6b0b5f137ed7..5f59960ebafd1 100644 --- a/shell/common/shell_io_manager.h +++ b/shell/common/shell_io_manager.h @@ -27,7 +27,9 @@ class ShellIOManager final : public IOManager { ShellIOManager( sk_sp resource_context, std::shared_ptr is_gpu_disabled_sync_switch, - fml::RefPtr unref_queue_task_runner); + fml::RefPtr unref_queue_task_runner, + fml::TimeDelta unref_queue_drain_delay = + fml::TimeDelta::FromMilliseconds(8)); ~ShellIOManager() override; diff --git a/shell/common/shell_io_manager_unittests.cc b/shell/common/shell_io_manager_unittests.cc new file mode 100644 index 0000000000000..2387e0b7e2764 --- /dev/null +++ b/shell/common/shell_io_manager_unittests.cc @@ -0,0 +1,135 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/shell/common/shell_io_manager.h" + +#include "flutter/common/task_runners.h" +#include "flutter/fml/mapping.h" +#include "flutter/lib/ui/painting/multi_frame_codec.h" +#include "flutter/testing/dart_isolate_runner.h" +#include "flutter/testing/fixture_test.h" +#include "flutter/testing/post_task_sync.h" +#include "flutter/testing/test_gl_surface.h" +#include "flutter/testing/testing.h" + +namespace flutter { +namespace testing { + +static sk_sp OpenFixtureAsSkData(const char* name) { + auto fixtures_directory = + fml::OpenDirectory(GetFixturesPath(), false, fml::FilePermission::kRead); + if (!fixtures_directory.is_valid()) { + return nullptr; + } + + auto fixture_mapping = + fml::FileMapping::CreateReadOnly(fixtures_directory, name); + + if (!fixture_mapping) { + return nullptr; + } + + SkData::ReleaseProc on_release = [](const void* ptr, void* context) -> void { + delete reinterpret_cast(context); + }; + + auto data = SkData::MakeWithProc(fixture_mapping->GetMapping(), + fixture_mapping->GetSize(), on_release, + fixture_mapping.get()); + + if (!data) { + return nullptr; + } + // The data is now owned by Skia. + fixture_mapping.release(); + return data; +} + +class ShellIOManagerTest : public FixtureTest {}; + +// Regression test for https://github.com/flutter/engine/pull/32106. +TEST_F(ShellIOManagerTest, + ItDoesNotCrashThatSkiaUnrefQueueDrainAfterIOManagerReset) { + auto settings = CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(settings); + auto vm_data = vm_ref.GetVMData(); + auto gif_mapping = OpenFixtureAsSkData("hello_loop_2.gif"); + ASSERT_TRUE(gif_mapping); + + ImageGeneratorRegistry registry; + std::shared_ptr gif_generator = + registry.CreateCompatibleGenerator(gif_mapping); + ASSERT_TRUE(gif_generator); + + TaskRunners runners(GetCurrentTestName(), // label + CreateNewThread("platform"), // platform + CreateNewThread("raster"), // raster + CreateNewThread("ui"), // ui + CreateNewThread("io") // io + ); + + std::unique_ptr gl_surface; + std::unique_ptr io_manager; + fml::RefPtr codec; + + // Setup the IO manager. + PostTaskSync(runners.GetIOTaskRunner(), [&]() { + gl_surface = std::make_unique(SkISize::Make(1, 1)); + io_manager = std::make_unique( + gl_surface->CreateGrContext(), std::make_shared(), + runners.GetIOTaskRunner(), fml::TimeDelta::FromMilliseconds(0)); + }); + + auto isolate = RunDartCodeInIsolate(vm_ref, settings, runners, "emptyMain", + {}, GetDefaultKernelFilePath(), + io_manager->GetWeakIOManager()); + + PostTaskSync(runners.GetUITaskRunner(), [&]() { + fml::AutoResetWaitableEvent isolate_latch; + + EXPECT_TRUE(isolate->RunInIsolateScope([&]() -> bool { + Dart_Handle library = Dart_RootLibrary(); + if (Dart_IsError(library)) { + isolate_latch.Signal(); + return false; + } + Dart_Handle closure = + Dart_GetField(library, Dart_NewStringFromCString("frameCallback")); + if (Dart_IsError(closure) || !Dart_IsClosure(closure)) { + isolate_latch.Signal(); + return false; + } + + codec = fml::MakeRefCounted(std::move(gif_generator)); + codec->getNextFrame(closure); + isolate_latch.Signal(); + return true; + })); + isolate_latch.Wait(); + }); + + // Destroy the IO manager + PostTaskSync(runners.GetIOTaskRunner(), [&]() { + // 'SkiaUnrefQueue.Drain' will be called after 'io_manager.reset()' in this + // test, If the resource context has been destroyed at that time, it will + // crash. + // + // 'Drain()' currently checks whether the weak pointer is still valid or not + // before trying to call anything on it. + // + // However, calling 'unref' on the 'SkImage_Lazy' ends up freeing a + // 'GrBackendTexture'. That object seems to assume that something else is + // keeping the context alive. This seems like it might be a bad assumption + // on Skia's part, but in Skia's defense we're doing something pretty weird + // here by keeping GPU resident objects alive without keeping the + // 'GrDirectContext' alive ourselves. + // + // See https://github.com/flutter/flutter/issues/87895 + io_manager.reset(); + gl_surface.reset(); + }); +} + +} // namespace testing +} // namespace flutter