From 34cd9b72f1474ccbfd2338049d81a24bfa814f35 Mon Sep 17 00:00:00 2001
From: Mathias Agopian <mathias@google.com>
Date: Tue, 3 May 2011 17:04:02 -0700
Subject: [PATCH] Fix a race in SurfaceFlinger that could cause layers to be
 leaked forever.

The transaction flags were atomically read-and-cleared to determine if
a transaction was needed, in the later case, mStateLock was taken to
keep the current state still during the transaction. This left a small
window open, where a layer could be removed after the transaction flags
were checked but before the transaction was started holding the lock.
In that situation eTraversalNeeded would be set but only seen during the
next transaction cycle; however, because we're handling this transaction
(because of another flag) it will be commited, "loosing" the information
about the layer being removed -- so when the next transaction cycle due
to eTraversalNeeded starts, it won't notice that layers have been removed
and won't populated the ditchedLayers array.

Bug: 4483049

Change-Id: Ibb5989312f871339928ee1aa3f9567770d72969b
---
 services/surfaceflinger/SurfaceFlinger.cpp | 17 ++++++++++++++++-
 services/surfaceflinger/SurfaceFlinger.h   |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9f007b314..5314cb09e 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -366,7 +366,7 @@ bool SurfaceFlinger::threadLoop()
     if (LIKELY(mTransactionCount == 0)) {
         // if we're in a global transaction, don't do anything.
         const uint32_t mask = eTransactionNeeded | eTraversalNeeded;
-        uint32_t transactionFlags = getTransactionFlags(mask);
+        uint32_t transactionFlags = peekTransactionFlags(mask);
         if (LIKELY(transactionFlags)) {
             handleTransaction(transactionFlags);
         }
@@ -477,7 +477,17 @@ void SurfaceFlinger::handleTransaction(uint32_t transactionFlags)
         Mutex::Autolock _l(mStateLock);
         const nsecs_t now = systemTime();
         mDebugInTransaction = now;
+
+        // Here we're guaranteed that some transaction flags are set
+        // so we can call handleTransactionLocked() unconditionally.
+        // We call getTransactionFlags(), which will also clear the flags,
+        // with mStateLock held to guarantee that mCurrentState won't change
+        // until the transaction is commited.
+
+        const uint32_t mask = eTransactionNeeded | eTraversalNeeded;
+        transactionFlags = getTransactionFlags(mask);
         handleTransactionLocked(transactionFlags, ditchedLayers);
+
         mLastTransactionTime = systemTime() - now;
         mDebugInTransaction = 0;
         // here the transaction has been committed
@@ -1085,6 +1095,11 @@ status_t SurfaceFlinger::invalidateLayerVisibility(const sp<LayerBase>& layer)
     return NO_ERROR;
 }
 
+uint32_t SurfaceFlinger::peekTransactionFlags(uint32_t flags)
+{
+    return android_atomic_release_load(&mTransactionFlags);
+}
+
 uint32_t SurfaceFlinger::getTransactionFlags(uint32_t flags)
 {
     return android_atomic_and(~flags, &mTransactionFlags) & flags;
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index bb89f4314..a76a1c118 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -325,6 +325,7 @@ private:
             status_t    purgatorizeLayer_l(const sp<LayerBase>& layer);
 
             uint32_t    getTransactionFlags(uint32_t flags);
+            uint32_t    peekTransactionFlags(uint32_t flags);
             uint32_t    setTransactionFlags(uint32_t flags);
             void        commitTransaction();