free gralloc buffers as soon as possible (when a surface is not visible any longer), client who have the buffers still mapped won't crash, btu may see garbage data

5 files changed, 28 insertions, 38 deletions

diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index c40fec16fbc9..991615800da4 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -76,7 +76,9 @@ void Layer::destroy()
             eglDestroyImageKHR(dpy, mTextures[i].image);
             mTextures[i].image = EGL_NO_IMAGE_KHR;
         }
+        mBuffers[i].free();
     }
+    mSurface.clear();
 }
 
 void Layer::initStates(uint32_t w, uint32_t h, uint32_t flags)
@@ -95,7 +97,6 @@ sp<LayerBaseClient::Surface> Layer::createSurface() const
 status_t Layer::ditch()
 {
     // the layer is not on screen anymore. free as much resources as possible
-    mSurface.clear();
     destroy();
     return NO_ERROR;
 }
diff --git a/libs/surfaceflinger/LayerBitmap.cpp b/libs/surfaceflinger/LayerBitmap.cpp
index a368ca93aaa6..e0984fe48c7c 100644
--- a/libs/surfaceflinger/LayerBitmap.cpp
+++ b/libs/surfaceflinger/LayerBitmap.cpp
@@ -130,13 +130,10 @@ status_t Buffer::lock(GGLSurface* sur, uint32_t usage)
     return res;
 }
 
-
-
 // ===========================================================================
 // LayerBitmap
 // ===========================================================================
 
-
 LayerBitmap::LayerBitmap()
     : mInfo(0), mWidth(0), mHeight(0)
 {
@@ -184,7 +181,7 @@ sp<Buffer> LayerBitmap::allocate()
     sp<Buffer> buffer(mBuffer);
     const uint32_t w = mWidth; 
     const uint32_t h = mHeight;
-    if (w != buffer->getWidth() || h != buffer->getHeight()) {
+    if (buffer!=0 && (w != buffer->getWidth() || h != buffer->getHeight())) {
         surface_info_t* info = mInfo;
         buffer = new Buffer(w, h, mFormat, mFlags);
         status_t err = buffer->initCheck();
@@ -200,6 +197,15 @@ sp<Buffer> LayerBitmap::allocate()
     return buffer;
 }
 
+status_t LayerBitmap::free()
+{
+    mBuffer.clear();
+    mWidth = 0;
+    mHeight = 0;
+    return NO_ERROR;
+}
+
+
 // ---------------------------------------------------------------------------
 
 }; // namespace android
diff --git a/libs/surfaceflinger/LayerBitmap.h b/libs/surfaceflinger/LayerBitmap.h
index 824e0f2b311b..f3636c03e03b 100644
--- a/libs/surfaceflinger/LayerBitmap.h
+++ b/libs/surfaceflinger/LayerBitmap.h
@@ -110,7 +110,8 @@ public:
     status_t setSize(uint32_t w, uint32_t h);
 
     sp<Buffer> allocate();
-
+    status_t free();
+    
     sp<const Buffer>  getBuffer() const { return mBuffer; }
     sp<Buffer>        getBuffer()       { return mBuffer; }
     
diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp
index e74e0009232d..c38668113213 100644
--- a/libs/surfaceflinger/SurfaceFlinger.cpp
+++ b/libs/surfaceflinger/SurfaceFlinger.cpp
@@ -1095,12 +1095,9 @@ status_t SurfaceFlinger::removeLayer_l(const sp<LayerBase>& layerBase)
 
 status_t SurfaceFlinger::purgatorizeLayer_l(const sp<LayerBase>& layerBase)
 {
-    // First add the layer to the purgatory list, which makes sure it won't 
-    // go away, then remove it from the main list (through a transaction).
+    // remove the layer from the main list (through a transaction).
     ssize_t err = removeLayer_l(layerBase);
-    if (err >= 0) {
-        mLayerPurgatory.add(layerBase);
-    }
+
     // it's possible that we don't find a layer, because it might
     // have been destroyed already -- this is not technically an error
     // from the user because there is a race between BClient::destroySurface(),
@@ -1336,7 +1333,7 @@ status_t SurfaceFlinger::removeSurface(SurfaceID index)
 
 status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer)
 {
-    /* called by ~ISurface() when all references are gone */
+    // called by ~ISurface() when all references are gone
     
     class MessageDestroySurface : public MessageBase {
         SurfaceFlinger* flinger;
@@ -1349,33 +1346,21 @@ status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer)
             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.
+            /*
+             * 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;
         }
     };
 
-    // It's better to not acquire our internal lock here, because it's hard
-    // to predict that it's not going to be already taken when ~Surface()
-    // is called.
-
     mEventQueue.postMessage( new MessageDestroySurface(this, layer) );
     return NO_ERROR;
 }
@@ -1537,8 +1522,7 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
                 mFreezeDisplay?"yes":"no", mFreezeCount,
                 mCurrentState.orientation, hw.canDraw());
         result.append(buffer);
-        snprintf(buffer, SIZE, "  purgatory size: %d, client count: %d\n",
-                mLayerPurgatory.size(), mClientsMap.size());
+        snprintf(buffer, SIZE, "  client count: %d\n", mClientsMap.size());
         result.append(buffer);
         const BufferAllocator& alloc(BufferAllocator::get());
         alloc.dump(result);
diff --git a/libs/surfaceflinger/SurfaceFlinger.h b/libs/surfaceflinger/SurfaceFlinger.h
index 149eef049158..b7b008d9ff06 100644
--- a/libs/surfaceflinger/SurfaceFlinger.h
+++ b/libs/surfaceflinger/SurfaceFlinger.h
@@ -307,8 +307,6 @@ private:
     volatile    int32_t                 mTransactionFlags;
     volatile    int32_t                 mTransactionCount;
                 Condition               mTransactionCV;
-                SortedVector< sp<LayerBase> > mLayerPurgatory;
-
                 
                 // protected by mStateLock (but we could use another lock)
                 Tokenizer                               mTokens;