MiPad4/android_device_xiaomi_sdm660-common
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

another potential race condition

Browse source 
LocTimer on timeout would currently delete timer delegate.
This meddles into the management of LocTimer::stop() call,
and the order of obj delete needs to be synchronized in a
few different places as a result.

This change lets the delete of the timer delegate obj fold
into the stop() handling, which would be easier to synch.

Change-Id: Ic3e0b3d183dceb9e6e2db4c47ec9d6e296b0c3f6
CRs-Fixed: 916590
This commit is contained in:
Kevin Tang 2015-09-25 15:57:45 -07:00
parent bd034a21f2
commit 9b3aa46ebb
1 changed files with 16 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

34
utils/LocTimer.cpp
View file

@ -333,13 +333,15 @@ void LocTimerContainer::remove(LocTimerDelegate& timer) {
LocMsg(), mTimerContainer(&container), mTimer(&timer) {} LocMsg(), mTimerContainer(&container), mTimer(&timer) {}
inline virtual void proc() const { inline virtual void proc() const {
LocTimerDelegate* priorTop = mTimerContainer->getSoonestTimer(); LocTimerDelegate* priorTop = mTimerContainer->getSoonestTimer();
// update soonest timer only if mTimer is actually removed from mTimerContainer
// AND mTimer is not priorTop. // update soonest timer only if mTimer is actually removed from
// mTimerContainer AND mTimer is not priorTop.
if (priorTop == ((LocHeap*)mTimerContainer)->remove((LocRankable&)*mTimer)) { if (priorTop == ((LocHeap*)mTimerContainer)->remove((LocRankable&)*mTimer)) {
// if passing in NULL, we tell updateSoonestTime to update kernel with // if passing in NULL, we tell updateSoonestTime to update
// the current top timer interval. // kernel with the current top timer interval.
mTimerContainer->updateSoonestTime(NULL); mTimerContainer->updateSoonestTime(NULL);
} }
// all timers are deleted here, and only here.
delete mTimer; delete mTimer;
} }
}; };
@ -367,7 +369,6 @@ void LocTimerContainer::expire() {
timer = mTimerContainer->popIfOutRanks(timerOfNow)) { timer = mTimerContainer->popIfOutRanks(timerOfNow)) {
// the timer delegate obj will be deleted before the return of this call // the timer delegate obj will be deleted before the return of this call
timer->expire(); timer->expire();
delete timer;
} }
mTimerContainer->updateSoonestTime(NULL); mTimerContainer->updateSoonestTime(NULL);
} }
@ -493,9 +494,10 @@ void LocTimerDelegate::destroyLocked() {
if (container) { if (container) {
container->remove(*this); container->remove(*this);
} }
} // else we do not do anything, as *this* will be deleted either } // else we do not do anything. No such *this* can be
// after it expires or stop is first called (if case above), // created and reached here with mContainer ever been
// where container->remove() will have it deleted. // a non NULL. So *this* must have reached the if clause
// once, and we want it reach there only once.
} }
int LocTimerDelegate::ranks(LocRankable& rankable) { int LocTimerDelegate::ranks(LocRankable& rankable) {
@ -511,17 +513,11 @@ int LocTimerDelegate::ranks(LocRankable& rankable) {
inline inline
void LocTimerDelegate::expire() { void LocTimerDelegate::expire() {
mLock->lock();
// keeping a copy of client pointer to be safe // keeping a copy of client pointer to be safe
// when timeOutCallback() is called at the end of this // when timeOutCallback() is called at the end of this
// method, this obj is already deleted. // method, *this* obj may be already deleted.
LocTimer* client = mClient; LocTimer* client = mClient;
// this obj is already removed from mContainer. // force a stop, which will lead to delete of this obj
// NULL it here so that dtor won't try to call remove again
mContainer = NULL;
mClient = NULL;
mLock->unlock();
// force a stop, which will force a delete of this obj
if (client && client->stop()) { if (client && client->stop()) {
// calling client callback with a pointer save on the stack // calling client callback with a pointer save on the stack
// only if stop() returns true, i.e. it hasn't been stopped // only if stop() returns true, i.e. it hasn't been stopped
@ -545,6 +541,7 @@ LocTimer::~LocTimer() {
bool LocTimer::start(unsigned int timeOutInMs, bool wakeOnExpire) { bool LocTimer::start(unsigned int timeOutInMs, bool wakeOnExpire) {
bool success = false; bool success = false;
mLock->lock();
if (!mTimer) { if (!mTimer) {
struct timespec futureTime; struct timespec futureTime;
clock_gettime(CLOCK_BOOTTIME, &futureTime); clock_gettime(CLOCK_BOOTTIME, &futureTime);
@ -558,21 +555,22 @@ bool LocTimer::start(unsigned int timeOutInMs, bool wakeOnExpire) {
// if mTimer is non 0, success should be 0; or vice versa // if mTimer is non 0, success should be 0; or vice versa
success = (NULL != mTimer); success = (NULL != mTimer);
} }
mLock->unlock();
return success; return success;
} }
bool LocTimer::stop() { bool LocTimer::stop() {
bool success = false; bool success = false;
mLock->lock();
if (mTimer) { if (mTimer) {
mLock->lock();
LocTimerDelegate* timer = mTimer; LocTimerDelegate* timer = mTimer;
mTimer = NULL; mTimer = NULL;
if (timer) { if (timer) {
timer->destroyLocked(); timer->destroyLocked();
success = true; success = true;
} }
mLock->unlock();
} }
mLock->unlock();
return success; return success;
} }