SurfaceFlinger: add more thread-safety annotations.
As part of debugging a crash in SurfaceFlinger, we became suspicious of
markLayerPendingRemovalLocked because it wasn't annotated with thread
safety annotations. This ended up being a red herring, because
mStateLock is held on all paths into that function. Add some helpers to
MutexUtils.h to require/assert mutexes, and use them to sprinkle some
thread safety annotations to make things more clear.
Cherry-picked from aosp/2212955
Bug: 271644537
Test: m surfaceflinger
Change-Id: Ib48433765bb6ffcb351ec10cd5bc89e254b48545
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 755e585..8dec57b 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -75,6 +75,7 @@
#include "FrontEnd/LayerCreationArgs.h"
#include "FrontEnd/LayerHandle.h"
#include "LayerProtoHelper.h"
+#include "MutexUtils.h"
#include "SurfaceFlinger.h"
#include "TimeStats/TimeStats.h"
#include "TunnelModeEnabledReporter.h"
@@ -305,10 +306,12 @@
auto layersInTree = getRootLayer()->getLayersInTree(LayerVector::StateSet::Current);
std::sort(layersInTree.begin(), layersInTree.end());
- traverse(LayerVector::StateSet::Current, [&](Layer* layer) {
- layer->removeFromCurrentState();
- layer->removeRelativeZ(layersInTree);
- });
+ REQUIRE_MUTEX(mFlinger->mStateLock);
+ traverse(LayerVector::StateSet::Current,
+ [&](Layer* layer) REQUIRES(layer->mFlinger->mStateLock) {
+ layer->removeFromCurrentState();
+ layer->removeRelativeZ(layersInTree);
+ });
}
void Layer::addToCurrentState() {
@@ -1009,10 +1012,12 @@
mFlinger->mLayersAdded = true;
// set up SF to handle added color layer
if (isRemovedFromCurrentState()) {
+ MUTEX_ALIAS(mFlinger->mStateLock, mDrawingState.bgColorLayer->mFlinger->mStateLock);
mDrawingState.bgColorLayer->onRemovedFromCurrentState();
}
mFlinger->setTransactionFlags(eTransactionNeeded);
} else if (mDrawingState.bgColorLayer && alpha == 0) {
+ MUTEX_ALIAS(mFlinger->mStateLock, mDrawingState.bgColorLayer->mFlinger->mStateLock);
mDrawingState.bgColorLayer->reparent(nullptr);
mDrawingState.bgColorLayer = nullptr;
return true;
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 1af648a..0bfab7c 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -290,7 +290,7 @@
virtual bool setMetadata(const LayerMetadata& data);
virtual void setChildrenDrawingParent(const sp<Layer>&);
- virtual bool reparent(const sp<IBinder>& newParentHandle);
+ virtual bool reparent(const sp<IBinder>& newParentHandle) REQUIRES(mFlinger->mStateLock);
virtual bool setColorTransform(const mat4& matrix);
virtual mat4 getColorTransform() const;
virtual bool hasColorTransform() const;
@@ -316,7 +316,8 @@
bool setSidebandStream(const sp<NativeHandle>& /*sidebandStream*/);
bool setTransactionCompletedListeners(const std::vector<sp<CallbackHandle>>& /*handles*/,
bool willPresent);
- virtual bool setBackgroundColor(const half3& color, float alpha, ui::Dataspace dataspace);
+ virtual bool setBackgroundColor(const half3& color, float alpha, ui::Dataspace dataspace)
+ REQUIRES(mFlinger->mStateLock);
virtual bool setColorSpaceAgnostic(const bool agnostic);
virtual bool setDimmingEnabled(const bool dimmingEnabled);
virtual bool setDefaultFrameRateCompatibility(FrameRateCompatibility compatibility);
@@ -641,13 +642,13 @@
/*
* Remove from current state and mark for removal.
*/
- void removeFromCurrentState();
+ void removeFromCurrentState() REQUIRES(mFlinger->mStateLock);
/*
* called with the state lock from a binder thread when the layer is
* removed from the current list to the pending removal list
*/
- void onRemovedFromCurrentState();
+ void onRemovedFromCurrentState() REQUIRES(mFlinger->mStateLock);
/*
* Called when the layer is added back to the current state list.
@@ -880,6 +881,9 @@
mTransformHint = transformHint;
}
+ // Exposed so SurfaceFlinger can assert that it's held
+ const sp<SurfaceFlinger> mFlinger;
+
protected:
// For unit tests
friend class TestableSurfaceFlinger;
@@ -941,9 +945,6 @@
*/
std::pair<FloatRect, bool> getInputBounds(bool fillParentBounds) const;
- // constant
- sp<SurfaceFlinger> mFlinger;
-
bool mPremultipliedAlpha{true};
const std::string mName;
const std::string mTransactionName{"TX - " + mName};
diff --git a/services/surfaceflinger/MutexUtils.h b/services/surfaceflinger/MutexUtils.h
index f8be6f3..58f7cb4 100644
--- a/services/surfaceflinger/MutexUtils.h
+++ b/services/surfaceflinger/MutexUtils.h
@@ -50,4 +50,14 @@
const status_t status;
};
+// Require, under penalty of compilation failure, that the compiler thinks that a mutex is held.
+#define REQUIRE_MUTEX(expr) ([]() REQUIRES(expr) {})()
+
+// Tell the compiler that we know that a mutex is held.
+#define ASSERT_MUTEX(expr) ([]() ASSERT_CAPABILITY(expr) {})()
+
+// Specify that one mutex is an alias for another.
+// (e.g. SurfaceFlinger::mStateLock and Layer::mFlinger->mStateLock)
+#define MUTEX_ALIAS(held, alias) (REQUIRE_MUTEX(held), ASSERT_MUTEX(alias))
+
} // namespace android
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9ceb0af..a538335 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4769,6 +4769,7 @@
}
return 0;
}
+ MUTEX_ALIAS(mStateLock, layer->mFlinger->mStateLock);
ui::LayerStack oldLayerStack = layer->getLayerStack(LayerVector::StateSet::Current);
@@ -7739,6 +7740,7 @@
ALOGD("Layer was destroyed soon after creation %p", state.layer.unsafe_get());
return;
}
+ MUTEX_ALIAS(mStateLock, layer->mFlinger->mStateLock);
sp<Layer> parent;
bool addToRoot = state.addToRoot;
@@ -7916,6 +7918,7 @@
{
Mutex::Autolock lock(mStateLock);
createEffectLayer(mirrorArgs, &unused, &childMirror);
+ MUTEX_ALIAS(mStateLock, childMirror->mFlinger->mStateLock);
childMirror->setClonedChild(layer->createClone());
childMirror->reparent(mirrorDisplay.rootHandle);
}
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 74d00dd..834c8b6 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -797,7 +797,7 @@
status_t mirrorDisplay(DisplayId displayId, const LayerCreationArgs& args,
gui::CreateSurfaceResult& outResult);
- void markLayerPendingRemovalLocked(const sp<Layer>& layer);
+ void markLayerPendingRemovalLocked(const sp<Layer>& layer) REQUIRES(mStateLock);
// add a layer to SurfaceFlinger
status_t addClientLayer(LayerCreationArgs& args, const sp<IBinder>& handle,