From e42215ea70149e3ac92aa8d873b7f10e9bc48fe5 Mon Sep 17 00:00:00 2001
From: Mihai Popa <popam@google.com>
Date: Mon, 17 Jun 2019 12:34:50 +0100
Subject: [PATCH] [Magnifier-87] Fix deadlock causing ANR

Before this CL, the magnifier could deadlock when the following
happened:
1. the renderer is asked to draw (and a frame callback is provided)
2. a #dismiss() happens on the UI thread. This acquires mDestroyLock
(previously line 309)
3. InternalPopupWindow#destroy() is called, and this calls
mRenderer.destroy(). This attempts to destroy the renderer on the UI
thread, however the UI thread will wait until the pending frame callback
corresponding to step 1 is executed on the render thread.
4. The frame callback starts executing on the render thread, and tries
to acquire mDestroyLock (previously line 1093). However, this is held by
the UI thread, so a deadlock happens.

This CL completely removes mDestroyLock, relying on the existing
synchronization between the UI and render threads described in step 3.

Bug: 134584742
Test: manual testing
Change-Id: Ia4c75b5b997e0ed94d5a3814dd4507a8fffa124d
---
 core/java/android/widget/Magnifier.java | 71 ++++++++++---------------
 1 file changed, 27 insertions(+), 44 deletions(-)

diff --git a/core/java/android/widget/Magnifier.java b/core/java/android/widget/Magnifier.java
index 1719015d169db..ccafa640853d0 100644
--- a/core/java/android/widget/Magnifier.java
+++ b/core/java/android/widget/Magnifier.java
@@ -277,7 +277,7 @@ public final class Magnifier {
                             mWindowElevation, mWindowCornerRadius,
                             mOverlay != null ? mOverlay : new ColorDrawable(Color.TRANSPARENT),
                             Handler.getMain() /* draw the magnifier on the UI thread */, mLock,
-                            mDestroyLock, mCallback);
+                            mCallback);
                 }
             }
             performPixelCopy(startX, startY, true /* update window position */);
@@ -306,11 +306,9 @@ public final class Magnifier {
      */
     public void dismiss() {
         if (mWindow != null) {
-            synchronized (mDestroyLock) {
-                synchronized (mLock) {
-                    mWindow.destroy();
-                    mWindow = null;
-                }
+            synchronized (mLock) {
+                mWindow.destroy();
+                mWindow = null;
             }
             mPrevShowSourceCoords.x = NONEXISTENT_PREVIOUS_CONFIG_VALUE;
             mPrevShowSourceCoords.y = NONEXISTENT_PREVIOUS_CONFIG_VALUE;
@@ -835,24 +833,16 @@ public final class Magnifier {
         private int mWindowPositionY;
         private boolean mPendingWindowPositionUpdate;
 
-        // The lock used to synchronize the UI and render threads when a #destroy
-        // is performed on the UI thread and a frame callback on the render thread.
-        // When both mLock and mDestroyLock need to be held at the same time,
-        // mDestroyLock should be acquired before mLock in order to avoid deadlocks.
-        private final Object mDestroyLock;
-
         // The current content of the magnifier. It is mBitmap + mOverlay, only used for testing.
         private Bitmap mCurrentContent;
 
         InternalPopupWindow(final Context context, final Display display,
                 final SurfaceControl parentSurfaceControl, final int width, final int height,
                 final float elevation, final float cornerRadius, final Drawable overlay,
-                final Handler handler, final Object lock, final Object destroyLock,
-                final Callback callback) {
+                final Handler handler, final Object lock, final Callback callback) {
             mDisplay = display;
             mOverlay = overlay;
             mLock = lock;
-            mDestroyLock = destroyLock;
             mCallback = callback;
 
             mContentWidth = width;
@@ -1039,20 +1029,17 @@ public final class Magnifier {
         }
 
         /**
-         * Destroys this instance.
+         * Destroys this instance. The method has to be called in a context holding {@link #mLock}.
          */
         public void destroy() {
-            synchronized (mDestroyLock) {
-                mSurface.destroy();
-            }
-            synchronized (mLock) {
-                mRenderer.destroy();
-                mSurfaceControl.remove();
-                mSurfaceSession.kill();
-                mHandler.removeCallbacks(mMagnifierUpdater);
-                if (mBitmap != null) {
-                    mBitmap.recycle();
-                }
+            // Destroy the renderer. This will not proceed until pending frame callbacks complete.
+            mRenderer.destroy();
+            mSurface.destroy();
+            mSurfaceControl.remove();
+            mSurfaceSession.kill();
+            mHandler.removeCallbacks(mMagnifierUpdater);
+            if (mBitmap != null) {
+                mBitmap.recycle();
             }
         }
 
@@ -1090,24 +1077,20 @@ public final class Magnifier {
                     final int pendingY = mWindowPositionY;
 
                     callback = frame -> {
-                        synchronized (mDestroyLock) {
-                            if (!mSurface.isValid()) {
-                                return;
-                            }
-                            synchronized (mLock) {
-                                // Show or move the window at the content draw frame.
-                                SurfaceControl.openTransaction();
-                                mSurfaceControl.deferTransactionUntil(mSurface, frame);
-                                if (updateWindowPosition) {
-                                    mSurfaceControl.setPosition(pendingX, pendingY);
-                                }
-                                if (firstDraw) {
-                                    mSurfaceControl.setLayer(SURFACE_Z);
-                                    mSurfaceControl.show();
-                                }
-                                SurfaceControl.closeTransaction();
-                            }
+                        if (!mSurface.isValid()) {
+                            return;
                         }
+                        // Show or move the window at the content draw frame.
+                        SurfaceControl.openTransaction();
+                        mSurfaceControl.deferTransactionUntil(mSurface, frame);
+                        if (updateWindowPosition) {
+                            mSurfaceControl.setPosition(pendingX, pendingY);
+                        }
+                        if (firstDraw) {
+                            mSurfaceControl.setLayer(SURFACE_Z);
+                            mSurfaceControl.show();
+                        }
+                        SurfaceControl.closeTransaction();
                     };
                     mRenderer.setLightCenter(mDisplay, pendingX, pendingY);
                 } else {