From b340b734c747d69abd476f9af44848db9bc8d38e Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Tue, 16 Jun 2020 09:07:35 -0700 Subject: SF: avoid rearming Timer during cancel Averts a rare race condition by which a callback would be moved back a vsync period inappropriately by rearming the callback-dispatching timer only when the next-up callback was the one that was cancelled. This was an analagous problem (but through the cancel() path) to I0e7e2e3698ed6d1765082db20d9cf25f6e6c2db2 Test: 2 new unit tests Test: boot to home, inspect for problems Test: dogfood patch on device Test: A/B uibench sanity check Fixes: 154303580 Change-Id: I02b2ba12623ac683d9b1c592fdc35e7c7494261a --- .../Scheduler/VSyncDispatchTimerQueue.cpp | 10 +++-- .../unittests/VSyncDispatchTimerQueueTest.cpp | 46 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp index abeacfe66e..a596bce002 100644 --- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp +++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp @@ -338,10 +338,14 @@ CancelResult VSyncDispatchTimerQueue::cancel(CallbackToken token) { } auto& callback = it->second; - if (callback->wakeupTime()) { + auto const wakeupTime = callback->wakeupTime(); + if (wakeupTime) { callback->disarm(); - mIntendedWakeupTime = kInvalidTime; - rearmTimer(mTimeKeeper->now()); + + if (*wakeupTime == mIntendedWakeupTime) { + mIntendedWakeupTime = kInvalidTime; + rearmTimer(mTimeKeeper->now()); + } return CancelResult::Cancelled; } return CancelResult::TooLate; diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp index 793cb8bcca..d940dc5870 100644 --- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp +++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp @@ -701,6 +701,52 @@ TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminent EXPECT_THAT(cb.mCalls.size(), Eq(1)); } +// b/154303580. +TEST_F(VSyncDispatchTimerQueueTest, skipsRearmingWhenNotNextScheduled) { + Sequence seq; + EXPECT_CALL(mMockClock, alarmIn(_, 600)).InSequence(seq); + EXPECT_CALL(mMockClock, alarmCancel()).InSequence(seq); + CountingCallback cb1(mDispatch); + CountingCallback cb2(mDispatch); + + EXPECT_EQ(mDispatch.schedule(cb1, 400, 1000), ScheduleResult::Scheduled); + EXPECT_EQ(mDispatch.schedule(cb2, 100, 2000), ScheduleResult::Scheduled); + + mMockClock.setLag(100); + mMockClock.advanceBy(620); + + EXPECT_EQ(mDispatch.cancel(cb2), CancelResult::Cancelled); + + mMockClock.advanceBy(80); + + EXPECT_THAT(cb1.mCalls.size(), Eq(1)); + EXPECT_THAT(cb2.mCalls.size(), Eq(0)); +} + +TEST_F(VSyncDispatchTimerQueueTest, rearmsWhenCancelledAndIsNextScheduled) { + Sequence seq; + EXPECT_CALL(mMockClock, alarmIn(_, 600)).InSequence(seq); + EXPECT_CALL(mMockClock, alarmIn(_, 1280)).InSequence(seq); + EXPECT_CALL(mMockClock, alarmCancel()).InSequence(seq); + CountingCallback cb1(mDispatch); + CountingCallback cb2(mDispatch); + + EXPECT_EQ(mDispatch.schedule(cb1, 400, 1000), ScheduleResult::Scheduled); + EXPECT_EQ(mDispatch.schedule(cb2, 100, 2000), ScheduleResult::Scheduled); + + mMockClock.setLag(100); + mMockClock.advanceBy(620); + + EXPECT_EQ(mDispatch.cancel(cb1), CancelResult::Cancelled); + + EXPECT_THAT(cb1.mCalls.size(), Eq(0)); + EXPECT_THAT(cb2.mCalls.size(), Eq(0)); + mMockClock.advanceToNextCallback(); + + EXPECT_THAT(cb1.mCalls.size(), Eq(0)); + EXPECT_THAT(cb2.mCalls.size(), Eq(1)); +} + class VSyncDispatchTimerQueueEntryTest : public testing::Test { protected: nsecs_t const mPeriod = 1000; -- cgit v1.2.3-59-g8ed1b