From dbc93d7ae8c7660aab002bcec2ade86daad7c172 Mon Sep 17 00:00:00 2001 From: Garfield Tan Date: Tue, 26 Oct 2021 18:28:57 -0700 Subject: Save Layer instead of Handle to LayerCreatedState Saving Handle to LayerCreatedState can lead to the destruction of Handle when handleLayerCreatedLocked returns if the client side releases the Handle before that. That destruction will attempt to acquire mStateLock in the main thread again and lead to a deadlock. Not saving Handle to LayerCreatedState will avoid Handle being destroyed in the main thread. The destruction of Layer in the main thread is OK. To make that happen I moved the final resolution of addToRoot to createLayer as well because the treatments for invalid parentHandle and invalid parentLayer are different. Bug: 204204635 Bug: 202621651 Test: atest SurfaceFlinger_test Test: atest libsurfaceflinger_unittest Change-Id: I0854203c0949302d7f514d34b956eb7ae976c51f --- services/surfaceflinger/SurfaceFlinger.cpp | 35 +++++++++++++++--------------- services/surfaceflinger/SurfaceFlinger.h | 15 +++++-------- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 8d7221c1dc..48c3c108c1 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3356,8 +3356,7 @@ bool SurfaceFlinger::latchBuffers() { status_t SurfaceFlinger::addClientLayer(const sp& client, const sp& handle, const sp& gbc, const sp& lbc, - const sp& parentHandle, - const sp& parentLayer, bool addToRoot, + const wp& parent, bool addToRoot, uint32_t* outTransformHint) { if (mNumLayers >= ISurfaceComposer::MAX_LAYERS) { ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(), @@ -3369,7 +3368,7 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, const sp states; @@ -4227,7 +4226,7 @@ status_t SurfaceFlinger::mirrorLayer(const sp& client, const sp } *outLayerId = mirrorLayer->sequence; - return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, nullptr, false, + return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, false, nullptr /* outTransformHint */); } @@ -4296,8 +4295,15 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp& clie } bool addToRoot = callingThreadHasUnscopedSurfaceFlingerAccess(); - result = addClientLayer(client, *handle, *gbp, layer, parentHandle, parentLayer, addToRoot, - outTransformHint); + wp parent(parentHandle != nullptr ? fromHandle(parentHandle) : parentLayer); + if (parentHandle != nullptr && parent == nullptr) { + ALOGE("Invalid parent handle %p.", parentHandle.get()); + addToRoot = false; + } + if (parentLayer != nullptr) { + addToRoot = false; + } + result = addClientLayer(client, *handle, *gbp, layer, parent, addToRoot, outTransformHint); if (result != NO_ERROR) { return result; } @@ -6777,11 +6783,11 @@ void TransactionState::traverseStatesWithBuffers( } void SurfaceFlinger::setLayerCreatedState(const sp& handle, const wp& layer, - const sp& parent, const wp parentLayer, - const wp& producer, bool addToRoot) { + const wp parent, const wp& producer, + bool addToRoot) { Mutex::Autolock lock(mCreatedLayersLock); mCreatedLayers[handle->localBinder()] = - std::make_unique(layer, parent, parentLayer, producer, addToRoot); + std::make_unique(layer, parent, producer, addToRoot); } auto SurfaceFlinger::getLayerCreatedState(const sp& handle) { @@ -6819,19 +6825,14 @@ sp SurfaceFlinger::handleLayerCreatedLocked(const sp& handle) { } sp parent; - bool allowAddRoot = state->addToRoot; if (state->initialParent != nullptr) { - parent = fromHandle(state->initialParent).promote(); + parent = state->initialParent.promote(); if (parent == nullptr) { - ALOGE("Invalid parent %p", state->initialParent.get()); - allowAddRoot = false; + ALOGE("Invalid parent %p", state->initialParent.unsafe_get()); } - } else if (state->initialParentLayer != nullptr) { - parent = state->initialParentLayer.promote(); - allowAddRoot = false; } - if (parent == nullptr && allowAddRoot) { + if (parent == nullptr && state->addToRoot) { layer->setIsAtRoot(true); mCurrentState.layersSortedByZ.add(layer); } else if (parent == nullptr) { diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 276c7f6bfe..a1e431b54f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -795,8 +795,8 @@ private: // add a layer to SurfaceFlinger status_t addClientLayer(const sp& client, const sp& handle, const sp& gbc, const sp& lbc, - const sp& parentHandle, const sp& parentLayer, - bool addToRoot, uint32_t* outTransformHint); + const wp& parentLayer, bool addToRoot, + uint32_t* outTransformHint); // Traverse through all the layers and compute and cache its bounds. void computeLayerBounds(); @@ -1345,18 +1345,16 @@ private: GUARDED_BY(mStateLock); mutable Mutex mCreatedLayersLock; struct LayerCreatedState { - LayerCreatedState(const wp& layer, const sp& parent, - const wp parentLayer, const wp& producer, bool addToRoot) + LayerCreatedState(const wp& layer, const wp parent, + const wp& producer, bool addToRoot) : layer(layer), initialParent(parent), - initialParentLayer(parentLayer), initialProducer(producer), addToRoot(addToRoot) {} wp layer; // Indicates the initial parent of the created layer, only used for creating layer in // SurfaceFlinger. If nullptr, it may add the created layer into the current root layers. - sp initialParent; - wp initialParentLayer; + wp initialParent; // Indicates the initial graphic buffer producer of the created layer, only used for // creating layer in SurfaceFlinger. wp initialProducer; @@ -1370,8 +1368,7 @@ private: // thread. std::unordered_map> mCreatedLayers; void setLayerCreatedState(const sp& handle, const wp& layer, - const sp& parent, const wp parentLayer, - const wp& producer, bool addToRoot); + const wp parent, const wp& producer, bool addToRoot); auto getLayerCreatedState(const sp& handle); sp handleLayerCreatedLocked(const sp& handle) REQUIRES(mStateLock); -- cgit v1.2.3-59-g8ed1b