From be5bebafabd509df9a1abcbf4c54a60c24504c5c Mon Sep 17 00:00:00 2001
From: Li Li <dualli@google.com>
Date: Tue, 3 Nov 2020 18:02:56 -0800
Subject: [PATCH] Use CloseGuard for cursor leak detection

Protect Cursors with CloseGuard and report implicit cleanup.

By default, the leak detection code will print a one-line warning
message about failing to call the cleanup function. To report more
details like the original stacktrace and SQLite query statement, enable
the correspoinding VmPolicy flags as the example below, please.

/*
 * public void onCreate() {
 *     if (DEVELOPER_MODE) {
 *         StrictMode.setVmPolicy(new VmPolicy.Builder()
 *                 .detectLeakedSqlLiteObjects()  // for SqlLiteCursor
 *                 .detectLeakedClosableObjects() // for any Cursor
 *                 .penaltyLog()
 *                 .build());
 *     }
 *     super.onCreate();
 * }
 */

By enabling detectLeakedSqlLiteObjecs, the original SQLiteCursor query
statement is reported when close() or its equivalent cleanup function
is not called before finalize().

By enabling detectLeakedClosableObjects, the new CloseGuard report will
be provided, along with the original stack trace captured in open() or
its equivalent.

The former has better performance as it doesn't capture an original
stack trace during open() stage. Only enable the latter if performance
impact is not an issue. Both of them can be enabled at the same time.

Bug: 168639120
Test: manually test with an example SQLite Android application, with and
without calling cursor.close() after a db.rawQuery().

Change-Id: Ibe9fcdc8119c2e4651df1983e7ccd793f29e8e9d
Signed-off-by: Li Li <dualli@google.com>
---
 .../java/android/database/AbstractCursor.java |  8 +++++++
 .../android/database/sqlite/SQLiteCursor.java | 22 ++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/core/java/android/database/AbstractCursor.java b/core/java/android/database/AbstractCursor.java
index 3effc5ae5f422..cf25c3c56208e 100644
--- a/core/java/android/database/AbstractCursor.java
+++ b/core/java/android/database/AbstractCursor.java
@@ -24,6 +24,8 @@ import android.os.Build;
 import android.os.Bundle;
 import android.util.Log;
 
+import dalvik.system.CloseGuard;
+
 import java.lang.ref.WeakReference;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -86,6 +88,9 @@ public abstract class AbstractCursor implements CrossProcessCursor {
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
     private Bundle mExtras = Bundle.EMPTY;
 
+    /** CloseGuard to detect leaked cursor **/
+    private final CloseGuard mCloseGuard = CloseGuard.get();
+
     /* -------------------------------------------------------- */
     /* These need to be implemented by subclasses */
     @Override
@@ -179,6 +184,7 @@ public abstract class AbstractCursor implements CrossProcessCursor {
         mClosed = true;
         mContentObservable.unregisterAll();
         onDeactivateOrClose();
+        mCloseGuard.close();
     }
 
     /**
@@ -218,6 +224,7 @@ public abstract class AbstractCursor implements CrossProcessCursor {
     /* Implementation */
     public AbstractCursor() {
         mPos = -1;
+        mCloseGuard.open("close");
     }
 
     @Override
@@ -521,6 +528,7 @@ public abstract class AbstractCursor implements CrossProcessCursor {
             mContentResolver.unregisterContentObserver(mSelfObserver);
         }
         try {
+            if (mCloseGuard != null) mCloseGuard.warnIfOpen();
             if (!mClosed) close();
         } catch(Exception e) { }
     }
diff --git a/core/java/android/database/sqlite/SQLiteCursor.java b/core/java/android/database/sqlite/SQLiteCursor.java
index 3bfbe7e7b93b4..7ba63e6462f17 100644
--- a/core/java/android/database/sqlite/SQLiteCursor.java
+++ b/core/java/android/database/sqlite/SQLiteCursor.java
@@ -62,9 +62,6 @@ public class SQLiteCursor extends AbstractWindowedCursor {
     /** A mapping of column names to column indices, to speed up lookups */
     private Map<String, Integer> mColumnNameMap;
 
-    /** Used to find out where a cursor was allocated in case it never got released. */
-    private final Throwable mStackTrace;
-
     /** Controls fetching of rows relative to requested position **/
     private boolean mFillWindowForwardOnly;
 
@@ -102,11 +99,6 @@ public class SQLiteCursor extends AbstractWindowedCursor {
         if (query == null) {
             throw new IllegalArgumentException("query object cannot be null");
         }
-        if (StrictMode.vmSqliteObjectLeaksEnabled()) {
-            mStackTrace = new DatabaseObjectNotClosedException().fillInStackTrace();
-        } else {
-            mStackTrace = null;
-        }
         mDriver = driver;
         mEditTable = editTable;
         mColumnNameMap = null;
@@ -283,17 +275,17 @@ public class SQLiteCursor extends AbstractWindowedCursor {
         try {
             // if the cursor hasn't been closed yet, close it first
             if (mWindow != null) {
-                if (mStackTrace != null) {
+                // Report original sql statement
+                if (StrictMode.vmSqliteObjectLeaksEnabled()) {
                     String sql = mQuery.getSql();
                     int len = sql.length();
                     StrictMode.onSqliteObjectLeaked(
-                        "Finalizing a Cursor that has not been deactivated or closed. " +
-                        "database = " + mQuery.getDatabase().getLabel() +
-                        ", table = " + mEditTable +
-                        ", query = " + sql.substring(0, (len > 1000) ? 1000 : len),
-                        mStackTrace);
+                            "Finalizing a Cursor that has not been deactivated or closed. "
+                            + "database = " + mQuery.getDatabase().getLabel()
+                            + ", table = " + mEditTable
+                            + ", query = " + sql.substring(0, (len > 1000) ? 1000 : len),
+                            null);
                 }
-                close();
             }
         } finally {
             super.finalize();