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

4 files changed, 114 insertions, 84 deletions

diff --git a/java/src/com/android/inputmethod/dictionarypack/DictionaryProvider.java b/java/src/com/android/inputmethod/dictionarypack/DictionaryProvider.java
index 1d9b9991e..80def701d 100644
--- a/java/src/com/android/inputmethod/dictionarypack/DictionaryProvider.java
+++ b/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();
         }
     }
 
diff --git a/java/src/com/android/inputmethod/dictionarypack/DictionarySettingsFragment.java b/java/src/com/android/inputmethod/dictionarypack/DictionarySettingsFragment.java
index d18639741..dae2f22a4 100644
--- a/java/src/com/android/inputmethod/dictionarypack/DictionarySettingsFragment.java
+++ b/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>();
-            result.add(createErrorMessage(activity, R.string.no_dictionaries_available));
-            cursor.close();
-            return result;
-        } else {
-            final String systemLocaleString = Locale.getDefault().toString();
-            final TreeMap<String, WordListPreference> prefMap =
-                    new TreeMap<String, WordListPreference>();
-            final int idIndex = cursor.getColumnIndex(MetadataDbHelper.WORDLISTID_COLUMN);
-            final int versionIndex = cursor.getColumnIndex(MetadataDbHelper.VERSION_COLUMN);
-            final int localeIndex = cursor.getColumnIndex(MetadataDbHelper.LOCALE_COLUMN);
-            final int descriptionIndex = cursor.getColumnIndex(MetadataDbHelper.DESCRIPTION_COLUMN);
-            final int statusIndex = cursor.getColumnIndex(MetadataDbHelper.STATUS_COLUMN);
-            final int filesizeIndex = cursor.getColumnIndex(MetadataDbHelper.FILESIZE_COLUMN);
-            do {
-                final String wordlistId = cursor.getString(idIndex);
-                final int version = cursor.getInt(versionIndex);
-                final String localeString = cursor.getString(localeIndex);
-                final Locale locale = new Locale(localeString);
-                final String description = cursor.getString(descriptionIndex);
-                final int status = cursor.getInt(statusIndex);
-                final int matchLevel = LocaleUtils.getMatchLevel(systemLocaleString, localeString);
-                final String matchLevelString = LocaleUtils.getMatchLevelSortedString(matchLevel);
-                final int filesize = cursor.getInt(filesizeIndex);
-                // The key is sorted in lexicographic order, according to the match level, then
-                // the description.
-                final String key = matchLevelString + "." + description + "." + wordlistId;
-                final WordListPreference existingPref = prefMap.get(key);
-                if (null == existingPref || existingPref.hasPriorityOver(status)) {
-                    final WordListPreference oldPreference = mCurrentPreferenceMap.get(key);
-                    final WordListPreference pref;
-                    if (null != oldPreference
-                            && oldPreference.mVersion == version
-                            && oldPreference.hasStatus(status)
-                            && oldPreference.mLocale.equals(locale)) {
-                        // If the old preference has all the new attributes, reuse it. Ideally, we
-                        // should reuse the old pref even if its status is different and call
-                        // setStatus here, but setStatus calls Preference#setSummary() which needs
-                        // to be done on the UI thread and we're not on the UI thread here. We
-                        // could do all this work on the UI thread, but in this case it's probably
-                        // lighter to stay on a background thread and throw this old preference out.
-                        pref = oldPreference;
-                    } else {
-                        // 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);
+        }
+        try {
+            if (!cursor.moveToFirst()) {
+                final ArrayList<Preference> result = new ArrayList<Preference>();
+                result.add(createErrorMessage(activity, R.string.no_dictionaries_available));
+                return result;
+            } else {
+                final String systemLocaleString = Locale.getDefault().toString();
+                final TreeMap<String, WordListPreference> prefMap =
+                        new TreeMap<String, WordListPreference>();
+                final int idIndex = cursor.getColumnIndex(MetadataDbHelper.WORDLISTID_COLUMN);
+                final int versionIndex = cursor.getColumnIndex(MetadataDbHelper.VERSION_COLUMN);
+                final int localeIndex = cursor.getColumnIndex(MetadataDbHelper.LOCALE_COLUMN);
+                final int descriptionIndex =
+                        cursor.getColumnIndex(MetadataDbHelper.DESCRIPTION_COLUMN);
+                final int statusIndex = cursor.getColumnIndex(MetadataDbHelper.STATUS_COLUMN);
+                final int filesizeIndex = cursor.getColumnIndex(MetadataDbHelper.FILESIZE_COLUMN);
+                do {
+                    final String wordlistId = cursor.getString(idIndex);
+                    final int version = cursor.getInt(versionIndex);
+                    final String localeString = cursor.getString(localeIndex);
+                    final Locale locale = new Locale(localeString);
+                    final String description = cursor.getString(descriptionIndex);
+                    final int status = cursor.getInt(statusIndex);
+                    final int matchLevel =
+                            LocaleUtils.getMatchLevel(systemLocaleString, localeString);
+                    final String matchLevelString =
+                            LocaleUtils.getMatchLevelSortedString(matchLevel);
+                    final int filesize = cursor.getInt(filesizeIndex);
+                    // The key is sorted in lexicographic order, according to the match level, then
+                    // the description.
+                    final String key = matchLevelString + "." + description + "." + wordlistId;
+                    final WordListPreference existingPref = prefMap.get(key);
+                    if (null == existingPref || existingPref.hasPriorityOver(status)) {
+                        final WordListPreference oldPreference = mCurrentPreferenceMap.get(key);
+                        final WordListPreference pref;
+                        if (null != oldPreference
+                                && oldPreference.mVersion == version
+                                && oldPreference.hasStatus(status)
+                                && oldPreference.mLocale.equals(locale)) {
+                            // If the old preference has all the new attributes, reuse it. Ideally,
+                            // we should reuse the old pref even if its status is different and call
+                            // setStatus here, but setStatus calls Preference#setSummary() which
+                            // needs to be done on the UI thread and we're not on the UI thread
+                            // here. We could do all this work on the UI thread, but in this case
+                            // it's probably lighter to stay on a background thread and throw this
+                            // old preference out.
+                            pref = oldPreference;
+                        } else {
+                            // 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());
+                } while (cursor.moveToNext());
+                mCurrentPreferenceMap = prefMap;
+                return prefMap.values();
+            }
+        } finally {
             cursor.close();
-            mCurrentPreferenceMap = prefMap;
-            return prefMap.values();
         }
     }
 
diff --git a/java/src/com/android/inputmethod/dictionarypack/MetadataDbHelper.java b/java/src/com/android/inputmethod/dictionarypack/MetadataDbHelper.java
index ff5aba6d8..8badaf4b9 100644
--- a/java/src/com/android/inputmethod/dictionarypack/MetadataDbHelper.java
+++ b/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
-        // only one result is the right thing to do, because we couldn't handle several anyway
-        // and we should still handle one.
-        final ContentValues result = getFirstLineAsContentValues(cursor);
-        cursor.close();
-        return result;
+        if (null == cursor) {
+            return null;
+        }
+        try {
+            // There should never be more than one result. If because of some bug there are,
+            // 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
-        // is the best, so we just return the first one.
-        final ContentValues result = getFirstLineAsContentValues(cursor);
-        cursor.close();
-        return result;
+        if (null == cursor) {
+            return null;
+        }
+        try {
+            // 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.
-        final ContentValues result = getFirstLineAsContentValues(cursor);
-        cursor.close();
-        return result;
+        if (null == cursor) {
+            return null;
+        }
+        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.
-        final ContentValues result = getFirstLineAsContentValues(cursor);
-        cursor.close();
-        return result;
+        if (null == cursor) {
+            return null;
+        }
+        try {
+            // This is a lookup by primary key, so there can't be more than one result.
+            return getFirstLineAsContentValues(cursor);
+        } finally {
+            cursor.close();
+        }
     }
 
     /**
diff --git a/java/src/com/android/inputmethod/dictionarypack/MetadataHandler.java b/java/src/com/android/inputmethod/dictionarypack/MetadataHandler.java
index a0147b6d6..5c2289911 100644
--- a/java/src/com/android/inputmethod/dictionarypack/MetadataHandler.java
+++ b/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 (results.moveToFirst()) {
+        if (null != results && 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);
-        results.close();
-        return resultList;
+        // If null, we should return makeMetadataObject(null), so we go through.
+        try {
+            return makeMetadataObject(results);
+        } finally {
+            if (null != results) {
+                results.close();
+            }
+        }
     }
 
     /**