From 58aedbc9bea13415e2d42cf7c9fe8a7efd243e66 Mon Sep 17 00:00:00 2001
From: Jeff Brown <jeffbrown@google.com>
Date: Mon, 13 Feb 2012 20:15:01 -0800
Subject: 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
---
 core/jni/android_view_DisplayEventReceiver.cpp | 38 +++++++++-----------------
 1 file changed, 13 insertions(+), 25 deletions(-)

(limited to 'core/jni')

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 @@ private:
     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 @@ status_t NativeDisplayEventReceiver::initialize() {
         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 @@ status_t NativeDisplayEventReceiver::scheduleVsync() {
             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 @@ int NativeDisplayEventReceiver::handleReceiveCallback(int receiveFd, int events,
     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 @@ int NativeDisplayEventReceiver::handleReceiveCallback(int receiveFd, int events,
     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 @@ int NativeDisplayEventReceiver::handleReceiveCallback(int receiveFd, int events,
     }
 
     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 @@ int NativeDisplayEventReceiver::handleReceiveCallback(int receiveFd, int events,
         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
 }
 
 
-- 
cgit v1.1