Fix GraphicBuffer leaks in system_server

IWindowId is being kept alive by binder in certain cases, which
will leak WindowState, which can leak a whole lot including
the snapshot GraphicBuffer.

- Fix a leak in WindowContainer.mTmpChain1/2
- When the window gets removed, make sure to release the reference
to the window state so even if Binder keeps IWindowId alive, we
don't keep WindowState alive
- Make sure that even if we leak TaskSnapshotSurface we don't leak
the GraphicBuffer.

Test: Boot, some basic sanity testing.
Bug: 36279079
Change-Id: I4a0a784b32bc2df47934b1bed1d62b0682beb2dd

diff --git a/services/core/java/com/android/server/wm/TaskSnapshotSurface.java b/services/core/java/com/android/server/wm/TaskSnapshotSurface.java
index 5e7b910..5e71bcb 100644 (file)
--- a/services/core/java/com/android/server/wm/TaskSnapshotSurface.java
+++ b/services/core/java/com/android/server/wm/TaskSnapshotSurface.java
@@ -113,7 +113,7 @@ class TaskSnapshotSurface implements StartingSurface {
     private final Rect mStableInsets = new Rect();
     private final Rect mContentInsets = new Rect();
     private final Rect mFrame = new Rect();
-    private final TaskSnapshot mSnapshot;
+    private TaskSnapshot mSnapshot;
     private final CharSequence mTitle;
     private boolean mHasDrawn;
     private long mShownTime;
@@ -267,6 +267,9 @@ class TaskSnapshotSurface implements StartingSurface {
             mHasDrawn = true;
         }
         reportDrawn();
+
+        // In case window manager leaks us, make sure we don't retain the snapshot.
+        mSnapshot = null;
     }
 
     private void drawSizeMatchSnapshot(GraphicBuffer buffer) {

diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java
index ca8c484..6d3e0ed 100644 (file)
--- a/services/core/java/com/android/server/wm/WindowContainer.java
+++ b/services/core/java/com/android/server/wm/WindowContainer.java
@@ -655,39 +655,44 @@ class WindowContainer<E extends WindowContainer> implements Comparable<WindowCon
 
         final LinkedList<WindowContainer> thisParentChain = mTmpChain1;
         final LinkedList<WindowContainer> otherParentChain = mTmpChain2;
-        getParents(thisParentChain);
-        other.getParents(otherParentChain);
-
-        // Find the common ancestor of both containers.
-        WindowContainer commonAncestor = null;
-        WindowContainer thisTop = thisParentChain.peekLast();
-        WindowContainer otherTop = otherParentChain.peekLast();
-        while (thisTop != null && otherTop != null && thisTop == otherTop) {
-            commonAncestor = thisParentChain.removeLast();
-            otherParentChain.removeLast();
-            thisTop = thisParentChain.peekLast();
-            otherTop = otherParentChain.peekLast();
-        }
-
-        // Containers don't belong to the same hierarchy???
-        if (commonAncestor == null) {
-            throw new IllegalArgumentException("No in the same hierarchy this="
-                    + thisParentChain + " other=" + otherParentChain);
-        }
-
-        // Children are always considered greater than their parents, so if one of the containers
-        // we are comparing it the parent of the other then whichever is the child is greater.
-        if (commonAncestor == this) {
-            return -1;
-        } else if (commonAncestor == other) {
-            return 1;
-        }
-
-        // The position of the first non-common ancestor in the common ancestor list determines
-        // which is greater the which.
-        final WindowList<WindowContainer> list = commonAncestor.mChildren;
-        return list.indexOf(thisParentChain.peekLast()) > list.indexOf(otherParentChain.peekLast())
-                ? 1 : -1;
+        try {
+            getParents(thisParentChain);
+            other.getParents(otherParentChain);
+
+            // Find the common ancestor of both containers.
+            WindowContainer commonAncestor = null;
+            WindowContainer thisTop = thisParentChain.peekLast();
+            WindowContainer otherTop = otherParentChain.peekLast();
+            while (thisTop != null && otherTop != null && thisTop == otherTop) {
+                commonAncestor = thisParentChain.removeLast();
+                otherParentChain.removeLast();
+                thisTop = thisParentChain.peekLast();
+                otherTop = otherParentChain.peekLast();
+            }
+
+            // Containers don't belong to the same hierarchy???
+            if (commonAncestor == null) {
+                throw new IllegalArgumentException("No in the same hierarchy this="
+                        + thisParentChain + " other=" + otherParentChain);
+            }
+
+            // Children are always considered greater than their parents, so if one of the containers
+            // we are comparing it the parent of the other then whichever is the child is greater.
+            if (commonAncestor == this) {
+                return -1;
+            } else if (commonAncestor == other) {
+                return 1;
+            }
+
+            // The position of the first non-common ancestor in the common ancestor list determines
+            // which is greater the which.
+            final WindowList<WindowContainer> list = commonAncestor.mChildren;
+            return list.indexOf(thisParentChain.peekLast()) > list.indexOf(otherParentChain.peekLast())
+                    ? 1 : -1;
+        } finally {
+            mTmpChain1.clear();
+            mTmpChain2.clear();
+        }
     }
 
     private void getParents(LinkedList<WindowContainer> parents) {

diff --git a/services/core/java/com/android/server/wm/WindowState.java b/services/core/java/com/android/server/wm/WindowState.java
index 344c616..a69bc58 100644 (file)
--- a/services/core/java/com/android/server/wm/WindowState.java
+++ b/services/core/java/com/android/server/wm/WindowState.java
@@ -138,6 +138,7 @@ import com.android.internal.util.ToBooleanFunction;
 import com.android.server.input.InputWindowHandle;
 
 import java.io.PrintWriter;
+import java.lang.ref.WeakReference;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.LinkedList;
@@ -170,7 +171,7 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
     final int mOwnerUid;
     /** The owner has {@link android.Manifest.permission#INTERNAL_SYSTEM_WINDOW} */
     final boolean mOwnerCanAddInternalSystemWindow;
-    final IWindowId mWindowId;
+    final WindowId mWindowId;
     WindowToken mToken;
     // The same object as mToken if this is an app window and null for non-app windows.
     AppWindowToken mAppToken;
@@ -587,20 +588,7 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
         mAppToken = mToken.asAppWindowToken();
         mOwnerUid = ownerId;
         mOwnerCanAddInternalSystemWindow = ownerCanAddInternalSystemWindow;
-        mWindowId = new IWindowId.Stub() {
-            @Override
-            public void registerFocusObserver(IWindowFocusObserver observer) {
-                WindowState.this.registerFocusObserver(observer);
-            }
-            @Override
-            public void unregisterFocusObserver(IWindowFocusObserver observer) {
-                WindowState.this.unregisterFocusObserver(observer);
-            }
-            @Override
-            public boolean isFocused() {
-                return WindowState.this.isFocused();
-            }
-        };
+        mWindowId = new WindowId(this);
         mAttrs.copyFrom(a);
         mViewVisibility = viewVisibility;
         mPolicy = mService.mPolicy;
@@ -4435,6 +4423,40 @@ class WindowState extends WindowContainer<WindowState> implements WindowManagerP
         }
     }
 
+    private static final class WindowId extends IWindowId.Stub {
+        private final WeakReference<WindowState> mOuter;
+
+        private WindowId(WindowState outer) {
+
+            // Use a weak reference for the outer class. This is important to prevent the following
+            // leak: Since we send this class to the client process, binder will keep it alive as
+            // long as the client keeps it alive. Now, if the window is removed, we need to clear
+            // out our reference so even though this class is kept alive we don't leak WindowState,
+            // which can keep a whole lot of classes alive.
+            mOuter = new WeakReference<>(outer);
+        }
+
+        @Override
+        public void registerFocusObserver(IWindowFocusObserver observer) {
+            final WindowState outer = mOuter.get();
+            if (outer != null) {
+                outer.registerFocusObserver(observer);
+            }
+        }
+        @Override
+        public void unregisterFocusObserver(IWindowFocusObserver observer) {
+            final WindowState outer = mOuter.get();
+            if (outer != null) {
+                outer.unregisterFocusObserver(observer);
+            }
+        }
+        @Override
+        public boolean isFocused() {
+            final WindowState outer = mOuter.get();
+            return outer != null && outer.isFocused();
+        }
+    }
+
     boolean usesRelativeZOrdering() {
         if (!isChildWindow()) {
             return false;