Bug 1360151 - infer: NULL_DEREFERENCE in various db classes draft
authorAndrzej Hunt <ahunt@mozilla.com>
Thu, 27 Apr 2017 17:35:14 +0800
changeset 586572 9356aa6a830c0fdf8e0e0027a3a89f5545446d3f
parent 569701 2cca333f546f38860f84940d4c72d7470a3410f4
child 586573 b59bae2a61a309d6653e09b26bbada972819f94e
push id61470
push userahunt@mozilla.com
push dateTue, 30 May 2017 20:02:39 +0000
bugs1360151
milestone55.0a1
Bug 1360151 - infer: NULL_DEREFERENCE in various db classes MozReview-Commit-ID: LsWVewEZRhq
mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
@@ -1776,24 +1776,27 @@ public final class BrowserDatabaseHelper
         // url-annotations. This may change in future implementations, however currently we only need to care
         // about standard bookmarks (untouched during this migration) and bookmarks with a reader
         // view annotation (which we're creating here, and which are guaranteed to be saved offline).
         //
         // This is why we have to migrate the cache items (instead of cleaning the cache
         // and rebuilding it). We simply don't support uncached reader view bookmarks, and we would
         // break existing reading list items (they would convert into plain bookmarks without
         // reader view). This helps ensure that offline content isn't lost during the migration.
