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

Commit 6f1cd95

Browse files
0xZOne0xZOne
committed
Fix platformview deadlock.
1 parent 03657e0 commit 6f1cd95

File tree

6 files changed

+123
-18
lines changed

6 files changed

+123
-18
lines changed

‎fml/raster_thread_merger.cc‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,20 +50,36 @@ RasterThreadMerger::CreateOrShareThreadMerger(
5050
}
5151

5252
void RasterThreadMerger::MergeWithLease(size_t lease_term) {
53-
std::scoped_lock lock(mutex_);
5453
if (TaskQueuesAreSame()) {
5554
return;
5655
}
57-
if (!IsEnabledUnSafe()) {
56+
if (!IsEnabled()) {
5857
return;
5958
}
59+
6060
FML_DCHECK(lease_term > 0) << "lease_term should be positive.";
61+
{
62+
std::scoped_lock lock(mutex_);
63+
if (IsMergedUnSafe()) {
64+
merged_condition_.notify_one();
65+
return;
66+
}
67+
}
6168

62-
if (IsMergedUnSafe()) {
63-
merged_condition_.notify_one();
64-
return;
69+
// Here we flush all expired tasks in the raster task queue before thread
70+
// merging to ensure these sync tasks the platform thread post has been
71+
// executed.
72+
// TODO(0xZOne): Maybe we should only flush expired sync tasks posted by the
73+
// platform thread.
74+
if (MessageLoop::IsInitializedForCurrentThread()) {
75+
if (!IsOnPlatformThread()) {
76+
MessageLoop::GetCurrent().RunExpiredTasksNow();
77+
}
6578
}
6679

80+
// To avoid data races between the platform thread posting sync tasks to the
81+
// raster thread and the raster thread merging, we add a lock here.
82+
std::scoped_lock lock{mutex_, gThreadMergingLock};
6783
bool success = shared_merger_->MergeWithLease(this, lease_term);
6884
if (success && merge_unmerge_callback_ != nullptr) {
6985
merge_unmerge_callback_();

‎fml/raster_thread_merger_unittests.cc‎

Lines changed: 81 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -413,14 +413,15 @@ TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) {
413413
raster_thread_merger->MergeWithLease(1);
414414
post_merge.CountDown();
415415
});
416-
417-
loop_raster->GetTaskRunner()->PostTask([&]() {
418-
ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread());
419-
ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread());
420-
ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_platform);
421-
raster_thread_merger->DecrementLease();
422-
post_merge.CountDown();
423-
});
416+
loop_raster->GetTaskRunner()->PostDelayedTask(
417+
[&]() {
418+
ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread());
419+
ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread());
420+
ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_platform);
421+
raster_thread_merger->DecrementLease();
422+
post_merge.CountDown();
423+
},
424+
fml::TimeDelta::FromMilliseconds(100));
424425

425426
loop_raster->RunExpiredTasksNow();
426427
post_merge.Wait();
@@ -657,5 +658,77 @@ TEST(RasterThreadMerger,
657658
ASSERT_FALSE(merger_from_3_to_1->IsMerged());
658659
}
659660

661+
TEST(RasterThreadMerger, FlushAllExpiredTasksBeforeThreadMerging) {
662+
fml::MessageLoop* loop_platform = nullptr;
663+
fml::AutoResetWaitableEvent latch1;
664+
std::thread thread_platform([&loop_platform, &latch1]() {
665+
fml::MessageLoop::EnsureInitializedForCurrentThread();
666+
loop_platform = &fml::MessageLoop::GetCurrent();
667+
loop_platform->GetTaskRunner()->PostTask([&]() { latch1.Signal(); });
668+
loop_platform->Run();
669+
});
670+
671+
fml::MessageLoop* loop_raster = nullptr;
672+
fml::AutoResetWaitableEvent latch2;
673+
std::thread thread_raster([&loop_raster, &loop_platform, &latch1, &latch2]() {
674+
latch1.Wait();
675+
676+
fml::MessageLoop::EnsureInitializedForCurrentThread();
677+
loop_raster = &fml::MessageLoop::GetCurrent();
678+
fml::TaskQueueId qid_platform =
679+
loop_platform->GetTaskRunner()->GetTaskQueueId();
680+
fml::TaskQueueId qid_raster =
681+
loop_raster->GetTaskRunner()->GetTaskQueueId();
682+
fml::CountDownLatch post_merge(4);
683+
const auto raster_thread_merger =
684+
fml::MakeRefCounted<fml::RasterThreadMerger>(qid_platform, qid_raster);
685+
686+
int order = 0;
687+
loop_raster->GetTaskRunner()->PostTask([&]() {
688+
ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread());
689+
ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread());
690+
ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_raster);
691+
raster_thread_merger->MergeWithLease(1);
692+
ASSERT_EQ(3, ++order);
693+
post_merge.CountDown();
694+
});
695+
696+
loop_raster->GetTaskRunner()->PostTask([&]() {
697+
ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread());
698+
ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread());
699+
ASSERT_EQ(1, ++order);
700+
post_merge.CountDown();
701+
});
702+
703+
loop_raster->GetTaskRunner()->PostTask([&]() {
704+
ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread());
705+
ASSERT_FALSE(raster_thread_merger->IsOnPlatformThread());
706+
ASSERT_EQ(2, ++order);
707+
post_merge.CountDown();
708+
});
709+
710+
loop_raster->GetTaskRunner()->PostDelayedTask(
711+
[&]() {
712+
ASSERT_TRUE(raster_thread_merger->IsOnRasterizingThread());
713+
ASSERT_TRUE(raster_thread_merger->IsOnPlatformThread());
714+
ASSERT_EQ(fml::MessageLoop::GetCurrentTaskQueueId(), qid_platform);
715+
ASSERT_EQ(4, ++order);
716+
raster_thread_merger->DecrementLease();
717+
post_merge.CountDown();
718+
},
719+
fml::TimeDelta::FromMilliseconds(100));
720+
721+
loop_raster->RunExpiredTasksNow();
722+
post_merge.Wait();
723+
latch2.Signal();
724+
});
725+
726+
latch2.Wait();
727+
loop_platform->GetTaskRunner()->PostTask(
728+
[&]() { loop_platform->Terminate(); });
729+
730+
thread_platform.join();
731+
thread_raster.join();
732+
}
660733
} // namespace testing
661734
} // namespace fml

