diff options
-rw-r--r-- | services/surfaceflinger/Android.bp | 1 | ||||
-rw-r--r-- | services/surfaceflinger/FrontEnd/LayerHandle.cpp | 62 | ||||
-rw-r--r-- | services/surfaceflinger/FrontEnd/LayerHandle.h | 58 | ||||
-rw-r--r-- | services/surfaceflinger/Layer.cpp | 27 | ||||
-rw-r--r-- | services/surfaceflinger/Layer.h | 40 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.cpp | 40 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.h | 15 | ||||
-rw-r--r-- | services/surfaceflinger/fuzzer/surfaceflinger_fuzzer.cpp | 2 | ||||
-rw-r--r-- | services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h | 3 | ||||
-rw-r--r-- | services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h | 5 | ||||
-rw-r--r-- | services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp | 2 |
11 files changed, 156 insertions, 99 deletions
diff --git a/services/surfaceflinger/Android.bp b/services/surfaceflinger/Android.bp index b65f1b48e3..e76b191807 100644 --- a/services/surfaceflinger/Android.bp +++ b/services/surfaceflinger/Android.bp @@ -156,6 +156,7 @@ filegroup { "Effects/Daltonizer.cpp", "EventLog/EventLog.cpp", "FrontEnd/LayerCreationArgs.cpp", + "FrontEnd/LayerHandle.cpp", "FrontEnd/TransactionHandler.cpp", "FlagManager.cpp", "FpsReporter.cpp", diff --git a/services/surfaceflinger/FrontEnd/LayerHandle.cpp b/services/surfaceflinger/FrontEnd/LayerHandle.cpp new file mode 100644 index 0000000000..75e4e3ae11 --- /dev/null +++ b/services/surfaceflinger/FrontEnd/LayerHandle.cpp @@ -0,0 +1,62 @@ +/* + * Copyright 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "LayerHandle.h" +#include <cstdint> +#include "Layer.h" +#include "LayerCreationArgs.h" +#include "SurfaceFlinger.h" + +namespace android::surfaceflinger { + +LayerHandle::LayerHandle(const sp<android::SurfaceFlinger>& flinger, + const sp<android::Layer>& layer) + : mFlinger(flinger), mLayer(layer), mLayerId(static_cast<uint32_t>(layer->getSequence())) {} + +LayerHandle::~LayerHandle() { + if (mFlinger) { + mFlinger->onHandleDestroyed(this, mLayer, mLayerId); + } +} + +const String16 LayerHandle::kDescriptor = String16("android.Layer.LayerHandle"); + +sp<LayerHandle> LayerHandle::fromIBinder(const sp<IBinder>& binder) { + if (binder == nullptr) { + return nullptr; + } + + BBinder* b = binder->localBinder(); + if (b == nullptr || b->getInterfaceDescriptor() != LayerHandle::kDescriptor) { + ALOGD("handle does not have a valid descriptor"); + return nullptr; + } + + // We can safely cast this binder since its local and we verified its interface descriptor. + return sp<LayerHandle>::cast(binder); +} + +sp<android::Layer> LayerHandle::getLayer(const sp<IBinder>& binder) { + sp<LayerHandle> handle = LayerHandle::fromIBinder(binder); + return handle ? handle->mLayer : nullptr; +} + +uint32_t LayerHandle::getLayerId(const sp<IBinder>& binder) { + sp<LayerHandle> handle = LayerHandle::fromIBinder(binder); + return handle ? handle->mLayerId : UNASSIGNED_LAYER_ID; +} + +} // namespace android::surfaceflinger diff --git a/services/surfaceflinger/FrontEnd/LayerHandle.h b/services/surfaceflinger/FrontEnd/LayerHandle.h new file mode 100644 index 0000000000..5d0f783515 --- /dev/null +++ b/services/surfaceflinger/FrontEnd/LayerHandle.h @@ -0,0 +1,58 @@ +/* + * Copyright 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include <binder/Binder.h> +#include <utils/StrongPointer.h> + +namespace android { +class SurfaceFlinger; +class Layer; +} // namespace android + +namespace android::surfaceflinger { + +/* + * The layer handle is just a BBinder object passed to the client + * (remote process) -- we don't keep any reference on our side such that + * the dtor is called when the remote side let go of its reference. + * + * ~LayerHandle ensures that mFlinger->onLayerDestroyed() is called for + * this layer when the handle is destroyed. + */ +class LayerHandle : public BBinder { +public: + LayerHandle(const sp<android::SurfaceFlinger>& flinger, const sp<android::Layer>& layer); + // for testing + LayerHandle(uint32_t layerId) : mFlinger(nullptr), mLayer(nullptr), mLayerId(layerId) {} + ~LayerHandle(); + + // Static functions to access the layer and layer id safely from an incoming binder. + static sp<LayerHandle> fromIBinder(const sp<IBinder>& handle); + static sp<android::Layer> getLayer(const sp<IBinder>& handle); + static uint32_t getLayerId(const sp<IBinder>& handle); + static const String16 kDescriptor; + + const String16& getInterfaceDescriptor() const override { return kDescriptor; } + +private: + sp<android::SurfaceFlinger> mFlinger; + sp<android::Layer> mLayer; + const uint32_t mLayerId; +}; + +} // namespace android::surfaceflinger diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index b47188fea4..977709286b 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -70,6 +70,7 @@ #include "FrameTimeline.h" #include "FrameTracer/FrameTracer.h" #include "FrontEnd/LayerCreationArgs.h" +#include "FrontEnd/LayerHandle.h" #include "LayerProtoHelper.h" #include "SurfaceFlinger.h" #include "TimeStats/TimeStats.h" @@ -332,7 +333,7 @@ sp<IBinder> Layer::getHandle() { return nullptr; } mGetHandleCalled = true; - return sp<Handle>::make(mFlinger, sp<Layer>::fromExisting(this)); + return sp<LayerHandle>::make(mFlinger, sp<Layer>::fromExisting(this)); } // --------------------------------------------------------------------------- @@ -774,7 +775,7 @@ void Layer::setZOrderRelativeOf(const wp<Layer>& relativeOf) { } bool Layer::setRelativeLayer(const sp<IBinder>& relativeToHandle, int32_t relativeZ) { - sp<Layer> relative = fromHandle(relativeToHandle).promote(); + sp<Layer> relative = LayerHandle::getLayer(relativeToHandle); if (relative == nullptr) { return false; } @@ -1545,7 +1546,7 @@ void Layer::setChildrenDrawingParent(const sp<Layer>& newParent) { bool Layer::reparent(const sp<IBinder>& newParentHandle) { sp<Layer> newParent; if (newParentHandle != nullptr) { - newParent = fromHandle(newParentHandle).promote(); + newParent = LayerHandle::getLayer(newParentHandle); if (newParent == nullptr) { ALOGE("Unable to promote Layer handle"); return false; @@ -1938,7 +1939,8 @@ void Layer::commitChildList() { void Layer::setInputInfo(const WindowInfo& info) { mDrawingState.inputInfo = info; - mDrawingState.touchableRegionCrop = fromHandle(info.touchableRegionCropHandle.promote()); + mDrawingState.touchableRegionCrop = + LayerHandle::getLayer(info.touchableRegionCropHandle.promote()); mDrawingState.modified = true; mFlinger->mUpdateInputInfo = true; setTransactionFlags(eTransactionNeeded); @@ -2585,23 +2587,6 @@ void Layer::setClonedChild(const sp<Layer>& clonedChild) { mFlinger->mNumClones++; } -const String16 Layer::Handle::kDescriptor = String16("android.Layer.Handle"); - -wp<Layer> Layer::fromHandle(const sp<IBinder>& handleBinder) { - if (handleBinder == nullptr) { - return nullptr; - } - - BBinder* b = handleBinder->localBinder(); - if (b == nullptr || b->getInterfaceDescriptor() != Handle::kDescriptor) { - return nullptr; - } - - // We can safely cast this binder since its local and we verified its interface descriptor. - sp<Handle> handle = sp<Handle>::cast(handleBinder); - return handle->owner; -} - bool Layer::setDropInputMode(gui::DropInputMode mode) { if (mDrawingState.dropInputMode == mode) { return false; diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 336b4ff410..a3c4e5966c 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -226,46 +226,6 @@ public: bool dimmingEnabled = true; }; - /* - * Trivial class, used to ensure that mFlinger->onLayerDestroyed(mLayer) - * is called. - */ - class LayerCleaner { - sp<SurfaceFlinger> mFlinger; - sp<Layer> mLayer; - BBinder* mHandle; - - protected: - ~LayerCleaner() { - // destroy client resources - mFlinger->onHandleDestroyed(mHandle, mLayer); - } - - public: - LayerCleaner(const sp<SurfaceFlinger>& flinger, const sp<Layer>& layer, BBinder* handle) - : mFlinger(flinger), mLayer(layer), mHandle(handle) {} - }; - - /* - * The layer handle is just a BBinder object passed to the client - * (remote process) -- we don't keep any reference on our side such that - * the dtor is called when the remote side let go of its reference. - * - * LayerCleaner ensures that mFlinger->onLayerDestroyed() is called for - * this layer when the handle is destroyed. - */ - class Handle : public BBinder, public LayerCleaner { - public: - Handle(const sp<SurfaceFlinger>& flinger, const sp<Layer>& layer) - : LayerCleaner(flinger, layer, this), owner(layer) {} - const String16& getInterfaceDescriptor() const override { return kDescriptor; } - - static const String16 kDescriptor; - wp<Layer> owner; - }; - - static wp<Layer> fromHandle(const sp<IBinder>& handle); - explicit Layer(const LayerCreationArgs& args); virtual ~Layer(); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index c1eda1793d..cfebec70cb 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -124,6 +124,7 @@ #include "FrameTimeline/FrameTimeline.h" #include "FrameTracer/FrameTracer.h" #include "FrontEnd/LayerCreationArgs.h" +#include "FrontEnd/LayerHandle.h" #include "HdrLayerInfoReporter.h" #include "Layer.h" #include "LayerProtoHelper.h" @@ -1577,7 +1578,10 @@ status_t SurfaceFlinger::addRegionSamplingListener(const Rect& samplingArea, return BAD_VALUE; } - const wp<Layer> stopLayer = fromHandle(stopLayerHandle); + // LayerHandle::getLayer promotes the layer object in a binder thread but we will not destroy + // the layer here since the caller has a strong ref to the layer's handle. + // TODO (b/238781169): replace layer with layer id + const wp<Layer> stopLayer = LayerHandle::getLayer(stopLayerHandle); mRegionSamplingThread->addListener(samplingArea, stopLayer, listener); return NO_ERROR; } @@ -3698,7 +3702,7 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC using TransactionReadiness = TransactionHandler::TransactionReadiness; auto ready = TransactionReadiness::Ready; flushState.transaction->traverseStatesWithBuffersWhileTrue([&](const layer_state_t& s) -> bool { - sp<Layer> layer = Layer::fromHandle(s.surface).promote(); + sp<Layer> layer = LayerHandle::getLayer(s.surface); const auto& transaction = *flushState.transaction; // check for barrier frames if (s.bufferData->hasBarrier && @@ -3966,7 +3970,7 @@ bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin setClientStateLocked(frameTimelineInfo, state, desiredPresentTime, isAutoTimestamp, postTime, permissions, transactionId); if ((flags & eAnimation) && state.state.surface) { - if (const auto layer = fromHandle(state.state.surface).promote()) { + if (const auto layer = LayerHandle::getLayer(state.state.surface)) { using LayerUpdateType = scheduler::LayerHistory::LayerUpdateType; mScheduler->recordLayerHistory(layer.get(), isAutoTimestamp ? 0 : desiredPresentTime, @@ -4107,7 +4111,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime uint32_t flags = 0; sp<Layer> layer = nullptr; if (s.surface) { - layer = fromHandle(s.surface).promote(); + layer = LayerHandle::getLayer(s.surface); } else { // The client may provide us a null handle. Treat it as if the layer was removed. ALOGW("Attempt to set client state with a null layer handle"); @@ -4414,7 +4418,7 @@ status_t SurfaceFlinger::mirrorLayer(const LayerCreationArgs& args, LayerCreationArgs mirrorArgs(args); { Mutex::Autolock _l(mStateLock); - mirrorFrom = fromHandle(mirrorFromHandle).promote(); + mirrorFrom = LayerHandle::getLayer(mirrorFromHandle); if (!mirrorFrom) { return NAME_NOT_FOUND; } @@ -4520,19 +4524,15 @@ status_t SurfaceFlinger::createLayer(LayerCreationArgs& args, gui::CreateSurface } args.addToRoot = args.addToRoot && callingThreadHasUnscopedSurfaceFlingerAccess(); - wp<Layer> parent = fromHandle(args.parentHandle.promote()); + // We can safely promote the parent layer in binder thread because we have a strong reference + // to the layer's handle inside this scope. + sp<Layer> parent = LayerHandle::getLayer(args.parentHandle.promote()); if (args.parentHandle != nullptr && parent == nullptr) { - ALOGE("Invalid parent handle %p.", args.parentHandle.promote().get()); + ALOGE("Invalid parent handle %p", args.parentHandle.promote().get()); args.addToRoot = false; } - int parentId = -1; - // We can safely promote the layer in binder thread because we have a strong reference - // to the layer's handle inside this scope or we were passed in a sp reference to the layer. - sp<Layer> parentSp = parent.promote(); - if (parentSp != nullptr) { - parentId = parentSp->getSequence(); - } + const int parentId = parent ? parent->getSequence() : -1; if (mTransactionTracing) { mTransactionTracing->onLayerAdded(outResult.handle->localBinder(), layer->sequence, args.name, args.flags, parentId); @@ -4571,7 +4571,7 @@ void SurfaceFlinger::markLayerPendingRemovalLocked(const sp<Layer>& layer) { setTransactionFlags(eTransactionNeeded); } -void SurfaceFlinger::onHandleDestroyed(BBinder* handle, sp<Layer>& layer) { +void SurfaceFlinger::onHandleDestroyed(BBinder* handle, sp<Layer>& layer, uint32_t /* layerId */) { Mutex::Autolock lock(mStateLock); markLayerPendingRemovalLocked(layer); mBufferCountTracker.remove(handle); @@ -6176,7 +6176,7 @@ status_t SurfaceFlinger::captureLayers(const LayerCaptureArgs& args, { Mutex::Autolock lock(mStateLock); - parent = fromHandle(args.layerHandle).promote(); + parent = LayerHandle::getLayer(args.layerHandle); if (parent == nullptr) { ALOGE("captureLayers called with an invalid or removed parent"); return NAME_NOT_FOUND; @@ -6207,7 +6207,7 @@ status_t SurfaceFlinger::captureLayers(const LayerCaptureArgs& args, reqSize = ui::Size(crop.width() * args.frameScaleX, crop.height() * args.frameScaleY); for (const auto& handle : args.excludeHandles) { - sp<Layer> excludeLayer = fromHandle(handle).promote(); + sp<Layer> excludeLayer = LayerHandle::getLayer(handle); if (excludeLayer != nullptr) { excludeLayers.emplace(excludeLayer); } else { @@ -6723,10 +6723,6 @@ status_t SurfaceFlinger::getDesiredDisplayModeSpecs(const sp<IBinder>& displayTo return NO_ERROR; } -wp<Layer> SurfaceFlinger::fromHandle(const sp<IBinder>& handle) const { - return Layer::fromHandle(handle); -} - void SurfaceFlinger::onLayerFirstRef(Layer* layer) { mNumLayers++; if (!layer->isRemovedFromCurrentState()) { @@ -7015,7 +7011,7 @@ bool SurfaceFlinger::commitMirrorDisplays(VsyncId vsyncId) { for (const auto& mirrorDisplay : mirrorDisplays) { // Set mirror layer's default layer stack to -1 so it doesn't end up rendered on a display // accidentally. - sp<Layer> rootMirrorLayer = Layer::fromHandle(mirrorDisplay.rootHandle).promote(); + sp<Layer> rootMirrorLayer = LayerHandle::getLayer(mirrorDisplay.rootHandle); rootMirrorLayer->setLayerStack(ui::LayerStack::fromValue(-1)); for (const auto& layer : mDrawingState.layersSortedByZ) { if (layer->getLayerStack() != mirrorDisplay.layerStack || diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 9ffe6abbe6..85c194bbb5 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -273,6 +273,11 @@ public: void removeHierarchyFromOffscreenLayers(Layer* layer); void removeFromOffscreenLayers(Layer* layer); + // Called when all clients have released all their references to + // this layer. The layer may still be kept alive by its parents but + // the client can no longer modify this layer directly. + void onHandleDestroyed(BBinder* handle, sp<Layer>& layer, uint32_t layerId); + // TODO: Remove atomic if move dtor to main thread CL lands std::atomic<uint32_t> mNumClones; @@ -280,12 +285,6 @@ public: return mTransactionCallbackInvoker; } - // Converts from a binder handle to a Layer - // Returns nullptr if the handle does not point to an existing layer. - // Otherwise, returns a weak reference so that callers off the main-thread - // won't accidentally hold onto the last strong reference. - wp<Layer> fromHandle(const sp<IBinder>& handle) const; - // If set, disables reusing client composition buffers. This can be set by // debug.sf.disable_client_composition_cache bool mDisableClientCompositionCache = false; @@ -768,10 +767,6 @@ private: status_t mirrorDisplay(DisplayId displayId, const LayerCreationArgs& args, gui::CreateSurfaceResult& outResult); - // called when all clients have released all their references to - // this layer meaning it is entirely safe to destroy all - // resources associated to this layer. - void onHandleDestroyed(BBinder* handle, sp<Layer>& layer); void markLayerPendingRemovalLocked(const sp<Layer>& layer); // add a layer to SurfaceFlinger diff --git a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzer.cpp b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzer.cpp index 22d80ca3db..14384a7028 100644 --- a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzer.cpp +++ b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzer.cpp @@ -150,7 +150,7 @@ void SurfaceFlingerFuzzer::invokeFlinger() { sp<IBinder> handle = defaultServiceManager()->checkService( String16(mFdp.ConsumeRandomLengthString().c_str())); - mFlinger->fromHandle(handle); + LayerHandle::getLayer(handle); mFlinger->disableExpensiveRendering(); } diff --git a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h index 99279dce8b..e55586774f 100644 --- a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h +++ b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h @@ -34,6 +34,7 @@ #include "DisplayHardware/ComposerHal.h" #include "FrameTimeline/FrameTimeline.h" #include "FrameTracer/FrameTracer.h" +#include "FrontEnd/LayerHandle.h" #include "Layer.h" #include "NativeWindowSurface.h" #include "Scheduler/EventThread.h" @@ -766,7 +767,7 @@ public: auto& mutableDisplays() { return mFlinger->mDisplays; } auto& mutableDrawingState() { return mFlinger->mDrawingState; } - auto fromHandle(const sp<IBinder> &handle) { return mFlinger->fromHandle(handle); } + auto fromHandle(const sp<IBinder> &handle) { return LayerHandle::getLayer(handle); } ~TestableSurfaceFlinger() { mutableDisplays().clear(); diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 89812aad7d..7f471bc8b8 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -34,6 +34,7 @@ #include "FakeVsyncConfiguration.h" #include "FrameTracer/FrameTracer.h" #include "FrontEnd/LayerCreationArgs.h" +#include "FrontEnd/LayerHandle.h" #include "Layer.h" #include "NativeWindowSurface.h" #include "Scheduler/MessageQueue.h" @@ -530,9 +531,7 @@ public: auto& mutablePrimaryHwcDisplayId() { return getHwComposer().mPrimaryHwcDisplayId; } auto& mutableActiveDisplayId() { return mFlinger->mActiveDisplayId; } - auto fromHandle(const sp<IBinder>& handle) { - return mFlinger->fromHandle(handle); - } + auto fromHandle(const sp<IBinder>& handle) { return LayerHandle::getLayer(handle); } ~TestableSurfaceFlinger() { // All these pointer and container clears help ensure that GMock does diff --git a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp index de84faa5d2..9888f002fb 100644 --- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp @@ -294,7 +294,7 @@ TEST_F(TransactionApplicationTest, PlaceOnTransactionQueue_SyncInputWindows) { TEST_F(TransactionApplicationTest, FromHandle) { sp<IBinder> badHandle; auto ret = mFlinger.fromHandle(badHandle); - EXPECT_EQ(nullptr, ret.promote().get()); + EXPECT_EQ(nullptr, ret.get()); } class LatchUnsignaledTest : public TransactionApplicationTest { |