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

Commit 55a4f59f authored by Anthony Stange's avatar Anthony Stange
Browse files

Fix double release of ScopedWakelock 

Today, when a ScopedWakelock is moved, the default move constructor
doesn't unlock the old wakelock instance. This results in the moved
ScopedWakelock instance decrementing the wakelock ref count which leaves
the multi-HAL out of sync from the sensor service.

Fix this by adding a custom move constructor / operator to ensure old
state is cleared on the moved instance.

Bug: 163468874
Test: Load multi-HAL and verify that it properly waits to release the
wakelock until the sensor service notifies that it has a lock held.
Test: Run unit tests

Change-Id: Ifd5a3c7596f78d7a756c4472f30efb625d670791
parent d8f664a3

sensors/common/default/2.X/multihal/HalProxy.cpp

+4 −0
Original line number Diff line number Diff line
@@ -693,6 +693,10 @@ void HalProxy::decrementRefCountAndMaybeReleaseWakelock(size_t delta, 
                                                        int64_t timeoutStart /* = -1 */) {

    if (!mThreadsRun.load()) return;

    std::lock_guard<std::recursive_mutex> lockGuard(mWakelockMutex);

    if (delta > mWakelockRefCount) {

        ALOGE("Decrementing wakelock ref count by %zu when count is %zu",

              delta, mWakelockRefCount);

    }

    if (timeoutStart == -1) timeoutStart = mWakelockTimeoutResetTime;

    if (mWakelockRefCount == 0 || timeoutStart < mWakelockTimeoutResetTime) return;

    mWakelockRefCount -= std::min(mWakelockRefCount, delta);

sensors/common/default/2.X/multihal/ScopedWakelock.cpp

+15 −0
Original line number Diff line number Diff line
@@ -28,6 +28,21 @@ int64_t getTimeNow() { 
            .count();

}



ScopedWakelock::ScopedWakelock(ScopedWakelock&& other) {

    *this = std::move(other);

}



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

    mRefCounter = other.mRefCounter;

    mCreatedAtTimeNs = other.mCreatedAtTimeNs;

    mLocked = other.mLocked;



    other.mRefCounter = nullptr;

    other.mCreatedAtTimeNs = 0;

    other.mLocked = false;

    return *this;

}



ScopedWakelock::ScopedWakelock(IScopedWakelockRefCounter* refCounter, bool locked)

    : mRefCounter(refCounter), mLocked(locked) {

    if (mLocked) {

sensors/common/default/2.X/multihal/include/V2_0/ScopedWakelock.h

+3 −2
Original line number Diff line number Diff line
@@ -81,14 +81,15 @@ class IScopedWakelockRefCounter : public RefBase { 
 */

class ScopedWakelock {

  public:

    ScopedWakelock(ScopedWakelock&&) = default;

    ScopedWakelock& operator=(ScopedWakelock&&) = default;

    ScopedWakelock(ScopedWakelock&& other);

    ScopedWakelock& operator=(ScopedWakelock&& other);

    virtual ~ScopedWakelock();



    bool isLocked() const { return mLocked; }



  private:

    friend class HalProxyCallbackBase;

    friend class ScopedWakelockTest;

    IScopedWakelockRefCounter* mRefCounter;

    int64_t mCreatedAtTimeNs;

    bool mLocked;

sensors/common/default/2.X/multihal/tests/Android.bp

+4 −1
Original line number Diff line number Diff line
@@ -90,7 +90,10 @@ cc_test_library { 


cc_test {

    name: "android.hardware.sensors@2.X-halproxy-unit-tests",

    srcs: ["HalProxy_test.cpp"],

    srcs: [

        "HalProxy_test.cpp",

        "ScopedWakelock_test.cpp",

    ],

    vendor: true,

    header_libs: [

        "android.hardware.sensors@2.X-shared-utils",

sensors/common/default/2.X/multihal/tests/ScopedWakelock_test.cpp

0 → 100644
+110 −0
Original line number Diff line number Diff line 
//

// Copyright (C) 2020 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.



#include <gtest/gtest.h>



#include "V2_0/ScopedWakelock.h"



namespace android {

namespace hardware {

namespace sensors {

namespace V2_0 {

namespace implementation {



class RefCounter : public IScopedWakelockRefCounter {

  public:

    size_t incCount = 0;

    size_t decCount = 0;



    bool incrementRefCountAndMaybeAcquireWakelock(size_t /* delta */,

                                                  int64_t* /* timeoutStart */) override {

        incCount++;

        return true;

    }



    void decrementRefCountAndMaybeReleaseWakelock(size_t /* delta */,

                                                  int64_t /* timeoutStart */) override {

        decCount++;

    }

};



class ScopedWakelockTest : public testing::Test {

  public:

    ScopedWakelock createScopedWakelock(bool locked) {

        return ScopedWakelock(&mRefCounter, locked);

    }



    RefCounter mRefCounter;

};



TEST_F(ScopedWakelockTest, UnlockedAfterMoved) {

    ScopedWakelock wakelock = createScopedWakelock(false /* locked */);



    ScopedWakelock movedWakelock(std::move(wakelock));



    EXPECT_FALSE(wakelock.isLocked());

    EXPECT_FALSE(movedWakelock.isLocked());

}



TEST_F(ScopedWakelockTest, LockedAfterMoved) {

    ScopedWakelock wakelock = createScopedWakelock(true /* locked */);



    ScopedWakelock movedWakelock(std::move(wakelock));



    EXPECT_FALSE(wakelock.isLocked());

    EXPECT_TRUE(movedWakelock.isLocked());

}



TEST_F(ScopedWakelockTest, Locked) {

    ScopedWakelock wakelock = createScopedWakelock(true /* locked */);



    EXPECT_TRUE(wakelock.isLocked());

}



TEST_F(ScopedWakelockTest, Unlocked) {

    ScopedWakelock wakelock = createScopedWakelock(false /* locked */);



    EXPECT_FALSE(wakelock.isLocked());

}



TEST_F(ScopedWakelockTest, ScopedLocked) {

    { createScopedWakelock(true /* locked */); }



    EXPECT_EQ(mRefCounter.incCount, 1);

    EXPECT_EQ(mRefCounter.decCount, 1);

}



TEST_F(ScopedWakelockTest, ScopedUnlockIsNoop) {

    { createScopedWakelock(false /* locked */); }



    EXPECT_EQ(mRefCounter.incCount, 0);

    EXPECT_EQ(mRefCounter.decCount, 0);

}



TEST_F(ScopedWakelockTest, ScopedLockedMove) {

    {

        ScopedWakelock wakelock = createScopedWakelock(true /* locked */);

        ScopedWakelock movedWakelock(std::move(wakelock));

    }



    EXPECT_EQ(mRefCounter.incCount, 1);

    EXPECT_EQ(mRefCounter.decCount, 1);

}



}  // namespace implementation

}  // namespace V2_0

}  // namespace sensors

}  // namespace hardware

}  // namespace android
 
 No newline at end of file