Skip to content

Commit 4f85010

Browse files
Do not attempt to drain the SkiaUnrefQueue in the destructor (flutter#13237)
SkiaUnrefQueue should be empty at destruction time. If the queue is nonempty, then there will be a pending drain task that will hold a reference to the queue. The queue can only be destructed after the drain completes and the reference is dropped. Drains must only be done on the queue's task runner thread, which may not be the thread where the queue is destructed.
1 parent a16b0f9 commit 4f85010

File tree

5 files changed

+56
-3
lines changed

5 files changed

+56
-3
lines changed

ci/licenses_golden/licenses_flutter

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ FILE: ../../../flutter/flow/scene_update_context.cc
8080
FILE: ../../../flutter/flow/scene_update_context.h
8181
FILE: ../../../flutter/flow/skia_gpu_object.cc
8282
FILE: ../../../flutter/flow/skia_gpu_object.h
83+
FILE: ../../../flutter/flow/skia_gpu_object_unittests.cc
8384
FILE: ../../../flutter/flow/texture.cc
8485
FILE: ../../../flutter/flow/texture.h
8586
FILE: ../../../flutter/flow/texture_unittests.cc

flow/BUILD.gn

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,15 @@ executable("flow_unittests") {
114114
"matrix_decomposition_unittests.cc",
115115
"mutators_stack_unittests.cc",
116116
"raster_cache_unittests.cc",
117+
"skia_gpu_object_unittests.cc",
117118
"texture_unittests.cc",
118119
]
119120

120121
deps = [
121122
":flow",
122123
":flow_fixtures",
123124
"$flutter_root/fml",
125+
"$flutter_root/testing:testing_lib",
124126
"//third_party/dart/runtime:libdart_jit", # for tracing
125127
"//third_party/googletest:gtest",
126128
"//third_party/skia",

flow/skia_gpu_object.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ SkiaUnrefQueue::SkiaUnrefQueue(fml::RefPtr<fml::TaskRunner> task_runner,
1515
drain_pending_(false) {}
1616

1717
SkiaUnrefQueue::~SkiaUnrefQueue() {
18-
Drain();
18+
FML_DCHECK(objects_.empty());
1919
}
2020

2121
void SkiaUnrefQueue::Unref(SkRefCnt* object) {

flow/skia_gpu_object_unittests.cc

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "flutter/flow/skia_gpu_object.h"
6+
#include "flutter/fml/message_loop.h"
7+
#include "flutter/fml/synchronization/waitable_event.h"
8+
#include "flutter/fml/task_runner.h"
9+
#include "flutter/testing/thread_test.h"
10+
#include "gtest/gtest.h"
11+
#include "third_party/skia/include/core/SkRefCnt.h"
12+
13+
namespace flutter {
14+
namespace testing {
15+
16+
using SkiaGpuObjectTest = flutter::testing::ThreadTest;
17+
18+
class TestSkObject : public SkRefCnt {
19+
public:
20+
TestSkObject(std::shared_ptr<fml::AutoResetWaitableEvent> latch,
21+
fml::TaskQueueId* dtor_task_queue_id)
22+
: latch_(latch), dtor_task_queue_id_(dtor_task_queue_id) {}
23+
24+
~TestSkObject() {
25+
*dtor_task_queue_id_ = fml::MessageLoop::GetCurrentTaskQueueId();
26+
latch_->Signal();
27+
}
28+
29+
private:
30+
std::shared_ptr<fml::AutoResetWaitableEvent> latch_;
31+
fml::TaskQueueId* dtor_task_queue_id_;
32+
};
33+
34+
TEST_F(SkiaGpuObjectTest, UnrefQueue) {
35+
fml::RefPtr<fml::TaskRunner> task_runner = CreateNewThread();
36+
fml::RefPtr<SkiaUnrefQueue> queue = fml::MakeRefCounted<SkiaUnrefQueue>(
37+
task_runner, fml::TimeDelta::FromSeconds(0));
38+
39+
std::shared_ptr<fml::AutoResetWaitableEvent> latch =
40+
std::make_shared<fml::AutoResetWaitableEvent>();
41+
fml::TaskQueueId dtor_task_queue_id(0);
42+
SkRefCnt* ref_object = new TestSkObject(latch, &dtor_task_queue_id);
43+
44+
queue->Unref(ref_object);
45+
latch->Wait();
46+
ASSERT_EQ(dtor_task_queue_id, task_runner->GetTaskQueueId());
47+
}
48+
49+
} // namespace testing
50+
} // namespace flutter

testing/BUILD.gn

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ source_set("testing_lib") {
99
"$flutter_root/testing/assertions.h",
1010
"$flutter_root/testing/testing.cc",
1111
"$flutter_root/testing/testing.h",
12+
"$flutter_root/testing/thread_test.cc",
13+
"$flutter_root/testing/thread_test.h",
1214
]
1315

1416
public_deps = [
@@ -23,8 +25,6 @@ source_set("testing") {
2325

2426
sources = [
2527
"$flutter_root/testing/run_all_unittests.cc",
26-
"$flutter_root/testing/thread_test.cc",
27-
"$flutter_root/testing/thread_test.h",
2828
]
2929

3030
public_deps = [

0 commit comments

Comments
 (0)