Straighten out database cursors behavior.

Some were never closed, other closed twice. This change
makes all Cursor instances behave, having the #close()
call in a finally{} clause, and puts the burden of closing
the cursor squarely on the creator rather than in the
called methods.
There is however one exception that is beyond the scope
of this change: UserDictionarySettings have a Cursor
member, it's never closed, and fixing the problem is not
obvious. This change adds a TODO for now.

It's not very clear if this change actually helps with
bug#12670151, but it may be related and it's a good
think to do anyway.

Bug: 12670151
Change-Id: I87cc44387e7dee3da1488671b93a28d9d73f7dc0

java/src/com/android/inputmethod/dictionarypack/DictionaryProvider.java

@@ -350,7 +350,8 @@ public final class DictionaryProvider extends ContentProvider {
                         clientId);
         if (null == results) {
             return Collections.<WordListInfo>emptyList();
-        } else {
+        }
+        try {
             final HashMap<String, WordListInfo> dicts = new HashMap<String, WordListInfo>();
             final int idIndex = results.getColumnIndex(MetadataDbHelper.WORDLISTID_COLUMN);
             final int localeIndex = results.getColumnIndex(MetadataDbHelper.LOCALE_COLUMN);
@@ -416,8 +417,9 @@ public final class DictionaryProvider extends ContentProvider {
                     }
                 } while (results.moveToNext());
             }
-            results.close();
             return Collections.unmodifiableCollection(dicts.values());
+        } finally {
+            results.close();
         }
     }
 

java/src/com/android/inputmethod/dictionarypack/DictionarySettingsFragment.java

@@ -283,63 +283,70 @@ public final class DictionarySettingsFragment extends PreferenceFragment
             final ArrayList<Preference> result = new ArrayList<Preference>();
             result.add(createErrorMessage(activity, R.string.cannot_connect_to_dict_service));
             return result;
