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

Commit 6c440aeb authored by Leon Scroggins III's avatar Leon Scroggins III
Browse files

Make VSyncCallbackRegistration's move operator= call unregisterCallback 

Skipping unregisterCallback means that we might delete a callback while
it is being called. Note that EventThread uses the move operator=, and
this behaves differently than if it were stored in a unique_ptr like in
MessageQueue. Update move operator= so that they do behave the same, and
add tests verifying this, along with a couple other behavior tests.

Note that this may result in deadlocks similar to those in b/276367387.
That is fixed by If490f88115aa298d31aa9ad392a1f9f9dc987549.

Cleanups:
- Remove comment for VsyncCallbackRegistration regarding the lifetime
requirement for the VSyncDispatch.
Icdb80253436b4d0034fc20fcae8583efb7c30292 switched the VSyncDispatch
to an std::shared_ptr from a reference, so the client no longer needs
to ensure that the VSyncDispatch outlives the VsyncCallbackRegistration.
- Replace mValidToken with using an std::optional for the token.

Bug: 279209321
Test: VSyncCallbackRegistrationTest
Change-Id: I3c1eccb36914f29560600d48bb08b1b8f2fe7c96
parent 8bf29931

services/surfaceflinger/Scheduler/VSyncDispatch.h

+3 −6
Original line number Diff line number Diff line
@@ -155,10 +155,6 @@ protected: 
    VSyncDispatch& operator=(const VSyncDispatch&) = delete;

};



/*

 * Helper class to operate on registered callbacks. It is up to user of the class to ensure

 * that VsyncDispatch lifetime exceeds the lifetime of VSyncCallbackRegistation.

 */

class VSyncCallbackRegistration {

public:

    VSyncCallbackRegistration(std::shared_ptr<VSyncDispatch>, VSyncDispatch::Callback,
@@ -178,9 +174,10 @@ public: 
    CancelResult cancel();



private:

    friend class VSyncCallbackRegistrationTest;



    std::shared_ptr<VSyncDispatch> mDispatch;

    VSyncDispatch::CallbackToken mToken;

    bool mValidToken;

    std::optional<VSyncDispatch::CallbackToken> mToken;

};



} // namespace android::scheduler

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp

+14 −17
Original line number Diff line number Diff line
@@ -438,47 +438,44 @@ VSyncCallbackRegistration::VSyncCallbackRegistration(std::shared_ptr<VSyncDispat 
                                                     VSyncDispatch::Callback callback,

                                                     std::string callbackName)

      : mDispatch(std::move(dispatch)),

        mToken(mDispatch->registerCallback(std::move(callback), std::move(callbackName))),

        mValidToken(true) {}

        mToken(mDispatch->registerCallback(std::move(callback), std::move(callbackName))) {}



VSyncCallbackRegistration::VSyncCallbackRegistration(VSyncCallbackRegistration&& other)

      : mDispatch(std::move(other.mDispatch)),

        mToken(std::move(other.mToken)),

        mValidToken(std::move(other.mValidToken)) {

    other.mValidToken = false;

}

      : mDispatch(std::move(other.mDispatch)), mToken(std::exchange(other.mToken, std::nullopt)) {}



VSyncCallbackRegistration& VSyncCallbackRegistration::operator=(VSyncCallbackRegistration&& other) {

    if (this == &other) return *this;

    if (mToken) {

        mDispatch->unregisterCallback(*mToken);

    }

    mDispatch = std::move(other.mDispatch);

    mToken = std::move(other.mToken);

    mValidToken = std::move(other.mValidToken);

    other.mValidToken = false;

    mToken = std::exchange(other.mToken, std::nullopt);

    return *this;

}



VSyncCallbackRegistration::~VSyncCallbackRegistration() {

    if (mValidToken) mDispatch->unregisterCallback(mToken);

    if (mToken) mDispatch->unregisterCallback(*mToken);

}



