Fix a race that could cause GL commands to be executed from the wrong thread.

Change-Id: Ia3d407f7bf2f5553f46cfdade70b7b0badb35beb

6 files changed, 79 insertions, 108 deletions

diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index b2f95cd..bbf1814 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -79,6 +79,10 @@ Layer::~Layer()
     }
 }
 
+void Layer::destroy() const {
+    mFlinger->destroyLayer(this);
+}
+
 status_t Layer::setToken(const sp<UserClient>& userClient,
         SharedClient* sharedClient, int32_t token)
 {
@@ -147,18 +151,6 @@ sp<LayerBaseClient::Surface> Layer::createSurface() const
     return sur;
 }
 
-status_t Layer::ditch()
-{
-    // NOTE: Called from the main UI thread
-
-    // the layer is not on screen anymore. free as much resources as possible
-    mFreezeLock.clear();
-
-    Mutex::Autolock _l(mLock);
-    mWidth = mHeight = 0;
-    return NO_ERROR;
-}
-
 status_t Layer::setBuffers( uint32_t w, uint32_t h,
                             PixelFormat format, uint32_t flags)
 {
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 128f93d..278d64e 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -82,7 +82,6 @@ public:
     virtual bool isSecure() const           { return mSecure; }
     virtual bool isProtected() const;
     virtual sp<Surface> createSurface() const;
-    virtual status_t ditch();
     virtual void onRemoved();
 
     // only for debugging
@@ -93,6 +92,7 @@ public:
         return mFreezeLock; }
 
 protected:
+    virtual void destroy() const;
     virtual void dump(String8& result, char* scratch, size_t size) const;
 
 private:
diff --git a/services/surfaceflinger/LayerBase.cpp b/services/surfaceflinger/LayerBase.cpp
index 6025ed4..022f251 100644
--- a/services/surfaceflinger/LayerBase.cpp
+++ b/services/surfaceflinger/LayerBase.cpp
@@ -616,10 +616,7 @@ LayerBaseClient::Surface::~Surface()
      */
 
     // destroy client resources