-        } else if (!cursor.moveToFirst()) {
+        }
-            final ArrayList<Preference> result = new ArrayList<Preference>();
+        try {
-            result.add(createErrorMessage(activity, R.string.no_dictionaries_available));
+            if (!cursor.moveToFirst()) {
-            cursor.close();
+                final ArrayList<Preference> result = new ArrayList<Preference>();
-            return result;
+                result.add(createErrorMessage(activity, R.string.no_dictionaries_available));
-        } else {
+                return result;
-            final String systemLocaleString = Locale.getDefault().toString();
+            } else {
-            final TreeMap<String, WordListPreference> prefMap =
+                final String systemLocaleString = Locale.getDefault().toString();
-                    new TreeMap<String, WordListPreference>();
+                final TreeMap<String, WordListPreference> prefMap =
-            final int idIndex = cursor.getColumnIndex(MetadataDbHelper.WORDLISTID_COLUMN);
+                        new TreeMap<String, WordListPreference>();
-            final int versionIndex = cursor.getColumnIndex(MetadataDbHelper.VERSION_COLUMN);
+                final int idIndex = cursor.getColumnIndex(MetadataDbHelper.WORDLISTID_COLUMN);
-            final int localeIndex = cursor.getColumnIndex(MetadataDbHelper.LOCALE_COLUMN);
+                final int versionIndex = cursor.getColumnIndex(MetadataDbHelper.VERSION_COLUMN);
-            final int descriptionIndex = cursor.getColumnIndex(MetadataDbHelper.DESCRIPTION_COLUMN);
+                final int localeIndex = cursor.getColumnIndex(MetadataDbHelper.LOCALE_COLUMN);
-            final int statusIndex = cursor.getColumnIndex(MetadataDbHelper.STATUS_COLUMN);
+                final int descriptionIndex =
-            final int filesizeIndex = cursor.getColumnIndex(MetadataDbHelper.FILESIZE_COLUMN);
+                        cursor.getColumnIndex(MetadataDbHelper.DESCRIPTION_COLUMN);
-            do {
+                final int statusIndex = cursor.getColumnIndex(MetadataDbHelper.STATUS_COLUMN);
-                final String wordlistId = cursor.getString(idIndex);
+                final int filesizeIndex = cursor.getColumnIndex(MetadataDbHelper.FILESIZE_COLUMN);
-                final int version = cursor.getInt(versionIndex);
+                do {
-                final String localeString = cursor.getString(localeIndex);
+                    final String wordlistId = cursor.getString(idIndex);
-                final Locale locale = new Locale(localeString);
+                    final int version = cursor.getInt(versionIndex);
-                final String description = cursor.getString(descriptionIndex);
+                    final String localeString = cursor.getString(localeIndex);
-                final int status = cursor.getInt(statusIndex);
+                    final Locale locale = new Locale(localeString);
-                final int matchLevel = LocaleUtils.getMatchLevel(systemLocaleString, localeString);
+                    final String description = cursor.getString(descriptionIndex);
-                final String matchLevelString = LocaleUtils.getMatchLevelSortedString(matchLevel);
+                    final int status = cursor.getInt(statusIndex);
-                final int filesize = cursor.getInt(filesizeIndex);
+                    final int matchLevel =
-                // The key is sorted in lexicographic order, according to the match level, then
+                            LocaleUtils.getMatchLevel(systemLocaleString, localeString);
-                // the description.
+                    final String matchLevelString =
-                final String key = matchLevelString + "." + description + "." + wordlistId;
+                            LocaleUtils.getMatchLevelSortedString(matchLevel);
-                final WordListPreference existingPref = prefMap.get(key);
+                    final int filesize = cursor.getInt(filesizeIndex);
-                if (null == existingPref || existingPref.hasPriorityOver(status)) {
+                    // The key is sorted in lexicographic order, according to the match level, then
-                    final WordListPreference oldPreference = mCurrentPreferenceMap.get(key);
+                    // the description.
-                    final WordListPreference pref;
+                    final String key = matchLevelString + "." + description + "." + wordlistId;
-                    if (null != oldPreference
+                    final WordListPreference existingPref = prefMap.get(key);
-                            && oldPreference.mVersion == version
+                    if (null == existingPref || existingPref.hasPriorityOver(status)) {
-                            && oldPreference.hasStatus(status)
+                        final WordListPreference oldPreference = mCurrentPreferenceMap.get(key);
-                            && oldPreference.mLocale.equals(locale)) {
+                        final WordListPreference pref;
-                        // If the old preference has all the new attributes, reuse it. Ideally, we
+                        if (null != oldPreference
-                        // should reuse the old pref even if its status is different and call
+                                && oldPreference.mVersion == version
-                        // setStatus here, but setStatus calls Preference#setSummary() which needs
+                                && oldPreference.hasStatus(status)
-                        // to be done on the UI thread and we're not on the UI thread here. We
+                                && oldPreference.mLocale.equals(locale)) {
-                        // could do all this work on the UI thread, but in this case it's probably
+                            // If the old preference has all the new attributes, reuse it. Ideally,
-                        // lighter to stay on a background thread and throw this old preference out.
+                            // we should reuse the old pref even if its status is different and call
-                        pref = oldPreference;
+                            // setStatus here, but setStatus calls Preference#setSummary() which
-                    } else {
+                            // needs to be done on the UI thread and we're not on the UI thread
-                        // Otherwise, discard it and create a new one instead.
+                            // here. We could do all this work on the UI thread, but in this case
-                        // TODO: when the status is different from the old one, we need to
+                            // it's probably lighter to stay on a background thread and throw this
-                        // animate the old one out before animating the new one in.
+                            // old preference out.
-                        pref = new WordListPreference(activity, mDictionaryListInterfaceState,
+                            pref = oldPreference;
-                                mClientId, wordlistId, version, locale, description, status,
+                        } else {
-                                filesize);
+                            // Otherwise, discard it and create a new one instead.
+                            // TODO: when the status is different from the old one, we need to
+                            // animate the old one out before animating the new one in.
+                            pref = new WordListPreference(activity, mDictionaryListInterfaceState,
+                                    mClientId, wordlistId, version, locale, description, status,
+                                    filesize);
+                        }
+                        prefMap.put(key, pref);
                     }
-                    prefMap.put(key, pref);
+                } while (cursor.moveToNext());
-                }
+                mCurrentPreferenceMap = prefMap;
-            } while (cursor.moveToNext());
+                return prefMap.values();
+            }
+        } finally {
             cursor.close();
-            mCurrentPreferenceMap = prefMap;
-            return prefMap.values();
         }
     }
 

java/src/com/android/inputmethod/dictionarypack/MetadataDbHelper.java

@@ -533,12 +533,17 @@ public class MetadataDbHelper extends SQLiteOpenHelper {
                 PENDINGID_COLUMN + "= ?",
                 new String[] { Long.toString(id) },
                 null, null, null);
