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

Commit 6ddd261d authored by Ana Krulec's avatar Ana Krulec
Browse files

Fixing threading around mConnections in Scheduler 

The following happens when we create connection
1. initScheduler()
2. getFactory().createScheduler()
3. mTouchTimer.emplace
4. mScheduler->createConnection
Which means that timer created in step 3 expires exactly
while we are in step 4, so then the EventThread is half baked
in rare cases and we crash.

Also, do not continue to calling functions if the
optional fields do not have a value.

Test: libsurfaceflinger_unittest --gtest_filter=SchedulerTest.testDispatchCachedReportedConfig
Bug: 160926398
Change-Id: Ib2617b914145bc4180cc7ca27203c59dbd625c94
parent c6900fa3

services/surfaceflinger/Scheduler/Scheduler.cpp

+57 −12
Original line number Diff line number Diff line
@@ -229,6 +229,7 @@ Scheduler::ConnectionHandle Scheduler::createConnection(std::unique_ptr<EventThr 
    auto connection =

            createConnectionInternal(eventThread.get(), ISurfaceComposer::eConfigChangedSuppress);



    std::lock_guard<std::mutex> lock(mConnectionsLock);

    mConnections.emplace(handle, Connection{connection, std::move(eventThread)});

    return handle;

}
@@ -240,29 +241,47 @@ sp<EventThreadConnection> Scheduler::createConnectionInternal( 


sp<IDisplayEventConnection> Scheduler::createDisplayEventConnection(

        ConnectionHandle handle, ISurfaceComposer::ConfigChanged configChanged) {

    std::lock_guard<std::mutex> lock(mConnectionsLock);

    RETURN_IF_INVALID_HANDLE(handle, nullptr);

    return createConnectionInternal(mConnections[handle].thread.get(), configChanged);

}



sp<EventThreadConnection> Scheduler::getEventConnection(ConnectionHandle handle) {

    std::lock_guard<std::mutex> lock(mConnectionsLock);

    RETURN_IF_INVALID_HANDLE(handle, nullptr);

    return mConnections[handle].connection;

}



void Scheduler::onHotplugReceived(ConnectionHandle handle, PhysicalDisplayId displayId,

                                  bool connected) {

    android::EventThread* thread;

    {

        std::lock_guard<std::mutex> lock(mConnectionsLock);

        RETURN_IF_INVALID_HANDLE(handle);

    mConnections[handle].thread->onHotplugReceived(displayId, connected);

        thread = mConnections[handle].thread.get();

    }



    thread->onHotplugReceived(displayId, connected);

}



void Scheduler::onScreenAcquired(ConnectionHandle handle) {

    android::EventThread* thread;

    {

        std::lock_guard<std::mutex> lock(mConnectionsLock);

        RETURN_IF_INVALID_HANDLE(handle);

    mConnections[handle].thread->onScreenAcquired();

        thread = mConnections[handle].thread.get();

    }

    thread->onScreenAcquired();

}



void Scheduler::onScreenReleased(ConnectionHandle handle) {

    android::EventThread* thread;

    {

        std::lock_guard<std::mutex> lock(mConnectionsLock);

        RETURN_IF_INVALID_HANDLE(handle);

    mConnections[handle].thread->onScreenReleased();

        thread = mConnections[handle].thread.get();

    }

    thread->onScreenReleased();

}



void Scheduler::onPrimaryDisplayConfigChanged(ConnectionHandle handle, PhysicalDisplayId displayId,
@@ -274,6 +293,16 @@ void Scheduler::onPrimaryDisplayConfigChanged(ConnectionHandle handle, PhysicalD 
}



void Scheduler::dispatchCachedReportedConfig() {

    // Check optional fields first.

    if (!mFeatures.configId.has_value()) {

        ALOGW("No config ID found, not dispatching cached config.");

        return;

    }

    if (!mFeatures.cachedConfigChangedParams.has_value()) {

        ALOGW("No config changed params found, not dispatching cached config.");

        return;

    }



    const auto configId = *mFeatures.configId;

    const auto vsyncPeriod =

            mRefreshRateConfigs.getRefreshRateFromConfigId(configId).getVsyncPeriod();
@@ -295,24 +324,40 @@ void Scheduler::dispatchCachedReportedConfig() { 
void Scheduler::onNonPrimaryDisplayConfigChanged(ConnectionHandle handle,

                                                 PhysicalDisplayId displayId,

                                                 HwcConfigIndexType configId, nsecs_t vsyncPeriod) {

    android::EventThread* thread;

    {

        std::lock_guard<std::mutex> lock(mConnectionsLock);

        RETURN_IF_INVALID_HANDLE(handle);

    mConnections[handle].thread->onConfigChanged(displayId, configId, vsyncPeriod);

        thread = mConnections[handle].thread.get();

    }

    thread->onConfigChanged(displayId, configId, vsyncPeriod);

}



size_t Scheduler::getEventThreadConnectionCount(ConnectionHandle handle) {

    std::lock_guard<std::mutex> lock(mConnectionsLock);

    RETURN_IF_INVALID_HANDLE(handle, 0);

    return mConnections[handle].thread->getEventThreadConnectionCount();

}



void Scheduler::dump(ConnectionHandle handle, std::string& result) const {

    android::EventThread* thread;

    {

        std::lock_guard<std::mutex> lock(mConnectionsLock);

        RETURN_IF_INVALID_HANDLE(handle);

    mConnections.at(handle).thread->dump(result);

        thread = mConnections.at(handle).thread.get();

    }

    thread->dump(result);

}



void Scheduler::setDuration(ConnectionHandle handle, std::chrono::nanoseconds workDuration,

                            std::chrono::nanoseconds readyDuration) {

    android::EventThread* thread;

    {

        std::lock_guard<std::mutex> lock(mConnectionsLock);

        RETURN_IF_INVALID_HANDLE(handle);

    mConnections[handle].thread->setDuration(workDuration, readyDuration);

        thread = mConnections[handle].thread.get();

    }

    thread->setDuration(workDuration, readyDuration);

}



void Scheduler::getDisplayStatInfo(DisplayStatInfo* stats, nsecs_t now) {

services/surfaceflinger/Scheduler/Scheduler.h

+2 −1
Original line number Diff line number Diff line
@@ -228,7 +228,8 @@ private: 
    };



    ConnectionHandle::Id mNextConnectionHandleId = 0;

    std::unordered_map<ConnectionHandle, Connection> mConnections;

    mutable std::mutex mConnectionsLock;

    std::unordered_map<ConnectionHandle, Connection> mConnections GUARDED_BY(mConnectionsLock);



    bool mInjectVSyncs = false;

    InjectVSyncSource* mVSyncInjector = nullptr;

services/surfaceflinger/tests/unittests/SchedulerTest.cpp

+21 −0
Original line number Diff line number Diff line
@@ -175,4 +175,25 @@ TEST_F(SchedulerTest, noLayerHistory) { 
    mScheduler.chooseRefreshRateForContent();

}



TEST_F(SchedulerTest, testDispatchCachedReportedConfig) {

    // If the optional fields are cleared, the function should return before

    // onConfigChange is called.

    mScheduler.clearOptionalFieldsInFeatures();

    EXPECT_NO_FATAL_FAILURE(mScheduler.dispatchCachedReportedConfig());

    EXPECT_CALL(*mEventThread, onConfigChanged(_, _, _)).Times(0);

}



TEST_F(SchedulerTest, onNonPrimaryDisplayConfigChanged_invalidParameters) {

    HwcConfigIndexType configId = HwcConfigIndexType(111);

    nsecs_t vsyncPeriod = 111111;



    // If the handle is incorrect, the function should return before

    // onConfigChange is called.

    Scheduler::ConnectionHandle invalidHandle = {.id = 123};

    EXPECT_NO_FATAL_FAILURE(mScheduler.onNonPrimaryDisplayConfigChanged(invalidHandle,

                                                                        PHYSICAL_DISPLAY_ID,

                                                                        configId, vsyncPeriod));

    EXPECT_CALL(*mEventThread, onConfigChanged(_, _, _)).Times(0);

}



} // namespace android

services/surfaceflinger/tests/unittests/TestableScheduler.h

+17 −0
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


#pragma once



#include <Scheduler/Scheduler.h>

#include <gmock/gmock.h>

#include <gui/ISurfaceComposer.h>
@@ -93,6 +94,22 @@ public: 
        return mFeatures.touch == Scheduler::TouchState::Active;

    }



    void dispatchCachedReportedConfig() {

        std::lock_guard<std::mutex> lock(mFeatureStateLock);

        return Scheduler::dispatchCachedReportedConfig();

    }



    void clearOptionalFieldsInFeatures() {

        std::lock_guard<std::mutex> lock(mFeatureStateLock);

        mFeatures.cachedConfigChangedParams.reset();

    }



    void onNonPrimaryDisplayConfigChanged(ConnectionHandle handle, PhysicalDisplayId displayId,

                                          HwcConfigIndexType configId, nsecs_t vsyncPeriod) {

        return Scheduler::onNonPrimaryDisplayConfigChanged(handle, displayId, configId,

                                                           vsyncPeriod);

    }



    ~TestableScheduler() {

        // All these pointer and container clears help ensure that GMock does

        // not report a leaked object, since the Scheduler instance may