-        if (cacheDir.exists() && cacheDir.isDirectory()) {
+        final File[] cacheFiles = cacheDir.listFiles();
+        if (cacheDir.exists() &&
+                cacheDir.isDirectory() &&
+                cacheFiles != null) {
             SavedReaderViewHelper savedReaderViewHelper = SavedReaderViewHelper.getSavedReaderViewHelper(mContext);
 
             // Usually we initialise the helper during onOpen(). However onUpgrade() is run before
             // onOpen() hence we need to manually initialise it at this stage.
             savedReaderViewHelper.loadItems();
 
-            for (File cacheFile : cacheDir.listFiles()) {
+            for (File cacheFile : cacheFiles) {
                 if (fileToURLMap.containsKey(cacheFile.getName())) {
                     final String url = fileToURLMap.get(cacheFile.getName());
                     final String path = cacheFile.getAbsolutePath();
                     long size = cacheFile.length();
 
                     savedReaderViewHelper.put(url, path, size);
                 } else {
                     // This should never happen, but we don't actually know whether or not orphaned
--- a/mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
@@ -699,20 +699,21 @@ public class LocalBrowserDB extends Brow
         } else if ("favicons".equals(database)) {
             uri = mFaviconsUriWithProfile;
             columns = new String[] { Favicons._ID };
         }
 
         if (uri != null) {
             final Cursor cursor = cr.query(uri, columns, constraint, null, null);
 
-            try {
+            if (cursor != null) {
                 count = cursor.getCount();
-            } finally {
                 cursor.close();
+            } else {
+                count = 0;
             }
         }
 
         debug("Got count " + count + " for " + database);
         return count;
     }
 
     @Override
@@ -828,16 +829,21 @@ public class LocalBrowserDB extends Brow
         }
         // If we don't end with a trailing slash, then both https://foo.com and https://foo.company.biz will match.
         if (!prePath.endsWith("/")) {
             prePath = prePath + "/";
         }
         final Cursor cursor = cr.query(BrowserContract.History.CONTENT_URI,
                 new String[] { "MAX(" + BrowserContract.HistoryColumns.DATE_LAST_VISITED + ") AS date" },
                 BrowserContract.URLColumns.URL + " BETWEEN ? AND ?", new String[] { prePath, prePath + "\u007f" }, null);
+
+        if (cursor == null) {
+            return 0;
+        }
+
         try {
             cursor.moveToFirst();
             if (cursor.isAfterLast()) {
                 return 0;
             }
             return cursor.getLong(0);
         } finally {
             cursor.close();
@@ -1041,24 +1047,24 @@ public class LocalBrowserDB extends Brow
                                   new String[] { Bookmarks._ID },
                                   Bookmarks.PARENT + " = ? OR " +
                                   Bookmarks.PARENT + " = ? OR " +
                                   Bookmarks.PARENT + " = ?",
                                   new String[] { String.valueOf(getFolderIdFromGuid(cr, Bookmarks.TOOLBAR_FOLDER_GUID)),
                                                  String.valueOf(getFolderIdFromGuid(cr, Bookmarks.MENU_FOLDER_GUID)),
                                                  String.valueOf(getFolderIdFromGuid(cr, Bookmarks.UNFILED_FOLDER_GUID)) },
                                   null);
-
-        try {
+        if (c != null) {
             // Don't read back out of the cache to avoid races with invalidation.
             final boolean e = c.getCount() > 0;
             mDesktopBookmarksExist = e;
+            c.close();
             return e;
-        } finally {
-            c.close();
+        } else {
+            return false;
         }
     }
 
     @Override
     @RobocopTarget
     public boolean isBookmark(ContentResolver cr, String uri) {
         final Cursor c = cr.query(bookmarksUriWithLimit(1),
                                   new String[] { Bookmarks._ID },
@@ -1081,47 +1087,55 @@ public class LocalBrowserDB extends Brow
     @Override
     public String getUrlForKeyword(ContentResolver cr, String keyword) {
         final Cursor c = cr.query(mBookmarksUriWithProfile,
                                   new String[] { Bookmarks.URL },
                                   Bookmarks.KEYWORD + " = ?",
                                   new String[] { keyword },
                                   null);
         try {
-            if (!c.moveToFirst()) {
+            if (c == null || !c.moveToFirst()) {
                 return null;
             }
 
             return c.getString(c.getColumnIndexOrThrow(Bookmarks.URL));
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
     }
 
     private synchronized long getFolderIdFromGuid(final ContentResolver cr, final String guid) {
         if (mFolderIdMap.containsKey(guid)) {
             return mFolderIdMap.get(guid);
         }
 
         final Cursor c = cr.query(mBookmarksUriWithProfile,
                                   new String[] { Bookmarks._ID },
                                   Bookmarks.GUID + " = ?",
                                   new String[] { guid },
                                   null);
         try {
+            if (c == null || !c.moveToFirst()) {
+                return FOLDER_NOT_FOUND;
+            }
+
             final int col = c.getColumnIndexOrThrow(Bookmarks._ID);
-            if (!c.moveToFirst() || c.isNull(col)) {
+            if (c.isNull(col)) {
                 return FOLDER_NOT_FOUND;
             }
 
             final long id = c.getLong(col);
             mFolderIdMap.put(guid, id);
             return id;
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
     }
 
     private void addBookmarkItem(ContentResolver cr, String title, String uri, long folderId) {
         final long now = System.currentTimeMillis();
         ContentValues values = new ContentValues();
         if (title != null) {
             values.put(Bookmarks.TITLE, title);
@@ -1133,24 +1147,26 @@ public class LocalBrowserDB extends Brow
 
         // Get the page's favicon ID from the history table
         final Cursor c = cr.query(mHistoryUriWithProfile,
                                   new String[] { History.FAVICON_ID },
                                   History.URL + " = ?",
                                   new String[] { uri },
                                   null);
         try {
-            if (c.moveToFirst()) {
+            if (c != null && c.moveToFirst()) {
                 int columnIndex = c.getColumnIndexOrThrow(History.FAVICON_ID);
                 if (!c.isNull(columnIndex)) {
                     values.put(Bookmarks.FAVICON_ID, c.getLong(columnIndex));
                 }
             }
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
 
         // Restore deleted record if possible
         values.put(Bookmarks.IS_DELETED, 0);
 
         final Uri bookmarksWithInsert = mBookmarksUriWithProfile.buildUpon()
                                           .appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true")
                                           .build();
@@ -1296,30 +1312,32 @@ public class LocalBrowserDB extends Brow
                                   new String[] { Favicons.DATA },
                                   Favicons.URL + " = ? AND " + Favicons.DATA + " IS NOT NULL",
                                   new String[] { faviconURL },
                                   null);
 
         boolean shouldDelete = false;
         byte[] b = null;
         try {
-            if (!c.moveToFirst()) {
+            if (c == null || !c.moveToFirst()) {
                 return null;
             }
 
             final int faviconIndex = c.getColumnIndexOrThrow(Favicons.DATA);
             try {
                 b = c.getBlob(faviconIndex);
             } catch (IllegalStateException e) {
                 // This happens when the blob is more than 1MB: Bug 1106347.
                 // Delete that row.
                 shouldDelete = true;
             }
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
 
         if (shouldDelete) {
             try {
                 Log.d(LOGTAG, "Deleting invalid favicon.");
                 cr.delete(mFaviconsUriWithProfile,
                           Favicons.URL + " = ?",
                           new String[] { faviconURL });
@@ -1343,45 +1361,49 @@ public class LocalBrowserDB extends Brow
         // Check first in the history table.
         Cursor c = cr.query(mHistoryUriWithProfile,
                             new String[] { History.FAVICON_URL },
                             Combined.URL + " = ?",
                             new String[] { uri },
                             null);
 
         try {
-            if (c.moveToFirst()) {
+            if (c != null && c.moveToFirst()) {
                 // Interrupted page loads can leave History items without a valid favicon_id.
                 final int columnIndex = c.getColumnIndexOrThrow(History.FAVICON_URL);
                 if (!c.isNull(columnIndex)) {
                     final String faviconURL = c.getString(columnIndex);
                     if (faviconURL != null) {
                         return faviconURL;
                     }
                 }
             }
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
 
         // If that fails, check in the bookmarks table.
         c = cr.query(mBookmarksUriWithProfile,
                      new String[] { Bookmarks.FAVICON_URL },
                      Bookmarks.URL + " = ?",
                      new String[] { uri },
                      null);
 
         try {
-            if (c.moveToFirst()) {
+            if (c != null && c.moveToFirst()) {
                 return c.getString(c.getColumnIndexOrThrow(Bookmarks.FAVICON_URL));
             }
 
             return null;
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
     }
 
     @Override
     public boolean hideSuggestedSite(String url) {
         if (mSuggestedSites == null) {
             return false;
         }
@@ -1424,25 +1446,27 @@ public class LocalBrowserDB extends Brow
     @RobocopTarget
     public byte[] getThumbnailForUrl(ContentResolver cr, String uri) {
         final Cursor c = cr.query(mThumbnailsUriWithProfile,
                                   new String[]{ Thumbnails.DATA },
                                   Thumbnails.URL + " = ? AND " + Thumbnails.DATA + " IS NOT NULL",
                                   new String[]{ uri },
                                   null);
         try {
-            if (!c.moveToFirst()) {
+            if (c == null || !c.moveToFirst()) {
                 return null;
             }
 
             int thumbnailIndex = c.getColumnIndexOrThrow(Thumbnails.DATA);
 
             return c.getBlob(thumbnailIndex);
         } finally {
-            c.close();
+            if (c != null) {
+                c.close();
+            }
         }
 
     }
 
     /**
      * Query for non-null thumbnails matching the provided <code>urls</code>.
      * The returned cursor will have no more than, but possibly fewer than,
      * the requested number of thumbnails.
@@ -1981,17 +2005,18 @@ public class LocalBrowserDB extends Brow
                                          null,
                                          null,
                                          null);
 
         // It's possible that we will retrieve fewer sites than are required to fill the top-sites panel - in this case
         // we need to add "blank" tiles. It's much easier to add these here (as opposed to SQL), since we don't care
         // about their ordering (they go after all the other sites), but we do care about their number (and calculating
         // that inside out topsites SQL query would be difficult given the other processing we're already doing there).
-        final int blanksRequired = suggestedRangeLimit - topSitesCursor.getCount();
+        final int retrievedTopSitesCount = topSitesCursor != null ? topSitesCursor.getCount() : 0;
+        final int blanksRequired = suggestedRangeLimit - retrievedTopSitesCount;
 
         if (blanksRequired <= 0) {
             return topSitesCursor;
         }
 
         MatrixCursor blanksCursor = new MatrixCursor(new String[] {
                 TopSites._ID,
                 TopSites.BOOKMARK_ID,
--- a/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java
@@ -8,16 +8,17 @@ package org.mozilla.gecko.db;
 import android.content.Context;
 import android.content.ContentResolver;
 import android.content.SharedPreferences;
 import android.content.SharedPreferences.Editor;
 import android.database.Cursor;
 import android.database.MatrixCursor;
 import android.database.MatrixCursor.RowBuilder;
 import android.net.Uri;
+import android.support.annotation.NonNull;
 import android.text.TextUtils;
 import android.util.Log;
 
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStreamWriter;
@@ -170,19 +171,22 @@ public class SuggestedSites {
     }
 
     public SuggestedSites(Context appContext, Distribution distribution, File file) {
         this.context = appContext;
         this.distribution = distribution;
         this.cachedFile = file;
     }
 
-    synchronized File getFile() {
+    synchronized @NonNull File getFile() {
         if (cachedFile == null) {
             cachedFile = GeckoProfile.get(context).getFile(FILENAME);
+            if (cachedFile == null) {
+                throw new IllegalStateException("Unable to find SuggestedSites json");
+            }
         }
         return cachedFile;
     }
 
     private static boolean isNewLocale(Context context, Locale requestedLocale) {
         final SharedPreferences prefs = GeckoSharedPrefs.forProfile(context);
 
         String locale = prefs.getString(PREF_SUGGESTED_SITES_LOCALE_OLD, null);