-        // There should never be more than one result. If because of some bug there are, returning
+        if (null == cursor) {
-        // only one result is the right thing to do, because we couldn't handle several anyway
+            return null;
-        // and we should still handle one.
+        }
-        final ContentValues result = getFirstLineAsContentValues(cursor);
+        try {
-        cursor.close();
+            // There should never be more than one result. If because of some bug there are,
-        return result;
+            // returning only one result is the right thing to do, because we couldn't handle
+            // several anyway and we should still handle one.
+            return getFirstLineAsContentValues(cursor);
+        } finally {
+            cursor.close();
+        }
     }
 
     /**
@@ -559,11 +564,16 @@ public class MetadataDbHelper extends SQLiteOpenHelper {
                 new String[] { id, Integer.toString(STATUS_INSTALLED),
                         Integer.toString(STATUS_DELETING) },
                 null, null, null);
-        // There should only be one result, but if there are several, we can't tell which
+        if (null == cursor) {
-        // is the best, so we just return the first one.
+            return null;
-        final ContentValues result = getFirstLineAsContentValues(cursor);
+        }
-        cursor.close();
+        try {
-        return result;
+            // There should only be one result, but if there are several, we can't tell which
+            // is the best, so we just return the first one.
+            return getFirstLineAsContentValues(cursor);
+        } finally {
+            cursor.close();
+        }
     }
 
     /**
@@ -622,10 +632,15 @@ public class MetadataDbHelper extends SQLiteOpenHelper {
                 METADATA_TABLE_COLUMNS,
                 WORDLISTID_COLUMN + "= ? AND " + VERSION_COLUMN + "= ?",
                 new String[] { id, Integer.toString(version) }, null, null, null);
-        // This is a lookup by primary key, so there can't be more than one result.
+        if (null == cursor) {
-        final ContentValues result = getFirstLineAsContentValues(cursor);
+            return null;
-        cursor.close();
+        }
-        return result;
+        try {
+            // This is a lookup by primary key, so there can't be more than one result.
+            return getFirstLineAsContentValues(cursor);
+        } finally {
+            cursor.close();
+        }
     }
 
     /**
@@ -641,10 +656,15 @@ public class MetadataDbHelper extends SQLiteOpenHelper {
                 METADATA_TABLE_COLUMNS,
                 WORDLISTID_COLUMN + "= ?",
                 new String[] { id }, null, null, VERSION_COLUMN + " DESC", "1");
-        // This is a lookup by primary key, so there can't be more than one result.
+        if (null == cursor) {
-        final ContentValues result = getFirstLineAsContentValues(cursor);
+            return null;
-        cursor.close();
+        }
-        return result;
+        try {
+            // This is a lookup by primary key, so there can't be more than one result.
+            return getFirstLineAsContentValues(cursor);
+        } finally {
+            cursor.close();
+        }
     }
 
     /**

java/src/com/android/inputmethod/dictionarypack/MetadataHandler.java

@@ -44,8 +44,7 @@ public class MetadataHandler {
      */
     private static List<WordListMetadata> makeMetadataObject(final Cursor results) {
         final ArrayList<WordListMetadata> buildingMetadata = new ArrayList<WordListMetadata>();
-
+        if (null != results && results.moveToFirst()) {
-        if (results.moveToFirst()) {
             final int localeColumn = results.getColumnIndex(MetadataDbHelper.LOCALE_COLUMN);
             final int typeColumn = results.getColumnIndex(MetadataDbHelper.TYPE_COLUMN);
             final int descriptionColumn =
@@ -61,7 +60,6 @@ public class MetadataHandler {
             final int versionIndex = results.getColumnIndex(MetadataDbHelper.VERSION_COLUMN);
             final int formatVersionIndex =
                     results.getColumnIndex(MetadataDbHelper.FORMATVERSION_COLUMN);
-
             do {
                 buildingMetadata.add(new WordListMetadata(results.getString(idIndex),
                         results.getInt(typeColumn),
@@ -75,8 +73,6 @@ public class MetadataHandler {
                         results.getInt(formatVersionIndex),
                         0, results.getString(localeColumn)));
             } while (results.moveToNext());
-
-            results.close();
         }
         return Collections.unmodifiableList(buildingMetadata);
     }
@@ -92,9 +88,14 @@ public class MetadataHandler {
         // If clientId is null, we get a cursor on the default database (see
         // MetadataDbHelper#getInstance() for more on this)
         final Cursor results = MetadataDbHelper.queryCurrentMetadata(context, clientId);
-        final List<WordListMetadata> resultList = makeMetadataObject(results);
+        // If null, we should return makeMetadataObject(null), so we go through.
-        results.close();
+        try {
-        return resultList;
+            return makeMetadataObject(results);
+        } finally {
+            if (null != results) {
+                results.close();
+            }
+        }
     }
 
     /**

java/src/com/android/inputmethod/latin/BinaryDictionaryFileDumper.java

@@ -142,7 +142,7 @@ public final class BinaryDictionaryFileDumper {
         final ContentProviderClient client = context.getContentResolver().
                 acquireContentProviderClient(getProviderUriBuilder("").build());
         if (null == client) return Collections.<WordListInfo>emptyList();
-
+        Cursor cursor = null;
         try {
             final Uri.Builder builder = getContentUriBuilderForType(clientId, client,
                     QUERY_PATH_DICT_INFO, locale.toString());
@@ -154,24 +154,22 @@ public final class BinaryDictionaryFileDumper {
             final boolean isProtocolV2 = (QUERY_PARAMETER_PROTOCOL_VALUE.equals(
                     queryUri.getQueryParameter(QUERY_PARAMETER_PROTOCOL)));
 
-            Cursor c = client.query(queryUri, DICTIONARY_PROJECTION, null, null, null);
+            cursor = client.query(queryUri, DICTIONARY_PROJECTION, null, null, null);
-            if (isProtocolV2 && null == c) {
+            if (isProtocolV2 && null == cursor) {
                 reinitializeClientRecordInDictionaryContentProvider(context, client, clientId);
-                c = client.query(queryUri, DICTIONARY_PROJECTION, null, null, null);
+                cursor = client.query(queryUri, DICTIONARY_PROJECTION, null, null, null);
             }
-            if (null == c) return Collections.<WordListInfo>emptyList();
+            if (null == cursor) return Collections.<WordListInfo>emptyList();
-            if (c.getCount() <= 0 || !c.moveToFirst()) {
+            if (cursor.getCount() <= 0 || !cursor.moveToFirst()) {
-                c.close();
                 return Collections.<WordListInfo>emptyList();
             }
             final ArrayList<WordListInfo> list = CollectionUtils.newArrayList();
             do {
-                final String wordListId = c.getString(0);
+                final String wordListId = cursor.getString(0);
-                final String wordListLocale = c.getString(1);
+                final String wordListLocale = cursor.getString(1);
                 if (TextUtils.isEmpty(wordListId)) continue;
                 list.add(new WordListInfo(wordListId, wordListLocale));
-            } while (c.moveToNext());
+            } while (cursor.moveToNext());
-            c.close();
             return list;
         } catch (RemoteException e) {
             // The documentation is unclear as to in which cases this may happen, but it probably
@@ -186,6 +184,9 @@ public final class BinaryDictionaryFileDumper {
             Log.e(TAG, "Unexpected exception communicating with the dictionary pack", e);
             return Collections.<WordListInfo>emptyList();
         } finally {
+            if (null != cursor) {
+                cursor.close();
+            }
             client.release();
         }
     }

java/src/com/android/inputmethod/latin/ContactsBinaryDictionary.java

@@ -139,23 +139,24 @@ public class ContactsBinaryDictionary extends ExpandableBinaryDictionary {
     }
 
     private void loadDictionaryAsyncForUri(final Uri uri) {
+        Cursor cursor = null;
         try {
-            Cursor cursor = mContext.getContentResolver()
+            cursor = mContext.getContentResolver().query(uri, PROJECTION, null, null, null);
-                    .query(uri, PROJECTION, null, null, null);
+            if (null == cursor) {
-            if (cursor != null) {
+                return;
-                try {
+            }
-                    if (cursor.moveToFirst()) {
+            if (cursor.moveToFirst()) {
-                        sContactCountAtLastRebuild = getContactCount();
+                sContactCountAtLastRebuild = getContactCount();
-                        addWords(cursor);
+                addWords(cursor);
-                    }
-                } finally {
-                    cursor.close();
-                }
             }
         } catch (final SQLiteException e) {
             Log.e(TAG, "SQLiteException in the remote Contacts process.", e);
         } catch (final IllegalStateException e) {
             Log.e(TAG, "Contacts DB is having problems", e);
+        } finally {
+            if (null != cursor) {
+                cursor.close();
+            }
         }
     }
 
@@ -186,18 +187,20 @@ public class ContactsBinaryDictionary extends ExpandableBinaryDictionary {
     private int getContactCount() {
         // TODO: consider switching to a rawQuery("select count(*)...") on the database if
         // performance is a bottleneck.
+        Cursor cursor = null;
         try {
-            final Cursor cursor = mContext.getContentResolver().query(
+            cursor = mContext.getContentResolver().query(Contacts.CONTENT_URI, PROJECTION_ID_ONLY,
-                    Contacts.CONTENT_URI, PROJECTION_ID_ONLY, null, null, null);
+                    null, null, null);
-            if (cursor != null) {
+            if (null == cursor) {
-                try {
+                return 0;
-                    return cursor.getCount();
-                } finally {
-                    cursor.close();
-                }
             }
+            return cursor.getCount();
         } catch (final SQLiteException e) {
             Log.e(TAG, "SQLiteException in the remote Contacts process.", e);
+        } finally {
+            if (null != cursor) {
+                cursor.close();
+            }
         }
         return 0;
     }
@@ -281,26 +284,27 @@ public class ContactsBinaryDictionary extends ExpandableBinaryDictionary {
         // Check all contacts since it's not possible to find out which names have changed.
         // This is needed because it's possible to receive extraneous onChange events even when no
         // name has changed.
-        Cursor cursor = mContext.getContentResolver().query(
+        final Cursor cursor = mContext.getContentResolver().query(Contacts.CONTENT_URI, PROJECTION,
-                Contacts.CONTENT_URI, PROJECTION, null, null, null);
+                null, null, null);
-        if (cursor != null) {
+        if (null == cursor) {
-            try {
+            return false;
-                if (cursor.moveToFirst()) {
+        }
-                    while (!cursor.isAfterLast()) {
+        try {
-                        String name = cursor.getString(INDEX_NAME);
+            if (cursor.moveToFirst()) {
-                        if (isValidName(name) && !isNameInDictionary(name)) {
+                while (!cursor.isAfterLast()) {
-                            if (DEBUG) {
+                    String name = cursor.getString(INDEX_NAME);
-                                Log.d(TAG, "Contact name missing: " + name + " (runtime = "
+                    if (isValidName(name) && !isNameInDictionary(name)) {
-                                        + (SystemClock.uptimeMillis() - startTime) + " ms)");
+                        if (DEBUG) {
-                            }
+                            Log.d(TAG, "Contact name missing: " + name + " (runtime = "
-                            return true;
+                                    + (SystemClock.uptimeMillis() - startTime) + " ms)");
                         }
-                        cursor.moveToNext();
+                        return true;
                     }
+                    cursor.moveToNext();
                 }
-            } finally {
-                cursor.close();
             }
+        } finally {
+            cursor.close();
         }
         if (DEBUG) {
             Log.d(TAG, "No contacts changed. (runtime = " + (SystemClock.uptimeMillis() - startTime)

java/src/com/android/inputmethod/latin/userdictionary/UserDictionaryList.java

@@ -61,12 +61,17 @@ public class UserDictionaryList extends PreferenceFragment {
         if (null == cursor) {
             // The user dictionary service is not present or disabled. Return null.
             return null;
-        } else if (cursor.moveToFirst()) {
+        }
-            final int columnIndex = cursor.getColumnIndex(UserDictionary.Words.LOCALE);
+        try {
-            do {
+            if (cursor.moveToFirst()) {
-                final String locale = cursor.getString(columnIndex);
+                final int columnIndex = cursor.getColumnIndex(UserDictionary.Words.LOCALE);
-                localeSet.add(null != locale ? locale : "");
+                do {
-            } while (cursor.moveToNext());
+                    final String locale = cursor.getString(columnIndex);
+                    localeSet.add(null != locale ? locale : "");
+                } while (cursor.moveToNext());
+            }
+        } finally {
+            cursor.close();
         }
         if (!UserDictionarySettings.IS_SHORTCUT_API_SUPPORTED) {
             // For ICS, we need to show "For all languages" in case that the keyboard locale

java/src/com/android/inputmethod/latin/userdictionary/UserDictionarySettings.java

@@ -140,6 +140,8 @@ public class UserDictionarySettings extends ListFragment {
         }
 
         mLocale = locale;
+        // WARNING: The following cursor is never closed! TODO: don't put that in a member, and
+        // make sure all cursors are correctly closed.
         mCursor = createCursor(locale);
         TextView emptyView = (TextView) getView().findViewById(android.R.id.empty);
         emptyView.setText(R.string.user_dict_settings_empty_text);