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

Commit 166663a1 authored by Atneya Nair's avatar Atneya Nair
Browse files

Prevent audioflinger battery notes leak 

Ensure battery notes from audioflinger are encapsulated in an RAII
object so noteStart is always properly matched.

Remove uid refcounting in audioserver, since it already exists in
the battery notifier utility class, and is error-prone. As a byproduct,
notes occur during track transitions rather than in the threadloop,
which happens to avoid problematic blocking.

Bump refcount mismatch logging to error.

Bug: 259449389
Test: Manual verification of battery stats notes w/ multiple refs
Change-Id: Ifc931a9ac46f37d662e42dd4696f0a62eec22c25
parent 8f83345c

media/utils/BatteryNotifier.cpp

+2 −2
Original line number Diff line number Diff line
@@ -85,8 +85,8 @@ void BatteryNotifier::noteStartAudio(uid_t uid) { 


void BatteryNotifier::noteStopAudio(uid_t uid) {

    Mutex::Autolock _l(mLock);

    if (mAudioRefCounts.find(uid) == mAudioRefCounts.end()) {

        ALOGW("%s: audio refcount is broken for uid(%d).", __FUNCTION__, (int)uid);

    if (mAudioRefCounts.find(uid) == mAudioRefCounts.end() || (mAudioRefCounts[uid] == 0)) {

        ALOGE("%s: audio refcount is broken for uid(%d).", __FUNCTION__, (int)uid);

        return;

    }

media/utils/include/mediautils/BatteryNotifier.h

+32 −0
Original line number Diff line number Diff line
@@ -68,6 +68,38 @@ private: 
    sp<IBatteryStats> getBatteryService_l();

};



namespace mediautils {

class BatteryStatsAudioHandle {

  public:

    static constexpr uid_t INVALID_UID = static_cast<uid_t>(-1);



    explicit BatteryStatsAudioHandle(uid_t uid) : mUid(uid) {

        if (uid != INVALID_UID) {

            BatteryNotifier::getInstance().noteStartAudio(mUid);

        }

    }



    BatteryStatsAudioHandle(BatteryStatsAudioHandle&& other) : mUid(other.mUid) {

        other.mUid = INVALID_UID;

    }



    BatteryStatsAudioHandle(const BatteryStatsAudioHandle& other) = delete;



    BatteryStatsAudioHandle& operator=(const BatteryStatsAudioHandle& other) = delete;



    BatteryStatsAudioHandle& operator=(BatteryStatsAudioHandle&& other) = delete;



    ~BatteryStatsAudioHandle() {

        if (mUid != INVALID_UID) {

            BatteryNotifier::getInstance().noteStopAudio(mUid);

        }

    }



  private:

    // Logically const

    uid_t mUid = INVALID_UID;

};

}  // namespace mediautils

}  // namespace android



#endif // MEDIA_BATTERY_NOTIFIER_H

services/audioflinger/AudioFlinger.h

+1 −0
Original line number Diff line number Diff line
@@ -74,6 +74,7 @@ 
#include <media/DeviceDescriptorBase.h>

#include <media/ExtendedAudioBufferProvider.h>

#include <media/VolumeShaper.h>

#include <mediautils/BatteryNotifier.h>

#include <mediautils/ServiceUtilities.h>

#include <mediautils/SharedMemoryAllocator.h>

#include <mediautils/Synchronization.h>

services/audioflinger/Threads.cpp

+3 −25
Original line number Diff line number Diff line
@@ -1877,7 +1877,7 @@ ssize_t AudioFlinger::ThreadBase::ActiveTracks<T>::add(const sp<T> &track) { 
    logTrack("add", track);

    mActiveTracksGeneration++;

    mLatestActiveTrack = track;

    ++mBatteryCounter[track->uid()].second;

    track->beginBatteryAttribution();

    mHasChanged = true;

    return mActiveTracks.add(track);

}
@@ -1891,7 +1891,7 @@ ssize_t AudioFlinger::ThreadBase::ActiveTracks<T>::remove(const sp<T> &track) { 
    }

    logTrack("remove", track);

    mActiveTracksGeneration++;

    --mBatteryCounter[track->uid()].second;

    track->endBatteryAttribution();

    // mLatestActiveTrack is not cleared even if is the same as track.

    mHasChanged = true;

#ifdef TEE_SINK
@@ -1904,14 +1904,13 @@ ssize_t AudioFlinger::ThreadBase::ActiveTracks<T>::remove(const sp<T> &track) { 
template <typename T>

void AudioFlinger::ThreadBase::ActiveTracks<T>::clear() {

    for (const sp<T> &track : mActiveTracks) {

        BatteryNotifier::getInstance().noteStopAudio(track->uid());

        track->endBatteryAttribution();

        logTrack("clear", track);

    }

    mLastActiveTracksGeneration = mActiveTracksGeneration;

    if (!mActiveTracks.empty()) { mHasChanged = true; }

    mActiveTracks.clear();

    mLatestActiveTrack.clear();

    mBatteryCounter.clear();

}



template <typename T>
@@ -1922,27 +1921,6 @@ void AudioFlinger::ThreadBase::ActiveTracks<T>::updatePowerState( 
        thread->updateWakeLockUids_l(getWakeLockUids());

        mLastActiveTracksGeneration = mActiveTracksGeneration;

    }



    // Updates BatteryNotifier uids

    for (auto it = mBatteryCounter.begin(); it != mBatteryCounter.end();) {

        const uid_t uid = it->first;

        ssize_t &previous = it->second.first;

        ssize_t &current = it->second.second;

        if (current > 0) {

            if (previous == 0) {

                BatteryNotifier::getInstance().noteStartAudio(uid);

            }

            previous = current;

            ++it;

        } else if (current == 0) {

            if (previous > 0) {

                BatteryNotifier::getInstance().noteStopAudio(uid);

            }

            it = mBatteryCounter.erase(it); // std::map<> is stable on iterator erase.

        } else /* (current < 0) */ {

            LOG_ALWAYS_FATAL("negative battery count %zd", current);

        }

    }

}



template <typename T>

services/audioflinger/Threads.h

+0 −2
Original line number Diff line number Diff line
@@ -826,8 +826,6 @@ protected: 
                        return wakeLockUids; // moved by underlying SharedBuffer

                    }



                    std::map<uid_t, std::pair<ssize_t /* previous */, ssize_t /* current */>>

                                        mBatteryCounter;

                    SortedVector<sp<T>> mActiveTracks;

                    int                 mActiveTracksGeneration;

                    int                 mLastActiveTracksGeneration;