From: Mathias Agopian Date: Thu, 19 May 2011 22:38:14 +0000 (-0700) Subject: Fix a race that could cause GL commands to be executed from the wrong thread. X-Git-Tag: android-x86-4.4-r1~4337^2~9^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=b4081bb52aa71b5ffe747d3b5aabd6a2cd392a75;p=android-x86%2Fframeworks-native.git Fix a race that could cause GL commands to be executed from the wrong thread. Bug: 4483050 Change-Id: I37f0f3156059c208c6168ee6131d0e267d823188 --- diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index c9dcef370a..4739cc3072 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -76,6 +76,10 @@ Layer::~Layer() } } +void Layer::destroy() const { + mFlinger->destroyLayer(this); +} + status_t Layer::setToken(const sp& userClient, SharedClient* sharedClient, int32_t token) { @@ -123,22 +127,6 @@ sp Layer::createSurface() const return mSurface; } -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(); - - EGLDisplay dpy(mFlinger->graphicPlane(0).getEGLDisplay()); - mBufferManager.destroy(dpy); - mSurface.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 7bb207a1fc..51b52f9eb1 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -78,7 +78,6 @@ public: virtual bool needsFiltering() const; virtual bool isSecure() const { return mSecure; } virtual sp createSurface() const; - virtual status_t ditch(); virtual void onRemoved(); virtual bool setBypass(bool enable); @@ -95,6 +94,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 21c36e1848..3986fdef0f 100644 --- a/services/surfaceflinger/LayerBase.cpp +++ b/services/surfaceflinger/LayerBase.cpp @@ -583,10 +583,7 @@ LayerBaseClient::Surface::~Surface() */ // destroy client resources - sp layer = getOwner(); - if (layer != 0) { - mFlinger->destroySurface(layer); - } + mFlinger->destroySurface(mOwner); } sp LayerBaseClient::Surface::getOwner() const { diff --git a/services/surfaceflinger/LayerBase.h b/services/surfaceflinger/LayerBase.h index afc5ec8d50..e69cb6a26f 100644 --- a/services/surfaceflinger/LayerBase.h +++ b/services/surfaceflinger/LayerBase.h @@ -196,10 +196,6 @@ public: */ virtual bool isSecure() 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() { }; @@ -264,7 +260,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 5314cb09e7..a93d756848 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -358,6 +358,9 @@ bool SurfaceFlinger::threadLoop() { waitForEvent(); + // call Layer's destructor + handleDestroyLayers(); + // check for transactions if (UNLIKELY(mConsoleSignals)) { handleConsoleEvents(); @@ -467,47 +470,26 @@ void SurfaceFlinger::handleConsoleEvents() void SurfaceFlinger::handleTransaction(uint32_t transactionFlags) { - Vector< sp > ditchedLayers; - - /* - * Perform and commit the transaction - */ - - { // scope for the lock - Mutex::Autolock _l(mStateLock); - const nsecs_t now = systemTime(); - mDebugInTransaction = now; + 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. + // 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. - const uint32_t mask = eTransactionNeeded | eTraversalNeeded; - transactionFlags = getTransactionFlags(mask); - handleTransactionLocked(transactionFlags, ditchedLayers); + const uint32_t mask = eTransactionNeeded | eTraversalNeeded; + transactionFlags = getTransactionFlags(mask); + handleTransactionLocked(transactionFlags); - mLastTransactionTime = systemTime() - now; - mDebugInTransaction = 0; - // here the transaction has been committed - } - - /* - * Clean-up all layers that went away - * (do this without the lock held) - */ - const size_t count = ditchedLayers.size(); - for (size_t i=0 ; iditch(); - } - } + mLastTransactionTime = systemTime() - now; + mDebugInTransaction = 0; + // here the transaction has been committed } -void SurfaceFlinger::handleTransactionLocked( - uint32_t transactionFlags, Vector< sp >& ditchedLayers) +void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) { const LayerVector& currentLayers(mCurrentState.layersSortedByZ); const size_t count = currentLayers.size(); @@ -579,7 +561,6 @@ void SurfaceFlinger::handleTransactionLocked( const sp& layer(previousLayers[i]); if (currentLayers.indexOf( layer ) < 0) { // this layer is not visible anymore - ditchedLayers.add(layer); mDirtyRegionRemovedLayer.orSelf(layer->visibleRegionScreen); } } @@ -589,6 +570,31 @@ void SurfaceFlinger::handleTransactionLocked( commitTransaction(); } +void SurfaceFlinger::destroyLayer(LayerBase const* layer) +{ + Mutex::Autolock _l(mDestroyedLayerLock); + mDestroyedLayers.add(layer); + signalEvent(); +} + +void SurfaceFlinger::handleDestroyLayers() +{ + Vector 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 ; igetName().string()); + delete destroyedLayers[i]; + } +} + sp SurfaceFlinger::getFreezeLock() const { return new FreezeLock(const_cast(this)); @@ -1329,38 +1335,18 @@ status_t SurfaceFlinger::removeSurface(const sp& client, SurfaceID sid) return err; } -status_t SurfaceFlinger::destroySurface(const sp& layer) +status_t SurfaceFlinger::destroySurface(const wp& layer) { // called by ~ISurface() when all references are gone - - class MessageDestroySurface : public MessageBase { - SurfaceFlinger* flinger; - sp layer; - public: - MessageDestroySurface( - SurfaceFlinger* flinger, const sp& layer) - : flinger(flinger), layer(layer) { } - virtual bool handler() { - sp 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); - 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 l(layer.promote()); + if (l != NULL) { + Mutex::Autolock _l(mStateLock); + err = removeLayer_l(l); + 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 a76a1c1182..9fa98cf2ed 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -218,6 +218,7 @@ public: status_t removeLayer(const sp& layer); status_t addLayer(const sp& layer); status_t invalidateLayerVisibility(const sp& layer); + void destroyLayer(LayerBase const* layer); sp getLayer(const sp& sur) const; @@ -255,7 +256,7 @@ private: uint32_t w, uint32_t h, uint32_t flags); status_t removeSurface(const sp& client, SurfaceID sid); - status_t destroySurface(const sp& layer); + status_t destroySurface(const wp& layer); status_t setClientState(const sp& 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 >& ditchedLayers); + void handleTransactionLocked(uint32_t transactionFlags); + void handleDestroyLayers(); void computeVisibleRegions( LayerVector& currentLayers, @@ -422,6 +422,11 @@ private: // these are thread safe mutable Barrier mReadyToRunBarrier; + + // protected by mDestroyedLayerLock; + mutable Mutex mDestroyedLayerLock; + Vector mDestroyedLayers; + // atomic variables enum { eConsoleReleased = 1,