another potential race condition

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

1 files changed, 16 insertions, 18 deletions

diff --git a/utils/LocTimer.cpp b/utils/LocTimer.cpp
index c992e7c..70904b2 100644
--- a/utils/LocTimer.cpp
+++ b/utils/LocTimer.cpp
@@ -333,13 +333,15 @@ void LocTimerContainer::remove(LocTimerDelegate& timer) {
             LocMsg(), mTimerContainer(&container), mTimer(&timer) {}
         inline virtual void proc() const {
             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 passing in NULL, we tell updateSoonestTime to update kernel with
-                // the current top timer interval.
+                // if passing in NULL, we tell updateSoonestTime to update
+                // kernel with the current top timer interval.
                 mTimerContainer->updateSoonestTime(NULL);
             }
+            // all timers are deleted here, and only here.
             delete mTimer;
         }
     };
@@ -367,7 +369,6 @@ void LocTimerContainer::expire() {
                  timer = mTimerContainer->popIfOutRanks(timerOfNow)) {
                 // the timer delegate obj will be deleted before the return of this call
                 timer->expire();
-                delete timer;
             }
             mTimerContainer->updateSoonestTime(NULL);
         }
@@ -493,9 +494,10 @@ void LocTimerDelegate::destroyLocked() {
         if (container) {
             container->remove(*this);
         }
-    } // else we do not do anything, as *this* will be deleted either
-      // after it expires or stop is first called (if case above),
-      // where container->remove() will have it deleted.
+    } // else we do not do anything. No such *this* can be
+      // created and reached here with mContainer ever been
+      // 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) {
@@ -511,17 +513,11 @@ int LocTimerDelegate::ranks(LocRankable& rankable) {
 
 inline
 void LocTimerDelegate::expire() {
-    mLock->lock();
     // keeping a copy of client pointer to be safe
     // 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;
-    // this obj is already removed from mContainer.
-    // 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
+    // force a stop, which will lead to delete of this obj
     if (client && client->stop()) {
         // calling client callback with a pointer save on the stack
         // 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 success = false;
+    mLock->lock();
     if (!mTimer) {
         struct timespec 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
         success = (NULL != mTimer);
     }
+    mLock->unlock();
     return success;
 }
 
 bool LocTimer::stop() {
     bool success = false;
+    mLock->lock();
     if (mTimer) {
-        mLock->lock();
         LocTimerDelegate* timer = mTimer;
         mTimer = NULL;
         if (timer) {
             timer->destroyLocked();
             success = true;
         }
-        mLock->unlock();
     }
+    mLock->unlock();
     return success;
 }