Fix deadlock in SF.

problem was that we were acquiring a strong reference
on Connection object with a lock held, when those
got out of scope (lock still held) their dtor
could be called if all other refs had dropped,
the dtor would acquire the lock again to
remove the Connection from the main list. boom.

we rearange the code so this doesn't happen.

Bug: 6942208

Change-Id: I0a0ebabce2842d29d60d645b64aac2f26640e59b

1 files changed, 86 insertions, 98 deletions

diff --git a/services/surfaceflinger/EventThread.cpp b/services/surfaceflinger/EventThread.cpp
index 9b61fa9..3833f48 100644
--- a/services/surfaceflinger/EventThread.cpp
+++ b/services/surfaceflinger/EventThread.cpp
@@ -36,10 +36,9 @@ namespace android {
 
 EventThread::EventThread(const sp<SurfaceFlinger>& flinger)
     : mFlinger(flinger),
-      mLastVSyncTimestamp(0),
       mVSyncTimestamp(0),
       mUseSoftwareVSync(false),
-      mDeliveredEvents(0),
+      mVSyncCount(0),
       mDebugVsyncEnabled(false) {
 }
 
@@ -116,133 +115,122 @@ void EventThread::onScreenAcquired() {
 void EventThread::onVSyncReceived(int, nsecs_t timestamp) {
     Mutex::Autolock _l(mLock);
     mVSyncTimestamp = timestamp;
+    mVSyncCount++;
     mCondition.broadcast();
 }
 
 bool EventThread::threadLoop() {
 
     nsecs_t timestamp;
+    size_t vsyncCount;
+    size_t activeEvents;
     DisplayEventReceiver::Event vsync;
-    Vector< wp<EventThread::Connection> > displayEventConnections;
+    Vector< sp<EventThread::Connection> > activeConnections;
 
     do {
         Mutex::Autolock _l(mLock);
-        do {
-            // latch VSYNC event if any
-            timestamp = mVSyncTimestamp;
-            mVSyncTimestamp = 0;
-
-            // check if we should be waiting for VSYNC events
-            bool waitForNextVsync = false;
-            size_t count = mDisplayEventConnections.size();
-            for (size_t i=0 ; i<count ; i++) {
-                sp<Connection> connection =
-                        mDisplayEventConnections.itemAt(i).promote();
-                if (connection!=0 && connection->count >= 0) {
+        // latch VSYNC event if any
+        timestamp = mVSyncTimestamp;
+        mVSyncTimestamp = 0;
+
+        // check if we should be waiting for VSYNC events
+        activeEvents = 0;
+        bool waitForNextVsync = false;
+        size_t count = mDisplayEventConnections.size();
+        for (size_t i=0 ; i<count ; i++) {
+            sp<Connection> connection(mDisplayEventConnections[i].promote());
+            if (connection != NULL) {
+                activeConnections.add(connection);
+                if (connection->count >= 0) {
                     // at least one continuous mode or active one-shot event
                     waitForNextVsync = true;
+                    activeEvents++;
                     break;
                 }
             }
+        }
 
-            if (timestamp) {
-                if (!waitForNextVsync) {
-                    // we received a VSYNC but we have no clients
-                    // don't report it, and disable VSYNC events
-                    disableVSyncLocked();
-                } else {
-                    // report VSYNC event
-                    break;
-                }
+        if (timestamp) {
+            if (!waitForNextVsync) {
+                // we received a VSYNC but we have no clients
+                // don't report it, and disable VSYNC events
+                disableVSyncLocked();
             } else {
-                // never disable VSYNC events immediately, instead
-                // we'll wait to receive the event and we'll
-                // reevaluate whether we need to dispatch it and/or
-                // disable VSYNC events then.
-                if (waitForNextVsync) {
-                    // enable
-                    enableVSyncLocked();
-                }
+                // report VSYNC event
+                break;
             }
-
-            // wait for something to happen
-            if (mUseSoftwareVSync && waitForNextVsync) {
-                // h/w vsync cannot be used (screen is off), so we use
-                // a  timeout instead. it doesn't matter how imprecise this
-                // is, we just need to make sure to serve the clients
-                if (mCondition.waitRelative(mLock, ms2ns(16)) == TIMED_OUT) {
-                    mVSyncTimestamp = systemTime(SYSTEM_TIME_MONOTONIC);
-                }
-            } else {
-                mCondition.wait(mLock);
+        } else {
+            // never disable VSYNC events immediately, instead
+            // we'll wait to receive the event and we'll
+            // reevaluate whether we need to dispatch it and/or
+            // disable VSYNC events then.
+            if (waitForNextVsync) {
+                // enable
+                enableVSyncLocked();
             }
-        } while(true);
-
-        // process vsync event
-        mDeliveredEvents++;
-        mLastVSyncTimestamp = timestamp;
+        }
 
-        // now see if we still need to report this VSYNC event
-        const size_t count = mDisplayEventConnections.size();
-        for (size_t i=0 ; i<count ; i++) {
-            bool reportVsync = false;
-            sp<Connection> connection =
-                    mDisplayEventConnections.itemAt(i).promote();
-            if (connection == 0)
-                continue;
-
-            const int32_t count = connection->count;
-            if (count >= 1) {
-                if (count==1 || (mDeliveredEvents % count) == 0) {
-                    // continuous event, and time to report it
-                    reportVsync = true;
-                }
-            } else if (count >= -1) {
-                if (count == 0) {
-                    // fired this time around
-                    reportVsync = true;
-                }
-                connection->count--;
-            }
-            if (reportVsync) {
-                displayEventConnections.add(connection);
+        // wait for something to happen
+        if (mUseSoftwareVSync && waitForNextVsync) {
+            // h/w vsync cannot be used (screen is off), so we use
+            // a  timeout instead. it doesn't matter how imprecise this
+            // is, we just need to make sure to serve the clients
+            if (mCondition.waitRelative(mLock, ms2ns(16)) == TIMED_OUT) {
+                mVSyncTimestamp = systemTime(SYSTEM_TIME_MONOTONIC);
+                mVSyncCount++;
             }
+        } else {
+            mCondition.wait(mLock);
         }
-    } while (!displayEventConnections.size());
+        vsyncCount = mVSyncCount;
+    } while (!activeConnections.size());
+
+    if (!activeEvents) {
+        // no events to return. start over.
+        // (here we make sure to exit the scope of this function
+        //  so that we release our Connection references)
+        return true;
+    }
 
     // dispatch vsync events to listeners...
     vsync.header.type = DisplayEventReceiver::DISPLAY_EVENT_VSYNC;
     vsync.header.timestamp = timestamp;
-    vsync.vsync.count = mDeliveredEvents;
+    vsync.vsync.count = vsyncCount;
 
-    const size_t count = displayEventConnections.size();
+    const size_t count = activeConnections.size();
     for (size_t i=0 ; i<count ; i++) {
-        sp<Connection> conn(displayEventConnections[i].promote());
-        // make sure the connection didn't die
-        if (conn != NULL) {
-            status_t err = conn->postEvent(vsync);
-            if (err == -EAGAIN || err == -EWOULDBLOCK) {
-                // The destination doesn't accept events anymore, it's probably
-                // full. For now, we just drop the events on the floor.
-                // Note that some events cannot be dropped and would have to be
-                // re-sent later. Right-now we don't have the ability to do
-                // this, but it doesn't matter for VSYNC.
-            } else if (err < 0) {
-                // handle any other error on the pipe as fatal. the only
-                // reasonable thing to do is to clean-up this connection.
-                // The most common error we'll get here is -EPIPE.
-                removeDisplayEventConnection(displayEventConnections[i]);
+        const sp<Connection>& conn(activeConnections[i]);
+        // now see if we still need to report this VSYNC event
+        const int32_t vcount = conn->count;
+        if (vcount >= 0) {
+            bool reportVsync = false;
+            if (vcount == 0) {
+                // fired this time around
+                conn->count = -1;
+                reportVsync = true;
+            } else if (vcount == 1 || (vsyncCount % vcount) == 0) {
+                // continuous event, and time to report it
+                reportVsync = true;
+            }
+
+            if (reportVsync) {
+                status_t err = conn->postEvent(vsync);
+                if (err == -EAGAIN || err == -EWOULDBLOCK) {
+                    // The destination doesn't accept events anymore, it's probably
+                    // full. For now, we just drop the events on the floor.
+                    // Note that some events cannot be dropped and would have to be
+                    // re-sent later. Right-now we don't have the ability to do
+                    // this, but it doesn't matter for VSYNC.
+                } else if (err < 0) {
+                    // handle any other error on the pipe as fatal. the only
+                    // reasonable thing to do is to clean-up this connection.
+                    // The most common error we'll get here is -EPIPE.
+                    removeDisplayEventConnection(activeConnections[i]);
+                }
             }
-        } else {
-            // somehow the connection is dead, but we still have it in our list
-            // just clean the list.
-            removeDisplayEventConnection(displayEventConnections[i]);
         }
     }
 
-    // clear all our references without holding mLock
-    displayEventConnections.clear();
-
     return true;
 }
 
@@ -273,7 +261,7 @@ void EventThread::dump(String8& result, char* buffer, size_t SIZE) const {
     result.appendFormat("  soft-vsync: %s\n",
             mUseSoftwareVSync?"enabled":"disabled");
     result.appendFormat("  numListeners=%u,\n  events-delivered: %u\n",
-            mDisplayEventConnections.size(), mDeliveredEvents);
+            mDisplayEventConnections.size(), mVSyncCount);
     for (size_t i=0 ; i<mDisplayEventConnections.size() ; i++) {
         sp<Connection> connection =
                 mDisplayEventConnections.itemAt(i).promote();