Fix deadlock when calling dumpsys surfaceflinger
We were holding the state lock when calling into the main thread to
access some debug data. The main thread was stuck waiting for the
state lock, causing the deadlock.
Fix by cleaning up the surface flinger dumpsys and reduce the scope
of the state lock.
Fixes: 313910768, 300525768
Test: dumpsys SurfaceFlinger
Test: dumpsys SurfaceFlinger --hwclayers
Test: dumpsys SurfaceFlinger --frontend
Test: dumpsys SurfaceFlinger --timestats --proto
Change-Id: Id62b9427a14bc37bc92759fbaa8ca2b3da94905d
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 8311617..1c0220b 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -5951,140 +5951,60 @@
!PermissionCache::checkPermission(sDump, pid, uid)) {
StringAppendF(&result, "Permission Denial: can't dump SurfaceFlinger from pid=%d, uid=%d\n",
pid, uid);
- } else {
- Dumper hwclayersDump = [this](const DumpArgs&, bool, std::string& result)
- FTL_FAKE_GUARD(mStateLock) -> void const {
- if (mLayerLifecycleManagerEnabled) {
- mScheduler
- ->schedule([this, &result]() FTL_FAKE_GUARD(kMainThreadContext)
- FTL_FAKE_GUARD(mStateLock) {
- dumpHwcLayersMinidump(result);
- })
- .get();
- } else {
- dumpHwcLayersMinidumpLockedLegacy(result);
- }
- };
-
- static const std::unordered_map<std::string, Dumper> dumpers = {
- {"--comp-displays"s, dumper(&SurfaceFlinger::dumpCompositionDisplays)},
- {"--display-id"s, dumper(&SurfaceFlinger::dumpDisplayIdentificationData)},
- {"--displays"s, dumper(&SurfaceFlinger::dumpDisplays)},
- {"--edid"s, argsDumper(&SurfaceFlinger::dumpRawDisplayIdentificationData)},
- {"--events"s, dumper(&SurfaceFlinger::dumpEvents)},
- {"--frametimeline"s, argsDumper(&SurfaceFlinger::dumpFrameTimeline)},
- {"--hdrinfo"s, dumper(&SurfaceFlinger::dumpHdrInfo)},
- {"--hwclayers"s, std::move(hwclayersDump)},
- {"--latency"s, argsDumper(&SurfaceFlinger::dumpStatsLocked)},
- {"--latency-clear"s, argsDumper(&SurfaceFlinger::clearStatsLocked)},
- {"--list"s, dumper(&SurfaceFlinger::listLayersLocked)},
- {"--planner"s, argsDumper(&SurfaceFlinger::dumpPlannerInfo)},
- {"--scheduler"s, dumper(&SurfaceFlinger::dumpScheduler)},
- {"--timestats"s, protoDumper(&SurfaceFlinger::dumpTimeStats)},
- {"--vsync"s, dumper(&SurfaceFlinger::dumpVsync)},
- {"--wide-color"s, dumper(&SurfaceFlinger::dumpWideColorInfo)},
- {"--frontend"s, dumper(&SurfaceFlinger::dumpFrontEnd)},
- };
-
- const auto flag = args.empty() ? ""s : std::string(String8(args[0]));
-
- // Traversal of drawing state must happen on the main thread.
- // Otherwise, SortedVector may have shared ownership during concurrent
- // traversals, which can result in use-after-frees.
- std::string compositionLayers;
- mScheduler
- ->schedule([&]() FTL_FAKE_GUARD(mStateLock) FTL_FAKE_GUARD(kMainThreadContext) {
- if (!mLayerLifecycleManagerEnabled) {
- StringAppendF(&compositionLayers, "Composition layers\n");
- mDrawingState.traverseInZOrder([&](Layer* layer) {
- auto* compositionState = layer->getCompositionState();
- if (!compositionState || !compositionState->isVisible) return;
- android::base::StringAppendF(&compositionLayers, "* Layer %p (%s)\n",
- layer,
- layer->getDebugName()
- ? layer->getDebugName()
- : "<unknown>");
- compositionState->dump(compositionLayers);
- });
- } else {
- std::ostringstream out;
- out << "\nComposition list\n";
- ui::LayerStack lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
- mLayerSnapshotBuilder.forEachVisibleSnapshot(
- [&](std::unique_ptr<frontend::LayerSnapshot>& snapshot) {
- if (snapshot->hasSomethingToDraw()) {
- if (lastPrintedLayerStackHeader !=
- snapshot->outputFilter.layerStack) {
- lastPrintedLayerStackHeader =
- snapshot->outputFilter.layerStack;
- out << "LayerStack=" << lastPrintedLayerStackHeader.id
- << "\n";
- }
- out << " " << *snapshot << "\n";
- }
- });
-
- out << "\nInput list\n";
- lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
- mLayerSnapshotBuilder.forEachInputSnapshot(
- [&](const frontend::LayerSnapshot& snapshot) {
- if (lastPrintedLayerStackHeader !=
- snapshot.outputFilter.layerStack) {
- lastPrintedLayerStackHeader =
- snapshot.outputFilter.layerStack;
- out << "LayerStack=" << lastPrintedLayerStackHeader.id
- << "\n";
- }
- out << " " << snapshot << "\n";
- });
-
- out << "\nLayer Hierarchy\n"
- << mLayerHierarchyBuilder.getHierarchy()
- << "\nOffscreen Hierarchy\n"
- << mLayerHierarchyBuilder.getOffscreenHierarchy() << "\n\n";
- compositionLayers = out.str();
- dumpHwcLayersMinidump(compositionLayers);
- }
- })
- .get();
-
- bool dumpLayers = true;
- {
- TimedLock lock(mStateLock, s2ns(1), __func__);
- if (!lock.locked()) {
- StringAppendF(&result, "Dumping without lock after timeout: %s (%d)\n",
- strerror(-lock.status), lock.status);
- }
-
- if (const auto it = dumpers.find(flag); it != dumpers.end()) {
- (it->second)(args, asProto, result);
- dumpLayers = false;
- } else if (!asProto) {
- dumpAllLocked(args, compositionLayers, result);
- }
- }
-
- if (dumpLayers) {
- perfetto::protos::LayersTraceFileProto traceFileProto =
- mLayerTracing.createTraceFileProto();
- perfetto::protos::LayersSnapshotProto* layersTrace = traceFileProto.add_entry();
- perfetto::protos::LayersProto layersProto = dumpProtoFromMainThread();
- layersTrace->mutable_layers()->Swap(&layersProto);
- auto displayProtos = dumpDisplayProto();
- layersTrace->mutable_displays()->Swap(&displayProtos);
-
- if (asProto) {
- result.append(traceFileProto.SerializeAsString());
- } else {
- // Dump info that we need to access from the main thread
- const auto layerTree = LayerProtoParser::generateLayerTree(layersTrace->layers());
- result.append(LayerProtoParser::layerTreeToString(layerTree));
- result.append("\n");
- dumpOffscreenLayers(result);
- }
- }
+ write(fd, result.c_str(), result.size());
+ return NO_ERROR;
}
+ if (asProto && args.empty()) {
+ perfetto::protos::LayersTraceFileProto traceFileProto =
+ mLayerTracing.createTraceFileProto();
+ perfetto::protos::LayersSnapshotProto* layersTrace = traceFileProto.add_entry();
+ perfetto::protos::LayersProto layersProto = dumpProtoFromMainThread();
+ layersTrace->mutable_layers()->Swap(&layersProto);
+ auto displayProtos = dumpDisplayProto();
+ layersTrace->mutable_displays()->Swap(&displayProtos);
+ result.append(traceFileProto.SerializeAsString());
+ write(fd, result.c_str(), result.size());
+ return NO_ERROR;
+ }
+
+ static const std::unordered_map<std::string, Dumper> dumpers = {
+ {"--comp-displays"s, dumper(&SurfaceFlinger::dumpCompositionDisplays)},
+ {"--display-id"s, dumper(&SurfaceFlinger::dumpDisplayIdentificationData)},
+ {"--displays"s, dumper(&SurfaceFlinger::dumpDisplays)},
+ {"--edid"s, argsDumper(&SurfaceFlinger::dumpRawDisplayIdentificationData)},
+ {"--events"s, dumper(&SurfaceFlinger::dumpEvents)},
+ {"--frametimeline"s, argsDumper(&SurfaceFlinger::dumpFrameTimeline)},
+ {"--frontend"s, mainThreadDumper(&SurfaceFlinger::dumpFrontEnd)},
+ {"--hdrinfo"s, dumper(&SurfaceFlinger::dumpHdrInfo)},
+ {"--hwclayers"s, mainThreadDumper(&SurfaceFlinger::dumpHwcLayersMinidump)},
+ {"--latency"s, argsDumper(&SurfaceFlinger::dumpStatsLocked)},
+ {"--latency-clear"s, argsDumper(&SurfaceFlinger::clearStatsLocked)},
+ {"--list"s, dumper(&SurfaceFlinger::listLayersLocked)},
+ {"--planner"s, argsDumper(&SurfaceFlinger::dumpPlannerInfo)},
+ {"--scheduler"s, dumper(&SurfaceFlinger::dumpScheduler)},
+ {"--timestats"s, protoDumper(&SurfaceFlinger::dumpTimeStats)},
+ {"--vsync"s, dumper(&SurfaceFlinger::dumpVsync)},
+ {"--wide-color"s, dumper(&SurfaceFlinger::dumpWideColorInfo)},
+ };
+
+ const auto flag = args.empty() ? ""s : std::string(String8(args[0]));
+ if (const auto it = dumpers.find(flag); it != dumpers.end()) {
+ (it->second)(args, asProto, result);
+ write(fd, result.c_str(), result.size());
+ return NO_ERROR;
+ }
+
+ // Traversal of drawing state must happen on the main thread.
+ // Otherwise, SortedVector may have shared ownership during concurrent
+ // traversals, which can result in use-after-frees.
+ std::string compositionLayers;
+ mScheduler
+ ->schedule([&]() FTL_FAKE_GUARD(mStateLock) FTL_FAKE_GUARD(kMainThreadContext) {
+ dumpVisibleFrontEnd(compositionLayers);
+ })
+ .get();
+ dumpAll(args, compositionLayers, result);
write(fd, result.c_str(), result.size());
return NO_ERROR;
}
@@ -6296,37 +6216,81 @@
}
void SurfaceFlinger::dumpFrontEnd(std::string& result) {
- mScheduler
- ->schedule([&]() FTL_FAKE_GUARD(mStateLock) FTL_FAKE_GUARD(kMainThreadContext) {
- std::ostringstream out;
- out << "\nComposition list\n";
- ui::LayerStack lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
- for (const auto& snapshot : mLayerSnapshotBuilder.getSnapshots()) {
- if (lastPrintedLayerStackHeader != snapshot->outputFilter.layerStack) {
- lastPrintedLayerStackHeader = snapshot->outputFilter.layerStack;
- out << "LayerStack=" << lastPrintedLayerStackHeader.id << "\n";
+ std::ostringstream out;
+ out << "\nComposition list\n";
+ ui::LayerStack lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
+ for (const auto& snapshot : mLayerSnapshotBuilder.getSnapshots()) {
+ if (lastPrintedLayerStackHeader != snapshot->outputFilter.layerStack) {
+ lastPrintedLayerStackHeader = snapshot->outputFilter.layerStack;
+ out << "LayerStack=" << lastPrintedLayerStackHeader.id << "\n";
+ }
+ out << " " << *snapshot << "\n";
+ }
+
+ out << "\nInput list\n";
+ lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
+ mLayerSnapshotBuilder.forEachInputSnapshot([&](const frontend::LayerSnapshot& snapshot) {
+ if (lastPrintedLayerStackHeader != snapshot.outputFilter.layerStack) {
+ lastPrintedLayerStackHeader = snapshot.outputFilter.layerStack;
+ out << "LayerStack=" << lastPrintedLayerStackHeader.id << "\n";
+ }
+ out << " " << snapshot << "\n";
+ });
+
+ out << "\nLayer Hierarchy\n"
+ << mLayerHierarchyBuilder.getHierarchy().dump() << "\nOffscreen Hierarchy\n"
+ << mLayerHierarchyBuilder.getOffscreenHierarchy().dump() << "\n\n";
+ result.append(out.str());
+}
+
+void SurfaceFlinger::dumpVisibleFrontEnd(std::string& result) {
+ if (!mLayerLifecycleManagerEnabled) {
+ StringAppendF(&result, "Composition layers\n");
+ mDrawingState.traverseInZOrder([&](Layer* layer) {
+ auto* compositionState = layer->getCompositionState();
+ if (!compositionState || !compositionState->isVisible) return;
+ android::base::StringAppendF(&result, "* Layer %p (%s)\n", layer,
+ layer->getDebugName() ? layer->getDebugName()
+ : "<unknown>");
+ compositionState->dump(result);
+ });
+
+ StringAppendF(&result, "Offscreen Layers\n");
+ for (Layer* offscreenLayer : mOffscreenLayers) {
+ offscreenLayer->traverse(LayerVector::StateSet::Drawing,
+ [&](Layer* layer) { layer->dumpOffscreenDebugInfo(result); });
+ }
+ } else {
+ std::ostringstream out;
+ out << "\nComposition list\n";
+ ui::LayerStack lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
+ mLayerSnapshotBuilder.forEachVisibleSnapshot(
+ [&](std::unique_ptr<frontend::LayerSnapshot>& snapshot) {
+ if (snapshot->hasSomethingToDraw()) {
+ if (lastPrintedLayerStackHeader != snapshot->outputFilter.layerStack) {
+ lastPrintedLayerStackHeader = snapshot->outputFilter.layerStack;
+ out << "LayerStack=" << lastPrintedLayerStackHeader.id << "\n";
+ }
+ out << " " << *snapshot << "\n";
}
- out << " " << *snapshot << "\n";
- }
+ });
- out << "\nInput list\n";
- lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
- mLayerSnapshotBuilder.forEachInputSnapshot(
- [&](const frontend::LayerSnapshot& snapshot) {
- if (lastPrintedLayerStackHeader != snapshot.outputFilter.layerStack) {
- lastPrintedLayerStackHeader = snapshot.outputFilter.layerStack;
- out << "LayerStack=" << lastPrintedLayerStackHeader.id << "\n";
- }
- out << " " << snapshot << "\n";
- });
+ out << "\nInput list\n";
+ lastPrintedLayerStackHeader = ui::INVALID_LAYER_STACK;
+ mLayerSnapshotBuilder.forEachInputSnapshot([&](const frontend::LayerSnapshot& snapshot) {
+ if (lastPrintedLayerStackHeader != snapshot.outputFilter.layerStack) {
+ lastPrintedLayerStackHeader = snapshot.outputFilter.layerStack;
+ out << "LayerStack=" << lastPrintedLayerStackHeader.id << "\n";
+ }
+ out << " " << snapshot << "\n";
+ });
- out << "\nLayer Hierarchy\n"
- << mLayerHierarchyBuilder.getHierarchy().dump()
- << "\nOffscreen Hierarchy\n"
- << mLayerHierarchyBuilder.getOffscreenHierarchy().dump() << "\n\n";
- result.append(out.str());
- })
- .get();
+ out << "\nLayer Hierarchy\n"
+ << mLayerHierarchyBuilder.getHierarchy() << "\nOffscreen Hierarchy\n"
+ << mLayerHierarchyBuilder.getOffscreenHierarchy() << "\n\n";
+ result = out.str();
+ dumpHwcLayersMinidump(result);
+ }
}
perfetto::protos::LayersProto SurfaceFlinger::dumpDrawingStateProto(uint32_t traceFlags) const {
@@ -6427,7 +6391,7 @@
}
void SurfaceFlinger::dumpHwcLayersMinidumpLockedLegacy(std::string& result) const {
- for (const auto& [token, display] : mDisplays) {
+ for (const auto& [token, display] : FTL_FAKE_GUARD(mStateLock, mDisplays)) {
const auto displayId = HalDisplayId::tryCast(display->getId());
if (!displayId) {
continue;
@@ -6444,7 +6408,10 @@
}
void SurfaceFlinger::dumpHwcLayersMinidump(std::string& result) const {
- for (const auto& [token, display] : mDisplays) {
+ if (!mLayerLifecycleManagerEnabled) {
+ return dumpHwcLayersMinidumpLockedLegacy(result);
+ }
+ for (const auto& [token, display] : FTL_FAKE_GUARD(mStateLock, mDisplays)) {
const auto displayId = HalDisplayId::tryCast(display->getId());
if (!displayId) {
continue;
@@ -6469,8 +6436,14 @@
}
}
-void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& compositionLayers,
- std::string& result) const {
+void SurfaceFlinger::dumpAll(const DumpArgs& args, const std::string& compositionLayers,
+ std::string& result) const {
+ TimedLock lock(mStateLock, s2ns(1), __func__);
+ if (!lock.locked()) {
+ StringAppendF(&result, "Dumping without lock after timeout: %s (%d)\n",
+ strerror(-lock.status), lock.status);
+ }
+
const bool colorize = !args.empty() && args[0] == String16("--color");
Colorizer colorizer(colorize);