SF: Fix tracing layerhandle to layerid map
Fixes an issue where the layer handle would be
added after it was used in a transaction by adding
the handle before constructing the create layer
transaction in SF.
Fixes an issue where the layer handle pointer
could be reused by a new handle by updating
the map when the handle is destroyed.
Test: presubmit
Bug: 200284593
Change-Id: Ia163e456500fc909bcda6e97dd8c7efa4be50c9e
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 0840241..9584592 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4422,10 +4422,6 @@
if (parentLayer != nullptr) {
addToRoot = false;
}
- result = addClientLayer(args.client, *outHandle, layer, parent, addToRoot, outTransformHint);
- if (result != NO_ERROR) {
- return result;
- }
int parentId = -1;
// We can safely promote the layer in binder thread because we have a strong reference
@@ -4439,6 +4435,11 @@
args.flags, parentId);
}
+ result = addClientLayer(args.client, *outHandle, layer, parent, addToRoot, outTransformHint);
+ if (result != NO_ERROR) {
+ return result;
+ }
+
setTransactionFlags(eTransactionNeeded);
*outLayerId = layer->sequence;
return result;
@@ -4521,6 +4522,9 @@
markLayerPendingRemovalLocked(layer);
mBufferCountTracker.remove(handle);
layer.clear();
+ if (mTransactionTracingEnabled) {
+ mTransactionTracing.onHandleRemoved(handle);
+ }
}
// ---------------------------------------------------------------------------
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.cpp b/services/surfaceflinger/Tracing/TransactionTracing.cpp
index 6dd43ca..c1b3d2e 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.cpp
+++ b/services/surfaceflinger/Tracing/TransactionTracing.cpp
@@ -199,7 +199,7 @@
entryProto.mutable_transactions()->Add(std::move(it->second));
mQueuedTransactions.erase(it);
} else {
- ALOGE("Could not find transaction id %" PRIu64, id);
+ ALOGW("Could not find transaction id %" PRIu64, id);
}
}
std::vector<proto::TransactionTraceEntry> entries = mBuffer->emplace(std::move(entryProto));
@@ -228,6 +228,9 @@
uint32_t flags, int parentId) {
std::scoped_lock lock(mTraceLock);
TracingLayerCreationArgs args{layerId, name, flags, parentId, -1 /* mirrorFromId */};
+ if (mLayerHandles.find(layerHandle) != mLayerHandles.end()) {
+ ALOGW("Duplicate handles found. %p", layerHandle);
+ }
mLayerHandles[layerHandle] = layerId;
proto::LayerCreationArgs protoArgs = TransactionProtoParser::toProto(args);
proto::LayerCreationArgs protoArgsCopy = protoArgs;
@@ -238,6 +241,9 @@
const std::string& name, int mirrorFromId) {
std::scoped_lock lock(mTraceLock);
TracingLayerCreationArgs args{layerId, name, 0 /* flags */, -1 /* parentId */, mirrorFromId};
+ if (mLayerHandles.find(layerHandle) != mLayerHandles.end()) {
+ ALOGW("Duplicate handles found. %p", layerHandle);
+ }
mLayerHandles[layerHandle] = layerId;
mCreatedLayers.emplace_back(TransactionProtoParser::toProto(args));
}
@@ -247,6 +253,11 @@
tryPushToTracingThread();
}
+void TransactionTracing::onHandleRemoved(BBinder* layerHandle) {
+ std::scoped_lock lock(mTraceLock);
+ mLayerHandles.erase(layerHandle);
+}
+
void TransactionTracing::tryPushToTracingThread() {
// Try to acquire the lock from main thread.
if (mMainThreadLock.try_lock()) {
@@ -270,7 +281,11 @@
return -1;
}
auto it = mLayerHandles.find(layerHandle->localBinder());
- return it == mLayerHandles.end() ? -1 : it->second;
+ if (it == mLayerHandles.end()) {
+ ALOGW("Could not find layer handle %p", layerHandle->localBinder());
+ return -1;
+ }
+ return it->second;
}
void TransactionTracing::updateStartingStateLocked(
@@ -288,7 +303,7 @@
for (const proto::LayerState& layerState : transaction.layer_changes()) {
auto it = mStartingStates.find(layerState.layer_id());
if (it == mStartingStates.end()) {
- ALOGE("Could not find layer id %d", layerState.layer_id());
+ ALOGW("Could not find layer id %d", layerState.layer_id());
continue;
}
TransactionProtoParser::fromProto(layerState, nullptr, it->second);
@@ -298,13 +313,6 @@
// Clean up stale starting states since the layer has been removed and the buffer does not
// contain any references to the layer.
for (const int32_t removedLayerId : removedEntry.removed_layers()) {
- auto it = std::find_if(mLayerHandles.begin(), mLayerHandles.end(),
- [removedLayerId](auto& layer) {
- return layer.second == removedLayerId;
- });
- if (it != mLayerHandles.end()) {
- mLayerHandles.erase(it);
- }
mStartingStates.erase(removedLayerId);
}
}
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.h b/services/surfaceflinger/Tracing/TransactionTracing.h
index 814f857..26a3758 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.h
+++ b/services/surfaceflinger/Tracing/TransactionTracing.h
@@ -67,6 +67,7 @@
void onMirrorLayerAdded(BBinder* layerHandle, int layerId, const std::string& name,
int mirrorFromId);
void onLayerRemoved(int layerId);
+ void onHandleRemoved(BBinder* layerHandle);
void dump(std::string&) const;
static constexpr auto CONTINUOUS_TRACING_BUFFER_SIZE = 512 * 1024;
static constexpr auto ACTIVE_TRACING_BUFFER_SIZE = 100 * 1024 * 1024;