diff options
author | TreeHugger Robot <treehugger-gerrit@google.com> | 2017-06-17 01:51:06 +0000 |
---|---|---|
committer | Android (Google) Code Review <android-gerrit@google.com> | 2017-06-17 01:51:07 +0000 |
commit | a17b14eb92f6c21a9cea97ed0ca30df74da9fc29 (patch) | |
tree | 55a0948aaa76e5d270540f7b39d5927f6ba2c80b | |
parent | 4e623e259c3e8677d57f761a3708423fb2e77e2a (diff) | |
parent | 515dc9c538b8206b746eeb4906ac0b8aed1fb497 (diff) | |
download | native-a17b14eb92f6c21a9cea97ed0ca30df74da9fc29.tar.gz |
Merge changes from topic 'layer-wp-race' into oc-dev
* changes:
surfaceflinger: Layer::getParent requires state lock held
surfaceflinger: distinguish mCurrentParent/mDrawingParent
surfaceflinger: protect Client::mParentLayer with a lock
-rw-r--r-- | services/surfaceflinger/Client.cpp | 19 | ||||
-rw-r--r-- | services/surfaceflinger/Client.h | 3 | ||||
-rw-r--r-- | services/surfaceflinger/Layer.cpp | 48 | ||||
-rw-r--r-- | services/surfaceflinger/Layer.h | 5 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.cpp | 20 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.h | 2 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger_hwc1.cpp | 20 |
7 files changed, 61 insertions, 56 deletions
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index e9a251305f..8ba6cb9ba7 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -57,9 +57,19 @@ Client::~Client() } void Client::setParentLayer(const sp<Layer>& parentLayer) { + Mutex::Autolock _l(mLock); mParentLayer = parentLayer; } +sp<Layer> Client::getParentLayer(bool* outParentDied) const { + Mutex::Autolock _l(mLock); + sp<Layer> parent = mParentLayer.promote(); + if (outParentDied != nullptr) { + *outParentDied = (mParentLayer != nullptr && parent == nullptr); + } + return parent; +} + status_t Client::initCheck() const { return NO_ERROR; } @@ -108,7 +118,7 @@ status_t Client::onTransact( // We grant an exception in the case that the Client has a "parent layer", as its // effects will be scoped to that layer. if (CC_UNLIKELY(pid != self_pid && uid != AID_GRAPHICS && uid != AID_SYSTEM && uid != 0) - && (mParentLayer.promote() == nullptr)) { + && (getParentLayer() == nullptr)) { // we're called from a different process, do the real check if (!PermissionCache::checkCallingPermission(sAccessSurfaceFlinger)) { @@ -135,11 +145,12 @@ status_t Client::createSurface( return NAME_NOT_FOUND; } } - if (parent == nullptr && mParentLayer != nullptr) { - parent = mParentLayer.promote(); + if (parent == nullptr) { + bool parentDied; + parent = getParentLayer(&parentDied); // If we had a parent, but it died, we've lost all // our capabilities. - if (parent == nullptr) { + if (parentDied) { return NAME_NOT_FOUND; } } diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h index b5f98b8a6f..2aab28f37d 100644 --- a/services/surfaceflinger/Client.h +++ b/services/surfaceflinger/Client.h @@ -71,12 +71,13 @@ private: virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags); + sp<Layer> getParentLayer(bool* outParentDied = nullptr) const; + // constant sp<SurfaceFlinger> mFlinger; // protected by mLock DefaultKeyedVector< wp<IBinder>, wp<Layer> > mLayers; - wp<Layer> mParentLayer; // thread-safe diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 2ff14aa395..022b41634d 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -403,7 +403,7 @@ Rect Layer::computeScreenBounds(bool reduceTransparentRegion) const { win.intersect(s.finalCrop, &win); } - const sp<Layer>& p = getParent(); + const sp<Layer>& p = mDrawingParent.promote(); // Now we need to calculate the parent bounds, so we can clip ourselves to those. // When calculating the parent bounds for purposes of clipping, // we don't need to constrain the parent to its transparent region. @@ -440,7 +440,7 @@ Rect Layer::computeBounds(const Region& activeTransparentRegion) const { } Rect bounds = win; - const auto& p = getParent(); + const auto& p = mDrawingParent.promote(); if (p != nullptr) { // Look in computeScreenBounds recursive call for explanation of // why we pass false here. @@ -498,7 +498,7 @@ FloatRect Layer::computeCrop(const sp<const DisplayDevice>& hw) const { // Screen space to make reduction to parent crop clearer. Rect activeCrop = computeInitialCrop(hw); - const auto& p = getParent(); + const auto& p = mDrawingParent.promote(); if (p != nullptr) { auto parentCrop = p->computeInitialCrop(hw); activeCrop.intersect(parentCrop, &activeCrop); @@ -710,7 +710,7 @@ void Layer::setGeometry( int type = s.type; int appId = s.appId; - sp<Layer> parent = mParent.promote(); + sp<Layer> parent = mDrawingParent.promote(); if (parent.get()) { auto& parentState = parent->getDrawingState(); type = parentState.type; @@ -1109,8 +1109,9 @@ void Layer::onDraw(const sp<const DisplayDevice>& hw, const Region& clip, * of a camera where the buffer remains in native orientation, * we want the pixels to always be upright. */ - if (getParent() != nullptr) { - const auto parentTransform = getParent()->getTransform(); + sp<Layer> p = mDrawingParent.promote(); + if (p != nullptr) { + const auto parentTransform = p->getTransform(); tr = tr * inverseOrientation(parentTransform.getOrientation()); } @@ -1928,7 +1929,7 @@ bool Layer::setDataSpace(android_dataspace dataSpace) { } uint32_t Layer::getLayerStack() const { - auto p = getParent(); + auto p = mDrawingParent.promote(); if (p == nullptr) { return getDrawingState().layerStack; } @@ -2071,7 +2072,7 @@ void Layer::releasePendingBuffer(nsecs_t dequeueReadyTime) { bool Layer::isHiddenByPolicy() const { const Layer::State& s(mDrawingState); - const auto& parent = getParent(); + const auto& parent = mDrawingParent.promote(); if (parent != nullptr && parent->isHiddenByPolicy()) { return true; } @@ -2555,25 +2556,7 @@ bool Layer::reparentChildren(const sp<IBinder>& newParentHandle) { } for (const sp<Layer>& child : mCurrentChildren) { - // We don't call addChild as we need to delay updating the child's parent pointer until - // a transaction occurs. Remember a refresh could occur in between now and the next - // transaction, in which case the Layer's parent pointer would be updated, but changes - // made to the parent in the same transaction would not have applied. - // This means that the following kind of scenario wont work: - // - // 1. Existing and visible child and parent surface exist - // 2. Create new surface hidden - // 3. Open transaction - // 4. Show the new surface, and reparent the old surface's children to it. - // 5. Close transaction. - // - // If we were to update the parent pointer immediately, then the child surface - // could disappear for one frame as it pointed at the new parent which - // hasn't yet become visible as the transaction hasn't yet occurred. - // - // Instead we defer the reparenting to commitChildList which happens as part - // of the global transaction. - newParent->mCurrentChildren.add(child); + newParent->addChild(child); sp<Client> client(child->mClientRef.promote()); if (client != nullptr) { @@ -2601,7 +2584,7 @@ bool Layer::detachChildren() { } void Layer::setParent(const sp<Layer>& layer) { - mParent = layer; + mCurrentParent = layer; } void Layer::clearSyncPoints() { @@ -2691,7 +2674,7 @@ void Layer::traverseInReverseZOrder(LayerVector::StateSet stateSet, Transform Layer::getTransform() const { Transform t; - const auto& p = getParent(); + const auto& p = mDrawingParent.promote(); if (p != nullptr) { t = p->getTransform(); @@ -2724,14 +2707,14 @@ Transform Layer::getTransform() const { #ifdef USE_HWC2 float Layer::getAlpha() const { - const auto& p = getParent(); + const auto& p = mDrawingParent.promote(); float parentAlpha = (p != nullptr) ? p->getAlpha() : 1.0; return parentAlpha * getDrawingState().alpha; } #else uint8_t Layer::getAlpha() const { - const auto& p = getParent(); + const auto& p = mDrawingParent.promote(); float parentAlpha = (p != nullptr) ? (p->getAlpha() / 255.0f) : 1.0; float drawingAlpha = getDrawingState().alpha / 255.0f; @@ -2743,11 +2726,10 @@ uint8_t Layer::getAlpha() const { void Layer::commitChildList() { for (size_t i = 0; i < mCurrentChildren.size(); i++) { const auto& child = mCurrentChildren[i]; - child->setParent(this); - child->commitChildList(); } mDrawingChildren = mCurrentChildren; + mDrawingParent = mCurrentParent; } // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 5335fff9ff..24fc10dd2b 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -524,7 +524,7 @@ public: // Returns index if removed, or negative value otherwise // for symmetry with Vector::remove ssize_t removeChild(const sp<Layer>& layer); - sp<Layer> getParent() const { return mParent.promote(); } + sp<Layer> getParent() const { return mCurrentParent.promote(); } bool hasParent() const { return getParent() != nullptr; } Rect computeScreenBounds(bool reduceTransparentRegion = true) const; @@ -801,7 +801,8 @@ private: // Child list used for rendering. LayerVector mDrawingChildren; - wp<Layer> mParent; + wp<Layer> mCurrentParent; + wp<Layer> mDrawingParent; }; // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index a49e8f4aa8..6174185969 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2679,14 +2679,18 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, return NO_ERROR; } -status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) { +status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly) { Mutex::Autolock _l(mStateLock); const auto& p = layer->getParent(); - const ssize_t index = (p != nullptr) ? p->removeChild(layer) : - mCurrentState.layersSortedByZ.remove(layer); - + ssize_t index; if (p != nullptr) { + if (topLevelOnly) { + return NO_ERROR; + } + + index = p->removeChild(layer); + sp<Layer> ancestor = p; while (ancestor->getParent() != nullptr) { ancestor = ancestor->getParent(); @@ -2695,6 +2699,8 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) { ALOGE("removeLayer called with a layer whose parent has been removed"); return NAME_NOT_FOUND; } + } else { + index = mCurrentState.layersSortedByZ.remove(layer); } // As a matter of normal operation, the LayerCleaner will produce a second @@ -3125,11 +3131,9 @@ status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer) if (l == nullptr) { // The layer has already been removed, carry on return NO_ERROR; - } if (l->getParent() != nullptr) { - // If we have a parent, then we can continue to live as long as it does. - return NO_ERROR; } - return removeLayer(l); + // If we have a parent, then we can continue to live as long as it does. + return removeLayer(l, true); } // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index c89e26f798..9239538932 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -396,7 +396,7 @@ private: status_t onLayerDestroyed(const wp<Layer>& layer); // remove a layer from SurfaceFlinger immediately - status_t removeLayer(const sp<Layer>& layer); + status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false); // add a layer to SurfaceFlinger status_t addClientLayer(const sp<Client>& client, diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp index e19e021239..3d421d2154 100644 --- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp +++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp @@ -2339,12 +2339,20 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, return NO_ERROR; } -status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) { +status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly) { Mutex::Autolock _l(mStateLock); const auto& p = layer->getParent(); - const ssize_t index = (p != nullptr) ? p->removeChild(layer) : - mCurrentState.layersSortedByZ.remove(layer); + ssize_t index; + if (p != nullptr) { + if (topLevelOnly) { + return NO_ERROR; + } + + index = p->removeChild(layer); + } else { + index = mCurrentState.layersSortedByZ.remove(layer); + } // As a matter of normal operation, the LayerCleaner will produce a second // attempt to remove the surface. The Layer will be kept alive in mDrawingState @@ -2769,11 +2777,9 @@ status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer) if (l == nullptr) { // The layer has already been removed, carry on return NO_ERROR; - } if (l->getParent() != nullptr) { - // If we have a parent, then we can continue to live as long as it does. - return NO_ERROR; } - return removeLayer(l); + // If we have a parent, then we can continue to live as long as it does. + return removeLayer(l, true); } // --------------------------------------------------------------------------- |