Fix possible races in vsync infrastructure.
Applications sometimes crashed on exit due to the display event
receiver pipe apparently being closed while still a member of the
Looper's epoll fd set.
This patch fixes a few different possible races related to
the display event receiver lifecycle.
1. The receiver used to play a little dance with the Looper,
registering and unregistering its callback after each vsync
request. This code was a holdover from a time before the
surface flinger supported one-shot vsync requests, so we can
get rid of it and make things a lot simpler.
2. When the Choreographer is being accessed from outside the UI
thread, it needs to take great care that it does not touch
the display event receiver. Bad things could happen if the receiver
is handling a vsync event on the Looper and the receiver is
disposed concurrently.
3. It was possible for the Choreographer to attempt to dispose
the receiver while handling a vsync message. Now we defer disposing
the receiver for a little while, which is also nice because we
may be able to avoid disposing the receiver altogether if we find
that we need it again a little while later.
Bug: 5974105
Change-Id: I77a158f51b0b689af34d07aee4245b969e6260d6
diff --git a/core/java/android/view/Choreographer.java b/core/java/android/view/Choreographer.java
index 3ee275c..3e2d7fc 100644
--- a/core/java/android/view/Choreographer.java
+++ b/core/java/android/view/Choreographer.java
@@ -31,12 +31,20 @@
* This object is thread-safe. Other threads can add and remove listeners
* or schedule work to occur at a later time on the UI thread.
*
+ * Ensuring thread-safety is a little tricky because the {@link DisplayEventReceiver}
+ * can only be accessed from the UI thread so operations that touch the event receiver
+ * are posted to the UI thread if needed.
+ *
* @hide
*/
public final class Choreographer extends Handler {
private static final String TAG = "Choreographer";
private static final boolean DEBUG = false;
+ // Amount of time in ms to wait before actually disposing of the display event
+ // receiver after all listeners have been removed.
+ private static final long DISPOSE_RECEIVER_DELAY = 200;
+
// The default amount of time in ms between animation frames.
// When vsync is not enabled, we want to have some idea of how long we should
// wait before posting the next animation message. It is important that the
@@ -78,6 +86,8 @@
private static final int MSG_DO_ANIMATION = 0;
private static final int MSG_DO_DRAW = 1;
+ private static final int MSG_DO_SCHEDULE_VSYNC = 2;
+ private static final int MSG_DO_DISPOSE_RECEIVER = 3;
private final Object mLock = new Object();
@@ -88,6 +98,7 @@
private boolean mAnimationScheduled;
private boolean mDrawScheduled;
+ private boolean mFrameDisplayEventReceiverNeeded;
private FrameDisplayEventReceiver mFrameDisplayEventReceiver;
private long mLastAnimationTime;
private long mLastDrawTime;
@@ -158,10 +169,21 @@
if (DEBUG) {
Log.d(TAG, "Scheduling vsync for animation.");
}
- if (mFrameDisplayEventReceiver == null) {
- mFrameDisplayEventReceiver = new FrameDisplayEventReceiver(mLooper);
+
+ // If running on the Looper thread, then schedule the vsync immediately,
+ // otherwise post a message to schedule the vsync from the UI thread
+ // as soon as possible.
+ if (!mFrameDisplayEventReceiverNeeded) {
+ mFrameDisplayEventReceiverNeeded = true;
+ if (mFrameDisplayEventReceiver != null) {
+ removeMessages(MSG_DO_DISPOSE_RECEIVER);
+ }
}
- mFrameDisplayEventReceiver.scheduleVsync();
+ if (isRunningOnLooperThreadLocked()) {
+ doScheduleVsyncLocked();
+ } else {
+ sendMessageAtFrontOfQueue(obtainMessage(MSG_DO_SCHEDULE_VSYNC));
+ }
} else {
final long now = SystemClock.uptimeMillis();
final long nextAnimationTime = Math.max(mLastAnimationTime + sFrameDelay, now);
@@ -224,6 +246,12 @@
case MSG_DO_DRAW:
doDraw();
break;
+ case MSG_DO_SCHEDULE_VSYNC:
+ doScheduleVsync();
+ break;
+ case MSG_DO_DISPOSE_RECEIVER:
+ doDisposeReceiver();
+ break;
}
}
@@ -295,6 +323,30 @@
}
}
+ private void doScheduleVsync() {
+ synchronized (mLock) {
+ doScheduleVsyncLocked();
+ }
+ }
+
+ private void doScheduleVsyncLocked() {
+ if (mFrameDisplayEventReceiverNeeded && mAnimationScheduled) {
+ if (mFrameDisplayEventReceiver == null) {
+ mFrameDisplayEventReceiver = new FrameDisplayEventReceiver(mLooper);
+ }
+ mFrameDisplayEventReceiver.scheduleVsync();
+ }
+ }
+
+ private void doDisposeReceiver() {
+ synchronized (mLock) {
+ if (!mFrameDisplayEventReceiverNeeded && mFrameDisplayEventReceiver != null) {
+ mFrameDisplayEventReceiver.dispose();
+ mFrameDisplayEventReceiver = null;
+ }
+ }
+ }
+
/**
* Adds an animation listener.
*
@@ -386,7 +438,9 @@
if (mAnimationScheduled) {
mAnimationScheduled = false;
- if (!USE_VSYNC) {
+ if (USE_VSYNC) {
+ removeMessages(MSG_DO_SCHEDULE_VSYNC);
+ } else {
removeMessages(MSG_DO_ANIMATION);
}
}
@@ -398,13 +452,24 @@
}
}
- if (mFrameDisplayEventReceiver != null) {
- mFrameDisplayEventReceiver.dispose();
- mFrameDisplayEventReceiver = null;
+ // Post a message to dispose the display event receiver if we haven't needed
+ // it again after a certain amount of time has elapsed. Another reason to
+ // defer disposal is that it is possible for use to attempt to dispose the
+ // receiver while handling a vsync event that it dispatched, which might
+ // cause a few problems...
+ if (mFrameDisplayEventReceiverNeeded) {
+ mFrameDisplayEventReceiverNeeded = false;
+ if (mFrameDisplayEventReceiver != null) {
+ sendEmptyMessageDelayed(MSG_DO_DISPOSE_RECEIVER, DISPOSE_RECEIVER_DELAY);
+ }
}
}
}
+ private boolean isRunningOnLooperThreadLocked() {
+ return Looper.myLooper() == mLooper;
+ }
+
/**
* Listens for animation frame timing events.
*/
diff --git a/core/java/android/view/DisplayEventReceiver.java b/core/java/android/view/DisplayEventReceiver.java
index d6711ee..6c2e540 100644
--- a/core/java/android/view/DisplayEventReceiver.java
+++ b/core/java/android/view/DisplayEventReceiver.java
@@ -25,6 +25,10 @@
/**
* Provides a low-level mechanism for an application to receive display events
* such as vertical sync.
+ *
+ * The display event receive is NOT thread safe. Moreover, its methods must only
+ * be called on the Looper thread to which it is attached.
+ *
* @hide
*/
public abstract class DisplayEventReceiver {
diff --git a/core/jni/android_view_DisplayEventReceiver.cpp b/core/jni/android_view_DisplayEventReceiver.cpp
index 25397b5..8a1c4a9 100644
--- a/core/jni/android_view_DisplayEventReceiver.cpp
+++ b/core/jni/android_view_DisplayEventReceiver.cpp
@@ -59,21 +59,20 @@
sp<Looper> mLooper;
DisplayEventReceiver mReceiver;
bool mWaitingForVsync;
- bool mFdCallbackRegistered;
};
NativeDisplayEventReceiver::NativeDisplayEventReceiver(JNIEnv* env,
jobject receiverObj, const sp<Looper>& looper) :
mReceiverObjGlobal(env->NewGlobalRef(receiverObj)),
- mLooper(looper), mWaitingForVsync(false), mFdCallbackRegistered(false) {
+ mLooper(looper), mWaitingForVsync(false) {
ALOGV("receiver %p ~ Initializing input event receiver.", this);
}
NativeDisplayEventReceiver::~NativeDisplayEventReceiver() {
ALOGV("receiver %p ~ Disposing display event receiver.", this);
- if (mFdCallbackRegistered) {
+ if (!mReceiver.initCheck()) {
mLooper->removeFd(mReceiver.getFd());
}
@@ -88,6 +87,11 @@
return result;
}
+ int rc = mLooper->addFd(mReceiver.getFd(), 0, ALOOPER_EVENT_INPUT,
+ handleReceiveCallback, this);
+ if (rc < 0) {
+ return UNKNOWN_ERROR;
+ }
return OK;
}
@@ -113,15 +117,6 @@
return status;
}
- if (!mFdCallbackRegistered) {
- int rc = mLooper->addFd(mReceiver.getFd(), 0, ALOOPER_EVENT_INPUT,
- handleReceiveCallback, this);
- if (rc < 0) {
- return UNKNOWN_ERROR;
- }
- mFdCallbackRegistered = true;
- }
-
mWaitingForVsync = true;
}
return OK;
@@ -133,7 +128,6 @@
if (events & (ALOOPER_EVENT_ERROR | ALOOPER_EVENT_HANGUP)) {
ALOGE("Display event receiver pipe was closed or an error occurred. "
"events=0x%x", events);
- r->mFdCallbackRegistered = false;
return 0; // remove the callback
}
@@ -150,7 +144,7 @@
DisplayEventReceiver::Event buf[EVENT_BUFFER_SIZE];
ssize_t n;
while ((n = r->mReceiver.getEvents(buf, EVENT_BUFFER_SIZE)) > 0) {
- ALOGV("receiver %p ~ Read %d events.", this, int(n));
+ ALOGV("receiver %p ~ Read %d events.", data, int(n));
while (n-- > 0) {
if (buf[n].header.type == DisplayEventReceiver::DISPLAY_EVENT_VSYNC) {
vsyncTimestamp = buf[n].header.timestamp;
@@ -161,20 +155,20 @@
}
if (vsyncTimestamp < 0) {
- ALOGV("receiver %p ~ Woke up but there was no vsync pulse!", this);
+ ALOGV("receiver %p ~ Woke up but there was no vsync pulse!", data);
return 1; // keep the callback, did not obtain a vsync pulse
}
ALOGV("receiver %p ~ Vsync pulse: timestamp=%lld, count=%d",
- this, vsyncTimestamp, vsyncCount);
+ data, vsyncTimestamp, vsyncCount);
r->mWaitingForVsync = false;
JNIEnv* env = AndroidRuntime::getJNIEnv();
- ALOGV("receiver %p ~ Invoking vsync handler.", this);
+ ALOGV("receiver %p ~ Invoking vsync handler.", data);
env->CallVoidMethod(r->mReceiverObjGlobal,
gDisplayEventReceiverClassInfo.dispatchVsync, vsyncTimestamp, vsyncCount);
- ALOGV("receiver %p ~ Returned from vsync handler.", this);
+ ALOGV("receiver %p ~ Returned from vsync handler.", data);
if (env->ExceptionCheck()) {
ALOGE("An exception occurred while dispatching a vsync event.");
@@ -182,13 +176,7 @@
env->ExceptionClear();
}
- // Check whether dispatchVsync called scheduleVsync reentrantly and set mWaitingForVsync.
- // If so, keep the callback, otherwise remove it.
- if (r->mWaitingForVsync) {
- return 1; // keep the callback
- }
- r->mFdCallbackRegistered = false;
- return 0; // remove the callback
+ return 1; // keep the callback
}