Merge "Fix deadlock of main thread and Perfetto thread (LayerDataSource::OnStart)" into main

4 files changed, 51 insertions, 25 deletions

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 65a0ed3065..06560d06ba 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -946,16 +946,20 @@ void SurfaceFlinger::init() FTL_FAKE_GUARD(kMainThreadContext) {
                         }));
                     }));
 
-    mLayerTracing.setTakeLayersSnapshotProtoFunction([&](uint32_t traceFlags) {
-        auto snapshot = perfetto::protos::LayersSnapshotProto{};
-        mScheduler
-                ->schedule([&]() FTL_FAKE_GUARD(mStateLock) FTL_FAKE_GUARD(kMainThreadContext) {
-                    snapshot = takeLayersSnapshotProto(traceFlags, TimePoint::now(),
-                                                       mLastCommittedVsyncId, true);
-                })
-                .wait();
-        return snapshot;
-    });
+    mLayerTracing.setTakeLayersSnapshotProtoFunction(
+            [&](uint32_t traceFlags,
+                const LayerTracing::OnLayersSnapshotCallback& onLayersSnapshot) {
+                // Do not wait the future to avoid deadlocks
+                // between main and Perfetto threads (b/313130597)
+                static_cast<void>(mScheduler->schedule(
+                        [&, onLayersSnapshot]() FTL_FAKE_GUARD(mStateLock)
+                                FTL_FAKE_GUARD(kMainThreadContext) {
+                                    auto snapshot =
+                                            takeLayersSnapshotProto(traceFlags, TimePoint::now(),
+                                                                    mLastCommittedVsyncId, true);
+                                    onLayersSnapshot(std::move(snapshot));
+                                }));
+            });
 
     // Commit secondary display(s).
     processDisplayChangesLocked();
diff --git a/services/surfaceflinger/Tracing/LayerDataSource.cpp b/services/surfaceflinger/Tracing/LayerDataSource.cpp
index ed1e2ec89c..cc0063cd2a 100644
--- a/services/surfaceflinger/Tracing/LayerDataSource.cpp
+++ b/services/surfaceflinger/Tracing/LayerDataSource.cpp
@@ -82,10 +82,13 @@ void LayerDataSource::OnFlush(const LayerDataSource::FlushArgs& args) {
     }
 }
 
