Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix crash about 'SkiaUnrefQueue::Drain' is called after 'IOManager' reset #32106

Merged
fluttergithubbot merged 6 commits into from
Mar 21, 2022
Merged

Fix crash about 'SkiaUnrefQueue::Drain' is called after 'IOManager' reset #32106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f2c763b
Fix crash about 'SkiaUnrefQueue::Drain' is called after 'IOManager' r…
ColdPaleLight Mar 18, 2022
50e7cb0
disable shell_io_manager_unittests on fuchsia
ColdPaleLight Mar 18, 2022
4dcd599
Add some comments
ColdPaleLight Mar 18, 2022
4a2ea80
The resource 'GrDirectContext' should be destructed the IO thread
ColdPaleLight Mar 19, 2022
44be8cd
Add a new test to verify that the resource context is destroyed in th…
ColdPaleLight Mar 20, 2022
ace9be6
remove unnecessary 'mutable' in test
ColdPaleLight Mar 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 0 additions & 1 deletion flow/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
52 changes: 0 additions & 52 deletions flow/skia_gpu_object.cc
View file
Open in desktop

This file was deleted.

72 changes: 61 additions & 11 deletions flow/skia_gpu_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,46 +11,96 @@
#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"

namespace flutter {

// A queue that holds Skia objects that must be destructed on the given task
// runner.
class SkiaUnrefQueue : public fml::RefCountedThreadSafe<SkiaUnrefQueue> {
template <class T>
class UnrefQueue : public fml::RefCountedThreadSafe<UnrefQueue<T>> {
public:
void Unref(SkRefCnt* object);
using ResourceContext = T;

void Unref(SkRefCnt* object) {
Copy link
Contributor

@dnfield dnfield Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these back to their own implementation file?

Copy link
Member Author

@ColdPaleLight ColdPaleLight Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test TEST_F(SkiaGpuObjectTest, UnrefResourceContextInTaskRunnerThread) in this commit 44be8cd to make sure the context is released in the io thread, but in order to write this test , I had to change UnrefQueue to a template class. So need to move all the code into the header file. I have no idea how to change my new test if the code is removed back.

Copy link
Contributor

@dnfield dnfield Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe my lack of C++ knowledge is showing, but shouldn't we be able to define the template methods in the cpp file?

Copy link
Member Author

@ColdPaleLight ColdPaleLight Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the template is defined in the implementation file, the test file cannot included it, so the compiler cannot help us generate the class UnrefQueue<TestResourceContext>, so the test code cannot be compiled.

If we really need to hide the implementation, we can consider introducing a skia_gpu_object_impl.h and put the implementation code in that file. WDYT?

Copy link
Contributor

@dnfield dnfield Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave this as is in that case

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<SkRefCnt*> skia_objects;
{
std::scoped_lock lock(mutex_);
objects_.swap(skia_objects);
drain_pending_ = false;
}
DoDrain(skia_objects, context_);
}

void UpdateResourceContext(sk_sp<ResourceContext> context) {
context_ = context;
}

private:
const fml::RefPtr<fml::TaskRunner> task_runner_;
const fml::TimeDelta drain_delay_;
std::mutex mutex_;
std::deque<SkRefCnt*> objects_;
bool drain_pending_;
fml::WeakPtr<GrDirectContext> context_;
sk_sp<ResourceContext> 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<fml::TaskRunner> task_runner,
fml::TimeDelta delay,
fml::WeakPtr<GrDirectContext> context = {});
UnrefQueue(fml::RefPtr<fml::TaskRunner> task_runner,
fml::TimeDelta delay,
sk_sp<ResourceContext> 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<SkRefCnt*>& skia_objects,
sk_sp<ResourceContext> 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<GrDirectContext>;

/// 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.
Expand Down
33 changes: 32 additions & 1 deletion flow/skia_gpu_object_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -34,6 +34,15 @@ class TestSkObject : public SkRefCnt {
fml::TaskQueueId* dtor_task_queue_id_;
};

class TestResourceContext : public TestSkObject {
public:
TestResourceContext(std::shared_ptr<fml::AutoResetWaitableEvent> 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()
Expand Down Expand Up @@ -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<fml::AutoResetWaitableEvent> latch =
std::make_shared<fml::AutoResetWaitableEvent>();
fml::RefPtr<UnrefQueue<TestResourceContext>> unref_queue;
fml::TaskQueueId dtor_task_queue_id(0);
unref_task_runner()->PostTask([&]() {
auto resource_context =
sk_make_sp<TestResourceContext>(latch, &dtor_task_queue_id);
unref_queue = fml::MakeRefCounted<UnrefQueue<TestResourceContext>>(
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
8 changes: 7 additions & 1 deletion shell/common/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down Expand Up @@ -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" ]
}
}
}
Binary file added shell/common/fixtures/hello_loop_2.gif
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions shell/common/fixtures/shell_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
8 changes: 5 additions & 3 deletions shell/common/shell_io_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ sk_sp<GrDirectContext> ShellIOManager::CreateCompatibleResourceLoadingContext(
ShellIOManager::ShellIOManager(
sk_sp<GrDirectContext> resource_context,
std::shared_ptr<const fml::SyncSwitch> is_gpu_disabled_sync_switch,
fml::RefPtr<fml::TaskRunner> unref_queue_task_runner)
fml::RefPtr<fml::TaskRunner> unref_queue_task_runner,
fml::TimeDelta unref_queue_drain_delay)
: resource_context_(std::move(resource_context)),
resource_context_weak_factory_(
resource_context_
Expand All @@ -43,8 +44,8 @@ ShellIOManager::ShellIOManager(
: nullptr),
unref_queue_(fml::MakeRefCounted<flutter::SkiaUnrefQueue>(
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_) {
Expand Down Expand Up @@ -81,6 +82,7 @@ void ShellIOManager::UpdateResourceContext(
? std::make_unique<fml::WeakPtrFactory<GrDirectContext>>(
resource_context_.get())
: nullptr;
unref_queue_->UpdateResourceContext(resource_context_);
}

fml::WeakPtr<ShellIOManager> ShellIOManager::GetWeakPtr() {
Expand Down
4 changes: 3 additions & 1 deletion shell/common/shell_io_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ class ShellIOManager final : public IOManager {
ShellIOManager(
sk_sp<GrDirectContext> resource_context,
std::shared_ptr<const fml::SyncSwitch> is_gpu_disabled_sync_switch,
fml::RefPtr<fml::TaskRunner> unref_queue_task_runner);
fml::RefPtr<fml::TaskRunner> unref_queue_task_runner,
fml::TimeDelta unref_queue_drain_delay =
fml::TimeDelta::FromMilliseconds(8));
Copy link

@hyiso hyiso Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ColdPaleLight I'm wondering whether number 8 here has any potential meaning?
I'm facing a problem when Supporting another platform because the delayed task posted in UnrefQueue#Unref in release mode (In debug mode, everything goes as expected)
When I change 8 to 0, it goes as expected in release mode.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ColdPaleLight I'm wondering whether number 8 here has any potential meaning? I'm facing a problem when Supporting another platform because the delayed task posted in UnrefQueue#Unref in release mode (In debug mode, everything goes as expected) When I change 8 to 0, it goes as expected in release mode.

I guess you can find the answer in the PR #9486. My commit did not change the value of delay, it just moved it to another place.


~ShellIOManager() override;

Expand Down
Loading