Avoid deadlock in EventThread::onNewVsyncSchedule
EventThread::onNewVsyncSchedule attempts to lock mMutex, but it is
currently called while Scheduler::mDisplayLock is held. But
shouldConsumeEvent is called while mMutex is locked, and
shouldConsumeEvent's call to mThrottleVsyncCallback results calls
Scheduler::isVsyncValid, which then calls multiple methods that require
locking mDisplayLock, resulting in deadlock.
MessageQueue::onNewVsyncSchedule runs into a similar problem. It is also
called while mDisplayLock is held.
Split up the calls to onNewVsyncSchedule into a separate method, which
is called after mDisplayLock is released. Create
promotePacesetterDisplayLocked, which returns the new pacesetter's
VsyncSchedule so it can be applied afterwards. This avoids the deadlock.
In addition, handle the connections the same as other calls: lock
mConnectionsLock, store a pointer to the EventThread, and call the
method after releasing the lock. This is safe because Scheduler never
deletes its EventThreads (until its own teardown).
Bug: 272943830
Test: manual (see b/272234280)
Change-Id: Ieb05fc3abcf4fc20f04ea5d3595da70155de2df5
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 74547d5..3423652 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -321,7 +321,18 @@
// Chooses a pacesetter among the registered displays, unless `pacesetterIdOpt` is specified.
// The new `mPacesetterDisplayId` is never `std::nullopt`.
void promotePacesetterDisplay(std::optional<PhysicalDisplayId> pacesetterIdOpt = std::nullopt)
+ REQUIRES(kMainThreadContext) EXCLUDES(mDisplayLock);
+
+ // Changes to the displays (e.g. registering and unregistering) must be made
+ // while mDisplayLock is locked, and the new pacesetter then must be promoted while
+ // mDisplayLock is still locked. However, a new pacesetter means that
+ // MessageQueue and EventThread need to use the new pacesetter's
+ // VsyncSchedule, and this must happen while mDisplayLock is *not* locked,
+ // or else we may deadlock with EventThread.
+ std::shared_ptr<VsyncSchedule> promotePacesetterDisplayLocked(
+ std::optional<PhysicalDisplayId> pacesetterIdOpt = std::nullopt)
REQUIRES(kMainThreadContext, mDisplayLock);
+ void applyNewVsyncSchedule(std::shared_ptr<VsyncSchedule>) EXCLUDES(mDisplayLock);
// Blocks until the pacesetter's idle timer thread exits. `mDisplayLock` must not be locked by
// the caller on the main thread to avoid deadlock, since the timer thread locks it before exit.