ScheduleResult VSyncCallbackRegistration::schedule(VSyncDispatch::ScheduleTiming scheduleTiming) {

    if (!mValidToken) {

    if (!mToken) {

        return std::nullopt;

    }

    return mDispatch->schedule(mToken, scheduleTiming);

    return mDispatch->schedule(*mToken, scheduleTiming);

}



ScheduleResult VSyncCallbackRegistration::update(VSyncDispatch::ScheduleTiming scheduleTiming) {

    if (!mValidToken) {

    if (!mToken) {

        return std::nullopt;

    }

    return mDispatch->update(mToken, scheduleTiming);

    return mDispatch->update(*mToken, scheduleTiming);

}



CancelResult VSyncCallbackRegistration::cancel() {

    if (!mValidToken) {

    if (!mToken) {

        return CancelResult::Error;

    }

    return mDispatch->cancel(mToken);

    return mDispatch->cancel(*mToken);

}



} // namespace android::scheduler

services/surfaceflinger/tests/unittests/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -128,6 +128,7 @@ cc_test { 
        "TransactionTracingTest.cpp",

        "TunnelModeEnabledReporterTest.cpp",

        "StrongTypingTest.cpp",

        "VSyncCallbackRegistrationTest.cpp",

        "VSyncDispatchTimerQueueTest.cpp",

        "VSyncDispatchRealtimeTest.cpp",

        "VsyncModulatorTest.cpp",

services/surfaceflinger/tests/unittests/VSyncCallbackRegistrationTest.cpp

0 → 100644
+144 −0
Original line number Diff line number Diff line 
/*

 * Copyright 2023 The Android Open Source Project

 *

 * Licensed under the Apache License, Version 2.0 (the "License");

 * you may not use this file except in compliance with the License.

 * You may obtain a copy of the License at

 *

 *      http://www.apache.org/licenses/LICENSE-2.0

 *

 * Unless required by applicable law or agreed to in writing, software

 * distributed under the License is distributed on an "AS IS" BASIS,

 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

 * See the License for the specific language governing permissions and

 * limitations under the License.

 */



#undef LOG_TAG

#define LOG_TAG "LibSurfaceFlingerUnittests"



#include <gmock/gmock.h>

#include <gtest/gtest.h>



#include "Scheduler/VSyncDispatch.h"

#include "mock/MockVSyncDispatch.h"



using namespace testing;



namespace android::scheduler {



class VSyncCallbackRegistrationTest : public Test {

protected:

    VSyncDispatch::Callback mCallback = [](nsecs_t, nsecs_t, nsecs_t) {};



    std::shared_ptr<mock::VSyncDispatch> mVsyncDispatch = std::make_shared<mock::VSyncDispatch>();

    VSyncDispatch::CallbackToken mCallbackToken{7};

    std::string mCallbackName = "callback";



    std::shared_ptr<mock::VSyncDispatch> mVsyncDispatch2 = std::make_shared<mock::VSyncDispatch>();

    VSyncDispatch::CallbackToken mCallbackToken2{42};

    std::string mCallbackName2 = "callback2";



    void assertDispatch(const VSyncCallbackRegistration& registration,

                        std::shared_ptr<VSyncDispatch> dispatch) {

        ASSERT_EQ(registration.mDispatch, dispatch);

    }



    void assertToken(const VSyncCallbackRegistration& registration,

                     const std::optional<VSyncDispatch::CallbackToken>& token) {

        ASSERT_EQ(registration.mToken, token);

    }

};



TEST_F(VSyncCallbackRegistrationTest, unregistersCallbackOnDestruction) {

    // TODO (b/279581095): With ftl::Function, `_` can be replaced with

    // `mCallback`, here and in other calls to `registerCallback, since the

    // ftl version has an operator==, unlike std::function.

    EXPECT_CALL(*mVsyncDispatch, registerCallback(_, mCallbackName))

            .WillOnce(Return(mCallbackToken));

    EXPECT_CALL(*mVsyncDispatch, unregisterCallback(mCallbackToken)).Times(1);



    VSyncCallbackRegistration registration(mVsyncDispatch, mCallback, mCallbackName);

    ASSERT_NO_FATAL_FAILURE(assertDispatch(registration, mVsyncDispatch));

    ASSERT_NO_FATAL_FAILURE(assertToken(registration, mCallbackToken));

}



TEST_F(VSyncCallbackRegistrationTest, unregistersCallbackOnPointerMove) {

    {

        InSequence seq;

        EXPECT_CALL(*mVsyncDispatch, registerCallback(_, mCallbackName))

                .WillOnce(Return(mCallbackToken));

        EXPECT_CALL(*mVsyncDispatch2, registerCallback(_, mCallbackName2))

                .WillOnce(Return(mCallbackToken2));

        EXPECT_CALL(*mVsyncDispatch2, unregisterCallback(mCallbackToken2)).Times(1);

        EXPECT_CALL(*mVsyncDispatch, unregisterCallback(mCallbackToken)).Times(1);

    }



    auto registration =

            std::make_unique<VSyncCallbackRegistration>(mVsyncDispatch, mCallback, mCallbackName);



    auto registration2 =

            std::make_unique<VSyncCallbackRegistration>(mVsyncDispatch2, mCallback, mCallbackName2);



    registration2 = std::move(registration);



    ASSERT_NO_FATAL_FAILURE(assertDispatch(*registration2.get(), mVsyncDispatch));

    ASSERT_NO_FATAL_FAILURE(assertToken(*registration2.get(), mCallbackToken));

}



TEST_F(VSyncCallbackRegistrationTest, unregistersCallbackOnMoveOperator) {

    {

        InSequence seq;

        EXPECT_CALL(*mVsyncDispatch, registerCallback(_, mCallbackName))

                .WillOnce(Return(mCallbackToken));

        EXPECT_CALL(*mVsyncDispatch2, registerCallback(_, mCallbackName2))

                .WillOnce(Return(mCallbackToken2));

        EXPECT_CALL(*mVsyncDispatch2, unregisterCallback(mCallbackToken2)).Times(1);

        EXPECT_CALL(*mVsyncDispatch, unregisterCallback(mCallbackToken)).Times(1);

    }



    VSyncCallbackRegistration registration(mVsyncDispatch, mCallback, mCallbackName);



    VSyncCallbackRegistration registration2(mVsyncDispatch2, mCallback, mCallbackName2);



    registration2 = std::move(registration);



    ASSERT_NO_FATAL_FAILURE(assertDispatch(registration, nullptr));

    ASSERT_NO_FATAL_FAILURE(assertToken(registration, std::nullopt));



    ASSERT_NO_FATAL_FAILURE(assertDispatch(registration2, mVsyncDispatch));

    ASSERT_NO_FATAL_FAILURE(assertToken(registration2, mCallbackToken));

}



TEST_F(VSyncCallbackRegistrationTest, moveConstructor) {

    EXPECT_CALL(*mVsyncDispatch, registerCallback(_, mCallbackName))

            .WillOnce(Return(mCallbackToken));

    EXPECT_CALL(*mVsyncDispatch, unregisterCallback(mCallbackToken)).Times(1);



    VSyncCallbackRegistration registration(mVsyncDispatch, mCallback, mCallbackName);

    VSyncCallbackRegistration registration2(std::move(registration));



    ASSERT_NO_FATAL_FAILURE(assertDispatch(registration, nullptr));

    ASSERT_NO_FATAL_FAILURE(assertToken(registration, std::nullopt));



    ASSERT_NO_FATAL_FAILURE(assertDispatch(registration2, mVsyncDispatch));

    ASSERT_NO_FATAL_FAILURE(assertToken(registration2, mCallbackToken));

}



TEST_F(VSyncCallbackRegistrationTest, moveOperatorEqualsSelf) {

    EXPECT_CALL(*mVsyncDispatch, registerCallback(_, mCallbackName))

            .WillOnce(Return(mCallbackToken));

    EXPECT_CALL(*mVsyncDispatch, unregisterCallback(mCallbackToken)).Times(1);



    VSyncCallbackRegistration registration(mVsyncDispatch, mCallback, mCallbackName);



    // Use a reference so the compiler doesn't realize that registration is

    // being moved to itself.

    VSyncCallbackRegistration& registrationRef = registration;

    registration = std::move(registrationRef);



    ASSERT_NO_FATAL_FAILURE(assertDispatch(registration, mVsyncDispatch));

    ASSERT_NO_FATAL_FAILURE(assertToken(registration, mCallbackToken));

}



} // namespace android::scheduler