From 63ad28fb53a1b3b652495e57eb5fe52eb2a0ec80 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 15 Jun 2011 20:41:24 -0700 Subject: [PATCH] Revert "revert surfaceflinger leak fix as it uncovered a crasher on xoom" This reverts commit af6edba59e250adbdfa5b3c3be134f70d8c38a16. Change-Id: I7793d3ca8a4d20a2b188364f47854328ab5f586d --- include/utils/RefBase.h | 14 +++++++ libs/utils/RefBase.cpp | 44 +++++++++++++++++----- services/surfaceflinger/Layer.cpp | 5 +++ services/surfaceflinger/Layer.h | 3 +- services/surfaceflinger/SurfaceFlinger.cpp | 28 ++++++++++++++ services/surfaceflinger/SurfaceFlinger.h | 6 +++ 6 files changed, 90 insertions(+), 10 deletions(-) diff --git a/include/utils/RefBase.h b/include/utils/RefBase.h index b31e993d8..c0be59757 100644 --- a/include/utils/RefBase.h +++ b/include/utils/RefBase.h @@ -117,6 +117,20 @@ public: typedef RefBase basetype; + // used to override the RefBase destruction. + class Destroyer { + friend class RefBase; + friend class weakref_type; + public: + virtual ~Destroyer(); + private: + virtual void destroy(RefBase const* base) = 0; + }; + + // Make sure to never acquire a strong reference from this function. The + // same restrictions than for destructors apply. + void setDestroyer(Destroyer* destroyer); + protected: RefBase(); virtual ~RefBase(); diff --git a/libs/utils/RefBase.cpp b/libs/utils/RefBase.cpp index bb6c1255f..6fae30cfa 100644 --- a/libs/utils/RefBase.cpp +++ b/libs/utils/RefBase.cpp @@ -49,6 +49,11 @@ namespace android { // --------------------------------------------------------------------------- +RefBase::Destroyer::~Destroyer() { +} + +// --------------------------------------------------------------------------- + class RefBase::weakref_impl : public RefBase::weakref_type { public: @@ -56,7 +61,7 @@ public: volatile int32_t mWeak; RefBase* const mBase; volatile int32_t mFlags; - + Destroyer* mDestroyer; #if !DEBUG_REFS @@ -65,6 +70,7 @@ public: , mWeak(0) , mBase(base) , mFlags(0) + , mDestroyer(0) { } @@ -357,7 +363,11 @@ void RefBase::decStrong(const void* id) const if (c == 1) { const_cast(this)->onLastStrongRef(id); if ((refs->mFlags&OBJECT_LIFETIME_WEAK) != OBJECT_LIFETIME_WEAK) { - delete this; + if (refs->mDestroyer) { + refs->mDestroyer->destroy(this); + } else { + delete this; + } } } refs->decWeak(id); @@ -390,7 +400,9 @@ int32_t RefBase::getStrongCount() const return mRefs->mStrong; } - +void RefBase::setDestroyer(RefBase::Destroyer* destroyer) { + mRefs->mDestroyer = destroyer; +} RefBase* RefBase::weakref_type::refBase() const { @@ -414,16 +426,28 @@ void RefBase::weakref_type::decWeak(const void* id) if (c != 1) return; if ((impl->mFlags&OBJECT_LIFETIME_WEAK) != OBJECT_LIFETIME_WEAK) { - if (impl->mStrong == INITIAL_STRONG_VALUE) - delete impl->mBase; - else { + if (impl->mStrong == INITIAL_STRONG_VALUE) { + if (impl->mBase) { + if (impl->mDestroyer) { + impl->mDestroyer->destroy(impl->mBase); + } else { + delete impl->mBase; + } + } + } else { // LOGV("Freeing refs %p of old RefBase %p\n", this, impl->mBase); delete impl; } } else { impl->mBase->onLastWeakRef(id); if ((impl->mFlags&OBJECT_LIFETIME_FOREVER) != OBJECT_LIFETIME_FOREVER) { - delete impl->mBase; + if (impl->mBase) { + if (impl->mDestroyer) { + impl->mDestroyer->destroy(impl->mBase); + } else { + delete impl->mBase; + } + } } } } @@ -545,8 +569,10 @@ RefBase::RefBase() RefBase::~RefBase() { - if (mRefs->mWeak == 0) { - delete mRefs; + if ((mRefs->mFlags & OBJECT_LIFETIME_WEAK) == OBJECT_LIFETIME_WEAK) { + if (mRefs->mWeak == 0) { + delete mRefs; + } } } diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 731d82b67..a35811def 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -61,6 +61,7 @@ Layer::Layer(SurfaceFlinger* flinger, mBufferManager(mTextureManager), mWidth(0), mHeight(0), mNeedsScaling(false), mFixedSize(false) { + setDestroyer(this); } Layer::~Layer() @@ -77,6 +78,10 @@ Layer::~Layer() } } +void Layer::destroy(RefBase const* base) { + mFlinger->destroyLayer(static_cast(base)); +} + status_t Layer::setToken(const sp& userClient, SharedClient* sharedClient, int32_t token) { diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 4c9227815..5330c0865 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -44,7 +44,7 @@ class UserClient; // --------------------------------------------------------------------------- -class Layer : public LayerBaseClient +class Layer : public LayerBaseClient, private RefBase::Destroyer { public: Layer(SurfaceFlinger* flinger, DisplayID display, @@ -92,6 +92,7 @@ public: return mFreezeLock; } protected: + virtual void destroy(RefBase const* base); virtual void dump(String8& result, char* scratch, size_t size) const; private: diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 33cb9a41a..a7b585769 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(); @@ -580,6 +583,31 @@ void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) 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)); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 31c20d728..f81b074ea 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -233,6 +233,7 @@ public: status_t addLayer(const sp& layer); status_t invalidateLayerVisibility(const sp& layer); void invalidateHwcGeometry(); + void destroyLayer(LayerBase const* layer); sp getLayer(const sp& sur) const; @@ -306,6 +307,7 @@ private: void handleConsoleEvents(); void handleTransaction(uint32_t transactionFlags); void handleTransactionLocked(uint32_t transactionFlags); + void handleDestroyLayers(); void computeVisibleRegions( LayerVector& currentLayers, @@ -428,6 +430,10 @@ private: mutable Barrier mReadyToRunBarrier; + // protected by mDestroyedLayerLock; + mutable Mutex mDestroyedLayerLock; + Vector mDestroyedLayers; + // atomic variables enum { eConsoleReleased = 1,