-    sp<LayerBaseClient> layer = getOwner();
-    if (layer != 0) {
-        mFlinger->destroySurface(layer);
-    }
+    mFlinger->destroySurface(mOwner);
 }
 
 sp<LayerBaseClient> LayerBaseClient::Surface::getOwner() const {
diff --git a/services/surfaceflinger/LayerBase.h b/services/surfaceflinger/LayerBase.h
index 7162e47..6c49a19 100644
--- a/services/surfaceflinger/LayerBase.h
+++ b/services/surfaceflinger/LayerBase.h
@@ -202,10 +202,6 @@ public:
      */
     virtual bool isProtected() const   { return false; }
 
-    /** Called from the main thread, when the surface is removed from the
-     * draw list */
-    virtual status_t ditch() { return NO_ERROR; }
-
     /** called with the state lock when the surface is removed from the
      *  current list */
     virtual void onRemoved() { };
@@ -271,7 +267,8 @@ protected:
     volatile    int32_t         mInvalidate;
                 
 
-protected:
+public:
+    // called from class SurfaceFlinger
     virtual ~LayerBase();
 
 private:
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index e8f0328..b7a51a4 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -387,6 +387,9 @@ bool SurfaceFlinger::threadLoop()
 {
     waitForEvent();
 
+    // call Layer's destructor
+    handleDestroyLayers();
+
     // check for transactions
     if (UNLIKELY(mConsoleSignals)) {
         handleConsoleEvents();
@@ -480,49 +483,27 @@ void SurfaceFlinger::handleConsoleEvents()
 
 void SurfaceFlinger::handleTransaction(uint32_t transactionFlags)
 {
-    Vector< sp<LayerBase> > ditchedLayers;
-
-    /*
-     * Perform and commit the transaction
-     */
-
-    { // scope for the lock
-        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.
+    Mutex::Autolock _l(mStateLock);
+    const nsecs_t now = systemTime();
+    mDebugInTransaction = now;
 
-        const uint32_t mask = eTransactionNeeded | eTraversalNeeded;
-        transactionFlags = getTransactionFlags(mask);
-        handleTransactionLocked(transactionFlags, ditchedLayers);
+    // 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 committed.
 
-        mLastTransactionTime = systemTime() - now;
-        mDebugInTransaction = 0;
-        invalidateHwcGeometry();
-        // here the transaction has been committed
-    }
+    const uint32_t mask = eTransactionNeeded | eTraversalNeeded;
+    transactionFlags = getTransactionFlags(mask);
+    handleTransactionLocked(transactionFlags);
 
-    /*
-     * Clean-up all layers that went away
-     * (do this without the lock held)
-     */
-
-    const size_t count = ditchedLayers.size();
-    for (size_t i=0 ; i<count ; i++) {
-        if (ditchedLayers[i] != 0) {
-            //LOGD("ditching layer %p", ditchedLayers[i].get());
-            ditchedLayers[i]->ditch();
-        }
-    }
+    mLastTransactionTime = systemTime() - now;
+    mDebugInTransaction = 0;
+    invalidateHwcGeometry();
+    // here the transaction has been committed
 }
 
-void SurfaceFlinger::handleTransactionLocked(
-        uint32_t transactionFlags, Vector< sp<LayerBase> >& ditchedLayers)
+void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags)
 {
     const LayerVector& currentLayers(mCurrentState.layersSortedByZ);
     const size_t count = currentLayers.size();
@@ -594,7 +575,6 @@ void SurfaceFlinger::handleTransactionLocked(
                 const sp<LayerBase>& layer(previousLayers[i]);
                 if (currentLayers.indexOf( layer ) < 0) {
                     // this layer is not visible anymore
-                    ditchedLayers.add(layer);
                     mDirtyRegionRemovedLayer.orSelf(layer->visibleRegionScreen);
                 }
             }
@@ -604,6 +584,31 @@ void SurfaceFlinger::handleTransactionLocked(
     commitTransaction();
 }
 
+void SurfaceFlinger::destroyLayer(LayerBase const* layer)
+{
+    Mutex::Autolock _l(mDestroyedLayerLock);
+    mDestroyedLayers.add(layer);
+    signalEvent();
+}
+
+void SurfaceFlinger::handleDestroyLayers()
+{
+    Vector<LayerBase const *> destroyedLayers;
+
+    { // scope for the lock
+        Mutex::Autolock _l(mDestroyedLayerLock);
+        destroyedLayers = mDestroyedLayers;
+        mDestroyedLayers.clear();
+    }
+
+    // call destructors without a lock held
+    const size_t count = destroyedLayers.size();
+    for (size_t i=0 ; i<count ; i++) {
+        //LOGD("destroying %s", destroyedLayers[i]->getName().string());
+        delete destroyedLayers[i];
+    }
+}
+
 sp<FreezeLock> SurfaceFlinger::getFreezeLock() const
 {
     return new FreezeLock(const_cast<SurfaceFlinger *>(this));
@@ -1377,51 +1382,26 @@ status_t SurfaceFlinger::removeSurface(const sp<Client>& client, SurfaceID sid)
     return err;
 }
 
-status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer)
+status_t SurfaceFlinger::destroySurface(const wp<LayerBaseClient>& layer)
 {
     // called by ~ISurface() when all references are gone
-
-    class MessageDestroySurface : public MessageBase {
-        SurfaceFlinger* flinger;
-        sp<LayerBaseClient> layer;
-    public:
-        MessageDestroySurface(
-                SurfaceFlinger* flinger, const sp<LayerBaseClient>& layer)
-            : flinger(flinger), layer(layer) { }
-        virtual bool handler() {
-            sp<LayerBaseClient> l(layer);
-            layer.clear(); // clear it outside of the lock;
-            Mutex::Autolock _l(flinger->mStateLock);
-            /*
-             * remove the layer from the current list -- chances are that it's
-             * not in the list anyway, because it should have been removed
-             * already upon request of the client (eg: window manager).
-             * However, a buggy client could have not done that.
-             * Since we know we don't have any more clients, we don't need
-             * to use the purgatory.
-             */
-            status_t err = flinger->removeLayer_l(l);
-            if (err == NAME_NOT_FOUND) {
-                // The surface wasn't in the current list, which means it was
-                // removed already, which means it is in the purgatory,
-                // and need to be removed from there.
-                // This needs to happen from the main thread since its dtor
-                // must run from there (b/c of OpenGL ES). Additionally, we
-                // can't really acquire our internal lock from
-                // destroySurface() -- see postMessage() below.
-                ssize_t idx = flinger->mLayerPurgatory.remove(l);
-                LOGE_IF(idx < 0,
-                        "layer=%p is not in the purgatory list", l.get());
-            }
-
-            LOGE_IF(err<0 && err != NAME_NOT_FOUND,
-                    "error removing layer=%p (%s)", l.get(), strerror(-err));
-            return true;
-        }
-    };
-
-    postMessageAsync( new MessageDestroySurface(this, layer) );
-    return NO_ERROR;
+    status_t err = NO_ERROR;
+    sp<LayerBaseClient> l(layer.promote());
+    if (l != NULL) {
+        Mutex::Autolock _l(mStateLock);
+        err = removeLayer_l(l);
+        if (err == NAME_NOT_FOUND) {
+            // The surface wasn't in the current list, which means it was
+            // removed already, which means it is in the purgatory,
+            // and need to be removed from there.
+            ssize_t idx = mLayerPurgatory.remove(l);
+            LOGE_IF(idx < 0,
+                    "layer=%p is not in the purgatory list", l.get());
+        }
+        LOGE_IF(err<0 && err != NAME_NOT_FOUND,
+                "error removing layer=%p (%s)", l.get(), strerror(-err));
+    }
+    return err;
 }
 
 status_t SurfaceFlinger::setClientState(
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 1e16943..992861a 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -227,6 +227,7 @@ public:
     status_t addLayer(const sp<LayerBase>& layer);
     status_t invalidateLayerVisibility(const sp<LayerBase>& layer);
     void invalidateHwcGeometry();
+    void destroyLayer(LayerBase const* layer);
 
     sp<Layer> getLayer(const sp<ISurface>& sur) const;
 
@@ -255,7 +256,7 @@ private:
             uint32_t w, uint32_t h, uint32_t flags);
 
     status_t removeSurface(const sp<Client>& client, SurfaceID sid);
-    status_t destroySurface(const sp<LayerBaseClient>& layer);
+    status_t destroySurface(const wp<LayerBaseClient>& layer);
     status_t setClientState(const sp<Client>& client,
             int32_t count, const layer_state_t* states);
 
@@ -300,9 +301,8 @@ public:     // hack to work around gcc 4.0.3 bug
 private:
             void        handleConsoleEvents();
             void        handleTransaction(uint32_t transactionFlags);
-            void        handleTransactionLocked(
-                            uint32_t transactionFlags, 
-                            Vector< sp<LayerBase> >& ditchedLayers);
+            void        handleTransactionLocked(uint32_t transactionFlags);
+            void        handleDestroyLayers();
 
             void        computeVisibleRegions(
                             const LayerVector& currentLayers,
@@ -424,6 +424,11 @@ private:
                 // these are thread safe
     mutable     Barrier                     mReadyToRunBarrier;
 
+
+                // protected by mDestroyedLayerLock;
+    mutable     Mutex                       mDestroyedLayerLock;
+                Vector<LayerBase const *>   mDestroyedLayers;
+
                 // atomic variables
                 enum {
                     eConsoleReleased = 1,