‎fml/task_runner.cc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
namespace fml {
1818

19+
std::mutex gThreadMergingLock;
20+
1921
TaskRunner::TaskRunner(fml::RefPtr<MessageLoopImpl> loop)
2022
: loop_(std::move(loop)) {}
2123

@@ -62,4 +64,9 @@ void TaskRunner::RunNowOrPostTask(fml::RefPtr<fml::TaskRunner> runner,
6264
}
6365
}
6466

67+
void TaskRunner::RunNowOrPostSyncTask(fml::RefPtr<fml::TaskRunner> runner,
68+
const fml::closure& task) {
69+
std::scoped_lock lock(gThreadMergingLock);
70+
RunNowOrPostTask(runner, task);
71+
}
6572
} // namespace fml

‎fml/task_runner.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ namespace fml {
1616

1717
class MessageLoopImpl;
1818

19+
/// To avoid data races between the platform thread posting sync tasks to the
20+
/// raster thread and the raster thread merging.
21+
extern std::mutex gThreadMergingLock;
22+
1923
/// An interface over the ability to schedule tasks on a \p TaskRunner.
2024
class BasicTaskRunner {
2125
public:
@@ -62,6 +66,11 @@ class TaskRunner : public fml::RefCountedThreadSafe<TaskRunner>,
6266
static void RunNowOrPostTask(fml::RefPtr<fml::TaskRunner> runner,
6367
const fml::closure& task);
6468

69+
/// Similar to |RunNowOrPostTask|, but it needs to synchronize with thread
70+
/// merging operation.
71+
static void RunNowOrPostSyncTask(fml::RefPtr<fml::TaskRunner> runner,
72+
const fml::closure& task);
73+
6574
protected:
6675
explicit TaskRunner(fml::RefPtr<MessageLoopImpl> loop);
6776

‎shell/common/platform_view.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ void PlatformView::NotifyCreated() {
6464
// so that the platform view is not collected till the surface is obtained.
6565
auto* platform_view = this;
6666
fml::ManualResetWaitableEvent latch;
67-
fml::TaskRunner::RunNowOrPostTask(
67+
fml::TaskRunner::RunNowOrPostSyncTask(
6868
task_runners_.GetRasterTaskRunner(), [platform_view, &surface, &latch]() {
6969
surface = platform_view->CreateRenderingSurface();
7070
if (surface && !surface->IsValid()) {

‎shell/platform/android/platform_view_android.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ void PlatformViewAndroid::NotifyCreated(
9898
InstallFirstFrameCallback();
9999

100100
fml::AutoResetWaitableEvent latch;
101-
fml::TaskRunner::RunNowOrPostTask(
101+
fml::TaskRunner::RunNowOrPostSyncTask(
102102
task_runners_.GetRasterTaskRunner(),
103103
[&latch, surface = android_surface_.get(),
104104
native_window = std::move(native_window)]() {
@@ -115,7 +115,7 @@ void PlatformViewAndroid::NotifySurfaceWindowChanged(
115115
fml::RefPtr<AndroidNativeWindow> native_window) {
116116
if (android_surface_) {
117117
fml::AutoResetWaitableEvent latch;
118-
fml::TaskRunner::RunNowOrPostTask(
118+
fml::TaskRunner::RunNowOrPostSyncTask(
119119
task_runners_.GetRasterTaskRunner(),
120120
[&latch, surface = android_surface_.get(),
121121
native_window = std::move(native_window)]() {
@@ -132,7 +132,7 @@ void PlatformViewAndroid::NotifyDestroyed() {
132132

133133
if (android_surface_) {
134134
fml::AutoResetWaitableEvent latch;
135-
fml::TaskRunner::RunNowOrPostTask(
135+
fml::TaskRunner::RunNowOrPostSyncTask(
136136
task_runners_.GetRasterTaskRunner(),
137137
[&latch, surface = android_surface_.get()]() {
138138
surface->TeardownOnScreenContext();
@@ -147,7 +147,7 @@ void PlatformViewAndroid::NotifyChanged(const SkISize& size) {
147147
return;
148148
}
149149
fml::AutoResetWaitableEvent latch;
150-
fml::TaskRunner::RunNowOrPostTask(
150+
fml::TaskRunner::RunNowOrPostSyncTask(
151151
task_runners_.GetRasterTaskRunner(), //
152152
[&latch, surface = android_surface_.get(), size]() {
153153
surface->OnScreenSurfaceResize(size);

0 commit comments

Comments
 (0)