Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b340b734 authored by Kevin DuBois's avatar Kevin DuBois
Browse files

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
parent c8cdfef8

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp

+7 −3
Original line number Original line Diff line number Diff line
@@ -338,10 +338,14 @@ CancelResult VSyncDispatchTimerQueue::cancel(CallbackToken token) { 
    }

    }

    auto& callback = it->second;

    auto& callback = it->second;





    if (callback->wakeupTime()) {

    auto const wakeupTime = callback->wakeupTime();

    if (wakeupTime) {

        callback->disarm();

        callback->disarm();



        if (*wakeupTime == mIntendedWakeupTime) {

            mIntendedWakeupTime = kInvalidTime;

            mIntendedWakeupTime = kInvalidTime;

            rearmTimer(mTimeKeeper->now());

            rearmTimer(mTimeKeeper->now());

        }

        return CancelResult::Cancelled;

        return CancelResult::Cancelled;

    }

    }

    return CancelResult::TooLate;

    return CancelResult::TooLate;

services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp

+46 −0
Original line number Original line Diff line number Diff line
@@ -701,6 +701,52 @@ TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminent 
    EXPECT_THAT(cb.mCalls.size(), Eq(1));

    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 {

class VSyncDispatchTimerQueueEntryTest : public testing::Test {

protected:

protected:

    nsecs_t const mPeriod = 1000;

    nsecs_t const mPeriod = 1000;