From 82871bdcfcc250e4aeeb731dfedeb0a23623244a Mon Sep 17 00:00:00 2001
From: Seigo Nonaka <nona@google.com>
Date: Mon, 17 Jul 2017 20:22:44 -0700
Subject: [PATCH] Fix race condition of removing surface.

mContainer.startingSurface can be replaced between posting
mRemoveStartingWindow and actually surface.remove() is called.

1. removeStartingWindow is called, then mRemoveStartingWindow is posted
   to mHandler
2. transferStartingWindow is called, then mContainer.startingWindow is
   replaced with new surface.
3. mHandler pops the callback and surface.remove() is called.

Here the remove is only called for replaced surface and never called for
older surface.

To fix this issue, surely removes the surface when the
removeStartingWindow is called.

Bug: 63156080
Bug: 63784898
Test: Watch /proc/<pid>/fd
Change-Id: I55e2c1b8fba32b3a19603e6ad4743f07576abd48
---
 .../wm/AppWindowContainerController.java      | 73 ++++++++-----------
 1 file changed, 29 insertions(+), 44 deletions(-)

diff --git a/services/core/java/com/android/server/wm/AppWindowContainerController.java b/services/core/java/com/android/server/wm/AppWindowContainerController.java
index a6ffe83c366eb..84fafe261d353 100644
--- a/services/core/java/com/android/server/wm/AppWindowContainerController.java
+++ b/services/core/java/com/android/server/wm/AppWindowContainerController.java
@@ -115,41 +115,6 @@ public class AppWindowContainerController
         mListener.onWindowsGone();
     };
 
-    private final Runnable mRemoveStartingWindow = () -> {
-        StartingSurface surface = null;
-        synchronized (mWindowMap) {
-            if (mContainer == null) {
-                if (DEBUG_STARTING_WINDOW) Slog.v(TAG_WM, "mContainer was null while trying to"
-                        + " remove starting window");
-                return;
-            }
-            if (DEBUG_STARTING_WINDOW) Slog.v(TAG_WM, "Remove starting " + mContainer
-                    + ": startingWindow=" + mContainer.startingWindow
-                    + " startingView=" + mContainer.startingSurface);
-            if (mContainer.startingData != null) {
-                surface = mContainer.startingSurface;
-                mContainer.startingData = null;
-                mContainer.startingSurface = null;
-                mContainer.startingWindow = null;
-                mContainer.startingDisplayed = false;
-                if (surface == null && DEBUG_STARTING_WINDOW) {
-                    Slog.v(TAG_WM, "startingWindow was set but startingSurface==null, couldn't "
-                            + "remove");
-                }
-            } else if (DEBUG_STARTING_WINDOW) {
-                Slog.v(TAG_WM, "Tried to remove starting window but startingWindow was null:"
-                        + mContainer);
-            }
-        }
-        if (surface != null) {
-            try {
-                surface.remove();
-            } catch (Exception e) {
-                Slog.w(TAG_WM, "Exception when removing starting window", e);
-            }
-        }
-    };
-
     private final Runnable mAddStartingWindow = () -> {
         final StartingData startingData;
         final AppWindowToken container;
@@ -665,13 +630,6 @@ public class AppWindowContainerController
 
     void removeStartingWindow() {
         synchronized (mWindowMap) {
-            if (mHandler.hasCallbacks(mRemoveStartingWindow)) {
-                // Already scheduled.
-                if (DEBUG_STARTING_WINDOW) Slog.v(TAG_WM, "Trying to remove starting window but "
-                        + "already scheduled");
-                return;
-            }
-
             if (mContainer.startingWindow == null) {
                 if (mContainer.startingData != null) {
                     // Starting window has not been added yet, but it is scheduled to be added.
@@ -683,12 +641,39 @@ public class AppWindowContainerController
                 return;
             }
 
+            final StartingSurface surface;
+            if (mContainer.startingData != null) {
+                surface = mContainer.startingSurface;
+                mContainer.startingData = null;
+                mContainer.startingSurface = null;
+                mContainer.startingWindow = null;
+                mContainer.startingDisplayed = false;
+                if (surface == null && DEBUG_STARTING_WINDOW) {
+                    Slog.v(TAG_WM, "startingWindow was set but startingSurface==null, couldn't "
+                            + "remove");
+                }
+            } else {
+                if (DEBUG_STARTING_WINDOW) {
+                    Slog.v(TAG_WM, "Tried to remove starting window but startingWindow was null:"
+                            + mContainer);
+                }
+                return;
+            }
+
             if (DEBUG_STARTING_WINDOW) Slog.v(TAG_WM, "Schedule remove starting " + mContainer
-                    + " startingWindow=" + mContainer.startingWindow);
+                    + " startingWindow=" + mContainer.startingWindow
+                    + " startingView=" + mContainer.startingSurface);
 
             // Use the same thread to remove the window as we used to add it, as otherwise we end up
             // with things in the view hierarchy being called from different threads.
-            mService.mAnimationHandler.post(mRemoveStartingWindow);
+            mHandler.post(() -> {
+                if (DEBUG_STARTING_WINDOW) Slog.v(TAG_WM, "Removing startingView=" + surface);
+                try {
+                    surface.remove();
+                } catch (Exception e) {
+                    Slog.w(TAG_WM, "Exception when removing starting window", e);
+                }
+            });
         }
     }