From 387838be955a44422509c2c7bc124327e9fe61d7 Mon Sep 17 00:00:00 2001
From: Robert Carr <racarr@google.com>
Date: Wed, 7 Sep 2016 14:12:44 -0700
Subject: [PATCH] SurfaceView: Force surface disconnection before reuse.

Prior to N, if a client received the surfaceDestroyed callback
from their SurfaceView they could trust that the surface would not be
reused. Now that is not true in multiple scenarios. If a client fails
to shut down its EGL context then, we could end up in a situation where,
the client will never disconnect. Then when we reuse the same
underlying IGraphicBufferProducer for surfaceCreated next
the app will crash with a double connect error. There is no valid
use of the surface inbetween surfaceDestroyed and surfaceCreated,
so we just force the disconnection after surfaceDestroyed.

Bug: 30236166
Change-Id: I2e3e4b3176492dc0c2d46a59e0b5a781bf9bc356
---
 core/java/android/view/Surface.java     | 11 +++++++++++
 core/java/android/view/SurfaceView.java | 12 ++++++++++++
 core/jni/android_view_Surface.cpp       |  6 ++++++
 3 files changed, 29 insertions(+)

diff --git a/core/java/android/view/Surface.java b/core/java/android/view/Surface.java
index f92d83af93a0a..3bbe88b3adb91 100644
--- a/core/java/android/view/Surface.java
+++ b/core/java/android/view/Surface.java
@@ -59,6 +59,7 @@ public class Surface implements Parcelable {
     private static native long nativeGetNextFrameNumber(long nativeObject);
     private static native int nativeSetScalingMode(long nativeObject, int scalingMode);
     private static native void nativeSetBuffersTransform(long nativeObject, long transform);
+    private static native int nativeForceScopedDisconnect(long nativeObject);
 
     public static final Parcelable.Creator<Surface> CREATOR =
             new Parcelable.Creator<Surface>() {
@@ -538,6 +539,16 @@ public class Surface implements Parcelable {
         }
     }
 
+    void forceScopedDisconnect() {
+        synchronized (mLock) {
+            checkNotReleasedLocked();
+            int err = nativeForceScopedDisconnect(mNativeObject);
+            if (err != 0) {
+                throw new RuntimeException("Failed to disconnect Surface instance (bad object?)");
+            }
+        }
+    }
+
     /**
      * Returns whether or not this Surface is backed by a single-buffered SurfaceTexture
      * @hide
diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 77bcf5ffe16aa..ee186e7e86fa4 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -586,6 +586,18 @@ public class SurfaceView extends View {
                             for (SurfaceHolder.Callback c : callbacks) {
                                 c.surfaceDestroyed(mSurfaceHolder);
                             }
+                            // Since Android N the same surface may be reused and given to us
+                            // again by the system server at a later point. However
+                            // as we didn't do this in previous releases, clients weren't
+                            // necessarily required to clean up properly in
+                            // surfaceDestroyed. This leads to problems for example when
+                            // clients don't destroy their EGL context, and try
+                            // and create a new one on the same surface following reuse.
+                            // Since there is no valid use of the surface in-between
+                            // surfaceDestroyed and surfaceCreated, we force a disconnect,
+                            // so the next connect will always work if we end up reusing
+                            // the surface.
+                            mSurface.forceScopedDisconnect();
                         }
                     }
 
diff --git a/core/jni/android_view_Surface.cpp b/core/jni/android_view_Surface.cpp
index 21e4d2fb229fb..a0c62c3662850 100644
--- a/core/jni/android_view_Surface.cpp
+++ b/core/jni/android_view_Surface.cpp
@@ -487,6 +487,11 @@ static jint nativeSetScalingMode(JNIEnv *env, jclass clazz, jlong nativeObject,
     return surface->setScalingMode(scalingMode);
 }
 
+static jint nativeForceScopedDisconnect(JNIEnv *env, jclass clazz, jlong nativeObject) {
+    Surface* surface = reinterpret_cast<Surface*>(nativeObject);
+    return surface->disconnect(-1, IGraphicBufferProducer::DisconnectMode::AllLocal);
+}
+
 namespace uirenderer {
 
 using namespace android::uirenderer::renderthread;
@@ -564,6 +569,7 @@ static const JNINativeMethod gSurfaceMethods[] = {
     {"nativeGetHeight", "(J)I", (void*)nativeGetHeight },
     {"nativeGetNextFrameNumber", "(J)J", (void*)nativeGetNextFrameNumber },
     {"nativeSetScalingMode", "(JI)I", (void*)nativeSetScalingMode },
+    {"nativeForceScopedDisconnect", "(J)I", (void*)nativeForceScopedDisconnect},
 
     // HWUI context
     {"nHwuiCreate", "(JJ)J", (void*) hwui::create },