From: Dan Stoza Date: Tue, 9 Aug 2016 20:21:03 +0000 (-0700) Subject: SF: Fix a couple of Layer ref count issues X-Git-Tag: android-x86-8.1-r1~377^2^2~25^2~124^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=de84eb6b82;p=android-x86%2Fframeworks-native.git SF: Fix a couple of Layer ref count issues This is an attempt at fixing two reference counting issues for Layers. The first issue is that since we were holding an sp (really a reference to a LayerCleaner) inside the layer state for deferred transactions, there was a possibility that it could end up being the last strong reference to the LayerCleaner such that when it was destroyed while applying a non-deferred transaction, it would attempt to grab the SurfaceFlinger main lock to destroy its Layer. Since this occurred in the main SurfaceFlinger loop, which was already holding the lock to process transactions, this would cause a deadlock. To fix this, the sp inside the layer state was changed to a wp, only being promoted when it actually needs to be accessed (i.e., when the deferred transaction is created). The second issue is that we were promoting and holding a strong reference to a Layer before calling into SurfaceFlinger to destroy it on the onLayerDestroyed path (triggered when a LayerCleaner is destroyed). After returning from the attempt to grab the SurfaceFlinger main lock, it was possible that this strong reference was the last one keeping the Layer alive, and destroying it at this point could cause the HWC2 version of the layer to be destroyed at effectively any point, even between validate/present. To fix this, the promotion of the weak Layer reference was moved inside the critical section where the SurfaceFlinger main lock is held. Test: Cherry-pick from internal branch Bug: 30503916 Bug: 30281222 Change-Id: I1c6a271f9a7b5d6eea9a9db61d971f262d0cfe84 --- diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index 2a025b8a93..3e48cd2fa0 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -43,10 +43,7 @@ Client::~Client() { const size_t count = mLayers.size(); for (size_t i=0 ; i layer(mLayers.valueAt(i).promote()); - if (layer != 0) { - mFlinger->removeLayer(layer); - } + mFlinger->removeLayer(mLayers.valueAt(i)); } } diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index a512acc1c9..9173165d0c 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -1289,9 +1289,14 @@ void Layer::pushPendingState() { // If this transaction is waiting on the receipt of a frame, generate a sync // point and send it to the remote layer. if (mCurrentState.handle != nullptr) { - sp handle = static_cast(mCurrentState.handle.get()); - sp handleLayer = handle->owner.promote(); - if (handleLayer == nullptr) { + sp strongBinder = mCurrentState.handle.promote(); + sp handle = nullptr; + sp handleLayer = nullptr; + if (strongBinder != nullptr) { + handle = static_cast(strongBinder.get()); + handleLayer = handle->owner.promote(); + } + if (strongBinder == nullptr || handleLayer == nullptr) { ALOGE("[%s] Unable to promote Layer handle", mName.string()); // If we can't promote the layer we are intended to wait on, // then it is expired or otherwise invalid. Allow this transaction diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 55e3b5461b..c96e7d5ee9 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -124,7 +124,7 @@ public: // If set, defers this state update until the Layer identified by handle // receives a frame with the given frameNumber - sp handle; + wp handle; uint64_t frameNumber; // the transparentRegion hint is a bit special, it's latched only diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index a4e356891e..47f5401c13 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2106,8 +2106,14 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, return NO_ERROR; } -status_t SurfaceFlinger::removeLayer(const sp& layer) { +status_t SurfaceFlinger::removeLayer(const wp& weakLayer) { Mutex::Autolock _l(mStateLock); + sp layer = weakLayer.promote(); + if (layer == nullptr) { + // The layer has already been removed, carry on + return NO_ERROR; + } + ssize_t index = mCurrentState.layersSortedByZ.remove(layer); if (index >= 0) { mLayersPendingRemoval.push(layer); @@ -2448,14 +2454,7 @@ status_t SurfaceFlinger::onLayerDestroyed(const wp& layer) { // called by ~LayerCleaner() when all references to the IBinder (handle) // are gone - status_t err = NO_ERROR; - sp l(layer.promote()); - if (l != NULL) { - err = removeLayer(l); - ALOGE_IF(err<0 && err != NAME_NOT_FOUND, - "error removing layer=%p (%s)", l.get(), strerror(-err)); - } - return err; + return removeLayer(layer); } // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 633e956d24..8279a85b45 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -311,7 +311,7 @@ private: status_t onLayerDestroyed(const wp& layer); // remove a layer from SurfaceFlinger immediately - status_t removeLayer(const sp& layer); + status_t removeLayer(const wp& layer); // add a layer to SurfaceFlinger status_t addClientLayer(const sp& client, diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp index 7f3b269f0e..072de81773 100644 --- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp +++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp @@ -2096,8 +2096,14 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, return NO_ERROR; } -status_t SurfaceFlinger::removeLayer(const sp& layer) { +status_t SurfaceFlinger::removeLayer(const wp& weakLayer) { Mutex::Autolock _l(mStateLock); + sp layer = weakLayer.promote(); + if (layer == nullptr) { + // The layer has already been removed, carry on + return NO_ERROR; + } + ssize_t index = mCurrentState.layersSortedByZ.remove(layer); if (index >= 0) { mLayersPendingRemoval.push(layer); @@ -2438,14 +2444,7 @@ status_t SurfaceFlinger::onLayerDestroyed(const wp& layer) { // called by ~LayerCleaner() when all references to the IBinder (handle) // are gone - status_t err = NO_ERROR; - sp l(layer.promote()); - if (l != NULL) { - err = removeLayer(l); - ALOGE_IF(err<0 && err != NAME_NOT_FOUND, - "error removing layer=%p (%s)", l.get(), strerror(-err)); - } - return err; + return removeLayer(layer); } // ---------------------------------------------------------------------------