From 3d57964a81cd631d80aa9575647e1ce35b5e82d5 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 4 Jun 2009 18:46:21 -0700 Subject: [PATCH] fix a bunch of problems with destroying surfaces. now, all destruction path, go through the purgatory which is emptied when ~ISurface is called, but we also make sure to remove the surface from the current list from there (in case a client forgot to request the destruction explicitely). --- include/ui/ISurfaceComposer.h | 1 - libs/surfaceflinger/SurfaceFlinger.cpp | 96 ++++++++++++++++---------- libs/surfaceflinger/SurfaceFlinger.h | 3 + 3 files changed, 63 insertions(+), 37 deletions(-) diff --git a/include/ui/ISurfaceComposer.h b/include/ui/ISurfaceComposer.h index ab6dd56e6..ce2b94c3e 100644 --- a/include/ui/ISurfaceComposer.h +++ b/include/ui/ISurfaceComposer.h @@ -62,7 +62,6 @@ public: eTransparentRegionChanged = 0x00000020, eVisibilityChanged = 0x00000040, eFreezeTintChanged = 0x00000080, - eDestroyed = 0x00000100 }; enum { diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp index 70f34a1a4..0d1be2d06 100644 --- a/libs/surfaceflinger/SurfaceFlinger.cpp +++ b/libs/surfaceflinger/SurfaceFlinger.cpp @@ -246,12 +246,12 @@ void SurfaceFlinger::destroyConnection(ClientID cid) Client* const client = mClientsMap.valueFor(cid); if (client) { // free all the layers this client owns - const Vector< wp >& layers = client->getLayers(); + Vector< wp > layers(client->getLayers()); const size_t count = layers.size(); for (size_t i=0 ; i layer(layers[i].promote()); if (layer != 0) { - removeLayer_l(layer); + purgatorizeLayer_l(layer); } } @@ -551,9 +551,25 @@ void SurfaceFlinger::handleConsoleEvents() void SurfaceFlinger::handleTransaction(uint32_t transactionFlags) { - Mutex::Autolock _l(mStateLock); + Vector< sp > ditchedLayers; - const LayerVector& currentLayers = mCurrentState.layersSortedByZ; + { // scope for the lock + Mutex::Autolock _l(mStateLock); + handleTransactionLocked(transactionFlags, ditchedLayers); + } + + // do this without lock held + const size_t count = ditchedLayers.size(); + for (size_t i=0 ; iditch(); + } +} + +void SurfaceFlinger::handleTransactionLocked( + uint32_t transactionFlags, Vector< sp >& ditchedLayers) +{ + const LayerVector& currentLayers(mCurrentState.layersSortedByZ); const size_t count = currentLayers.size(); /* @@ -628,14 +644,12 @@ void SurfaceFlinger::handleTransaction(uint32_t transactionFlags) if (mLayersRemoved) { mVisibleRegionsDirty = true; const LayerVector& previousLayers(mDrawingState.layersSortedByZ); - const ssize_t count = previousLayers.size(); - for (ssize_t i=0 ; i& layer(previousLayers[i]); if (currentLayers.indexOf( layer ) < 0) { // this layer is not visible anymore - // FIXME: would be better to call without the lock held - //LOGD("ditching layer %p", layer.get()); - layer->ditch(); + ditchedLayers.add(layer); } } } @@ -1012,9 +1026,10 @@ status_t SurfaceFlinger::addLayer(const sp& layer) status_t SurfaceFlinger::removeLayer(const sp& layer) { Mutex::Autolock _l(mStateLock); - removeLayer_l(layer); - setTransactionFlags(eTransactionNeeded); - return NO_ERROR; + status_t err = purgatorizeLayer_l(layer); + if (err == NO_ERROR) + setTransactionFlags(eTransactionNeeded); + return err; } status_t SurfaceFlinger::invalidateLayerVisibility(const sp& layer) @@ -1047,11 +1062,7 @@ status_t SurfaceFlinger::removeLayer_l(const sp& layerBase) } return NO_ERROR; } - // 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(), - // ~BClient() and destroySurface-from-a-transaction. - return (index == NAME_NOT_FOUND) ? status_t(NO_ERROR) : index; + return status_t(index); } status_t SurfaceFlinger::purgatorizeLayer_l(const sp& layerBase) @@ -1062,6 +1073,10 @@ status_t SurfaceFlinger::purgatorizeLayer_l(const sp& 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(), + // ~BClient() and ~ISurface(). return (err == NAME_NOT_FOUND) ? status_t(NO_ERROR) : err; } @@ -1294,12 +1309,7 @@ status_t SurfaceFlinger::removeSurface(SurfaceID index) status_t SurfaceFlinger::destroySurface(const sp& layer) { - /* - * called by ~ISurface() when all references are gone - * - * the surface must be removed from purgatory from the main thread - * since its dtor must run from there (b/c of OpenGL ES). - */ + /* called by ~ISurface() when all references are gone */ class MessageDestroySurface : public MessageBase { SurfaceFlinger* flinger; @@ -1310,11 +1320,33 @@ status_t SurfaceFlinger::destroySurface(const sp& layer) : flinger(flinger), layer(layer) { } virtual bool handler() { Mutex::Autolock _l(flinger->mStateLock); - ssize_t idx = flinger->mLayerPurgatory.remove(layer); - LOGE_IF(idx<0, "layer=%p is not in the purgatory list", layer.get()); + // 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(layer); + 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(layer); + LOGE_IF(idx < 0, + "layer=%p is not in the purgatory list", layer.get()); + } 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; } @@ -1329,18 +1361,9 @@ status_t SurfaceFlinger::setClientState( cid <<= 16; for (int i=0 ; i& layer = getLayerUser_l(s.surface | cid); + sp layer(getLayerUser_l(s.surface | cid)); if (layer != 0) { const uint32_t what = s.what; - // check if it has been destroyed first - if (what & eDestroyed) { - if (removeLayer_l(layer) == NO_ERROR) { - flags |= eTransactionNeeded; - // we skip everything else... well, no, not really - // we skip ONLY that transaction. - continue; - } - } if (what & ePositionChanged) { if (layer->setPosition(s.x, s.y)) flags |= eTraversalNeeded; @@ -1486,7 +1509,8 @@ status_t SurfaceFlinger::dump(int fd, const Vector& args) mFreezeDisplay?"yes":"no", mFreezeCount, mCurrentState.orientation, hw.canDraw()); result.append(buffer); - + snprintf(buffer, SIZE, " purgatory size: %d\n", mLayerPurgatory.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 6bbb21ff5..a21e18644 100644 --- a/libs/surfaceflinger/SurfaceFlinger.h +++ b/libs/surfaceflinger/SurfaceFlinger.h @@ -246,6 +246,9 @@ private: void handleConsoleEvents(); void handleTransaction(uint32_t transactionFlags); + void handleTransactionLocked( + uint32_t transactionFlags, + Vector< sp >& ditchedLayers); void computeVisibleRegions( LayerVector& currentLayers,