-void LayerDataSource::OnStop(const LayerDataSource::StopArgs&) {
+void LayerDataSource::OnStop(const LayerDataSource::StopArgs& args) {
     ALOGD("Received OnStop event (mode = 0x%02x, flags = 0x%02x)", mMode, mFlags);
     if (auto* p = mLayerTracing.load()) {
-        p->onStop(mMode);
+        // In dump mode we need to defer the stop (through HandleStopAsynchronously()) till
+        // the layers snapshot has been captured and written to perfetto. We must avoid writing
+        // to perfetto within the OnStop callback to prevent deadlocks (b/313130597).
+        p->onStop(mMode, mFlags, args.HandleStopAsynchronously());
     }
 }
 
diff --git a/services/surfaceflinger/Tracing/LayerTracing.cpp b/services/surfaceflinger/Tracing/LayerTracing.cpp
index d40b888504..d78f9bbbae 100644
--- a/services/surfaceflinger/Tracing/LayerTracing.cpp
+++ b/services/surfaceflinger/Tracing/LayerTracing.cpp
@@ -32,7 +32,7 @@
 namespace android {
 
 LayerTracing::LayerTracing() {
-    mTakeLayersSnapshotProto = [](uint32_t) { return perfetto::protos::LayersSnapshotProto{}; };
+    mTakeLayersSnapshotProto = [](uint32_t, const OnLayersSnapshotCallback&) {};
     LayerDataSource::Initialize(*this);
 }
 
@@ -45,7 +45,7 @@ LayerTracing::~LayerTracing() {
 }
 
 void LayerTracing::setTakeLayersSnapshotProtoFunction(
-        const std::function<perfetto::protos::LayersSnapshotProto(uint32_t)>& callback) {
+        const std::function<void(uint32_t, const OnLayersSnapshotCallback&)>& callback) {
     mTakeLayersSnapshotProto = callback;
 }
 
@@ -62,7 +62,10 @@ void LayerTracing::onStart(Mode mode, uint32_t flags) {
             // It might take a while before a layers change occurs and a "spontaneous" snapshot is
             // taken. Let's manually take a snapshot, so that the trace's first entry will contain
             // the current layers state.
-            addProtoSnapshotToOstream(mTakeLayersSnapshotProto(flags), Mode::MODE_ACTIVE);
+            auto onLayersSnapshot = [this](perfetto::protos::LayersSnapshotProto&& snapshot) {
+                addProtoSnapshotToOstream(std::move(snapshot), Mode::MODE_ACTIVE);
+            };
+            mTakeLayersSnapshotProto(flags, onLayersSnapshot);
             ALOGD("Started active tracing (traced initial snapshot)");
             break;
         }
@@ -89,9 +92,7 @@ void LayerTracing::onStart(Mode mode, uint32_t flags) {
             break;
         }
         case Mode::MODE_DUMP: {
-            auto snapshot = mTakeLayersSnapshotProto(flags);
-            addProtoSnapshotToOstream(std::move(snapshot), Mode::MODE_DUMP);
-            ALOGD("Started dump tracing (dumped single snapshot)");
+            ALOGD("Started dump tracing");
             break;
         }
         default: {
@@ -125,10 +126,27 @@ void LayerTracing::onFlush(Mode mode, uint32_t flags, bool isBugreport) {
     ALOGD("Flushed generated tracing");
 }
 
-void LayerTracing::onStop(Mode mode) {
-    if (mode == Mode::MODE_ACTIVE) {
-        mIsActiveTracingStarted.store(false);
-        ALOGD("Stopped active tracing");
+void LayerTracing::onStop(Mode mode, uint32_t flags, std::function<void()>&& deferredStopDone) {
+    switch (mode) {
+        case Mode::MODE_ACTIVE: {
+            mIsActiveTracingStarted.store(false);
+            deferredStopDone();
+            ALOGD("Stopped active tracing");
+            break;
+        }
+        case Mode::MODE_DUMP: {
+            auto onLayersSnapshot = [this, deferredStopDone = std::move(deferredStopDone)](
+                                            perfetto::protos::LayersSnapshotProto&& snapshot) {
+                addProtoSnapshotToOstream(std::move(snapshot), Mode::MODE_DUMP);
+                deferredStopDone();
+                ALOGD("Stopped dump tracing (written single snapshot)");
+            };
+            mTakeLayersSnapshotProto(flags, onLayersSnapshot);
+            break;
+        }
+        default: {
+            deferredStopDone();
+        }
     }
 }
 
diff --git a/services/surfaceflinger/Tracing/LayerTracing.h b/services/surfaceflinger/Tracing/LayerTracing.h
index 2895ba7026..e99fe4c7f1 100644
--- a/services/surfaceflinger/Tracing/LayerTracing.h
+++ b/services/surfaceflinger/Tracing/LayerTracing.h
@@ -87,6 +87,7 @@ EOF
 class LayerTracing {
 public:
     using Mode = perfetto::protos::pbzero::SurfaceFlingerLayersConfig::Mode;
+    using OnLayersSnapshotCallback = std::function<void(perfetto::protos::LayersSnapshotProto&&)>;
 
     enum Flag : uint32_t {
         TRACE_INPUT = 1 << 1,
@@ -102,7 +103,7 @@ public:
     LayerTracing(std::ostream&);
     ~LayerTracing();
     void setTakeLayersSnapshotProtoFunction(
-            const std::function<perfetto::protos::LayersSnapshotProto(uint32_t)>&);
+            const std::function<void(uint32_t, const OnLayersSnapshotCallback&)>&);
     void setTransactionTracing(TransactionTracing&);
 
     // Start event from perfetto data source
@@ -110,7 +111,7 @@ public:
     // Flush event from perfetto data source
     void onFlush(Mode mode, uint32_t flags, bool isBugreport);
     // Stop event from perfetto data source
-    void onStop(Mode mode);
+    void onStop(Mode mode, uint32_t flags, std::function<void()>&& deferredStopDone);
 
     void addProtoSnapshotToOstream(perfetto::protos::LayersSnapshotProto&& snapshot, Mode mode);
     bool isActiveTracingStarted() const;
@@ -123,7 +124,7 @@ private:
     void writeSnapshotToPerfetto(const perfetto::protos::LayersSnapshotProto& snapshot, Mode mode);
     bool checkAndUpdateLastVsyncIdWrittenToPerfetto(Mode mode, std::int64_t vsyncId);
 
-    std::function<perfetto::protos::LayersSnapshotProto(uint32_t)> mTakeLayersSnapshotProto;
+    std::function<void(uint32_t, const OnLayersSnapshotCallback&)> mTakeLayersSnapshotProto;
     TransactionTracing* mTransactionTracing;
 
     std::atomic<bool> mIsActiveTracingStarted{false};