1064506 - Eradicate redundant null checks from BrowserDB

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Chris Kitching [:ckitching]]

Richard pointed out in another bug the following antipattern:

  Foo x;
  try {
    x = bar();
  } finally {
    if (x != null) {
      x.close();
    }
  }

Unfortunately, BrowserDB is full of it. Lots of it is my fault. Let's fix that.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Eradicate lazy cursor pattern

Comment 2

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=7927a5f243b2

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8485960 [details] [diff] [review]
Eradicate lazy cursor pattern

Review of attachment 8485960 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/LocalBrowserDB.java
@@ +540,5 @@
>              uri = mFaviconsUriWithProfile;
>              columns = new String[] { Favicons._ID };
>          }
>          if (uri != null) {
> +            Cursor cursor = cr.query(uri, columns, constraint, null, null);

final

@@ +735,3 @@
>  
> +        // Check to see if there are any bookmarks in one of our three
> +        // fixed "Desktop Boomarks" folders.

Bookmarks

@@ +735,4 @@
>  
> +        // Check to see if there are any bookmarks in one of our three
> +        // fixed "Desktop Boomarks" folders.
> +        Cursor c = cr.query(bookmarksUriWithLimit(1),

final

@@ +739,3 @@
>                           new String[] { Bookmarks._ID },
>                           Bookmarks.PARENT + " = ? OR " +
>                           Bookmarks.PARENT + " = ? OR " +

Correct indentation in this block.

@@ +755,5 @@
>      }
>  
>      @RobocopTarget
>      public boolean isBookmark(ContentResolver cr, String uri) {
> +        Cursor c = cr.query(bookmarksUriWithLimit(1),

final

@@ +758,5 @@
>      public boolean isBookmark(ContentResolver cr, String uri) {
> +        Cursor c = cr.query(bookmarksUriWithLimit(1),
> +                            new String[] { Bookmarks._ID },
> +                            Bookmarks.URL + " = ? AND " +
> +                                    Bookmarks.PARENT + " != ?",

Correct indenting.

@@ +761,5 @@
> +                            Bookmarks.URL + " = ? AND " +
> +                                    Bookmarks.PARENT + " != ?",
> +                            new String[] { uri,
> +                                    String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) },
> +                            Bookmarks.URL);

Same.

@@ +777,4 @@
>      }
>  
>      public boolean isReadingListItem(ContentResolver cr, String uri) {
> +        Cursor c = cr.query(mReadingListUriWithProfile,

final

@@ +822,5 @@
>          }
>      }
>  
>      public String getUrlForKeyword(ContentResolver cr, String keyword) {
> +        Cursor c = cr.query(mBookmarksUriWithProfile,

final (might fix the indenting, too!)

@@ +837,3 @@
>          }
>  
>          return null;

Move this inside the `try`, early return null if !c.moveToFirst.

@@ +888,5 @@
>          values.put(Bookmarks.PARENT, folderId);
>          values.put(Bookmarks.DATE_MODIFIED, now);
>  
>          // Get the page's favicon ID from the history table
> +        Cursor c = cr.query(mHistoryUriWithProfile,

final.

@@ +1002,5 @@
>       * @param faviconURL The URL of the favicon to fetch from the database.
>       * @return The decoded Bitmap from the database, if any. null if none is stored.
>       */
>      public LoadFaviconResult getFaviconForUrl(ContentResolver cr, String faviconURL) {
> +        Cursor c = cr.query(mFaviconsUriWithProfile,

final

@@ +1028,5 @@
>          return FaviconDecoder.decodeFavicon(b);
>      }
>  
>      public String getFaviconUrlForHistoryUrl(ContentResolver cr, String uri) {
> +        Cursor c = cr.query(mHistoryUriWithProfile,

final

@@ +1044,3 @@
>          }
>  
>          return null;

Same comment as above.

@@ +1095,5 @@
>      }
>  
>      @RobocopTarget
>      public byte[] getThumbnailForUrl(ContentResolver cr, String uri) {
> +        Cursor c = cr.query(mThumbnailsUriWithProfile,

final

@@ +1162,5 @@
> +        };
> +
> +
> +        // We need to get the old visit count.
> +        Cursor cursor = cr.query(getAllHistoryUri(),

final

::: mobile/android/base/moz.build
@@ +15,5 @@
>  ajar.sources = [
>      'annotations/generatorannotations/OptionalGeneratedParameter.java',
>      'annotations/generatorannotations/WrapElementForJNI.java',
>      'annotations/generatorannotations/WrapEntireClassForJNI.java',
> +    'annotations/JNITarget.java',

Looks like this should be in a different patch.

Comment 4

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/f0edea26db81

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0edea26db81

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I stumbled upon an exception during a test run that looks like an NPE is being fired from the | finally | block because the cursor is null. Comment 0 doesn't give enough context to know why this is an anti-pattern:

W/dalvikvm( 4051): threadid=29: thread exiting with uncaught exception (group=0x40b741f8)
E/GeckoAppShell( 4051): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 164 ("ModernAsyncTask #3")
E/GeckoAppShell( 4051): java.lang.NullPointerException
E/GeckoAppShell( 4051): 	at org.mozilla.gecko.db.LocalBrowserDB.desktopBookmarksExist(LocalBrowserDB.java:755)
E/GeckoAppShell( 4051): 	at org.mozilla.gecko.db.LocalBrowserDB.getBookmarksInFolder(LocalBrowserDB.java:681)
E/GeckoAppShell( 4051): 	at org.mozilla.gecko.db.BrowserDB.getBookmarksInFolder(BrowserDB.java:156)
E/GeckoAppShell( 4051): 	at org.mozilla.gecko.home.BookmarksPanel$BookmarksLoader.loadCursor(BookmarksPanel.java:196)
E/GeckoAppShell( 4051): 	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:44)
E/GeckoAppShell( 4051): 	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:26)
E/GeckoAppShell( 4051): 	at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:242)
E/GeckoAppShell( 4051): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
E/GeckoAppShell( 4051): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
E/GeckoAppShell( 4051): 	at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
E/GeckoAppShell( 4051): 	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:305)
E/GeckoAppShell( 4051): 	at java.util.concurrent.FutureTask.run(FutureTask.java:137)
E/GeckoAppShell( 4051): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
E/GeckoAppShell( 4051): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
E/GeckoAppShell( 4051): 	at java.lang.Thread.run(Thread.java:856)

Full log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48815950&tree=Try&full=1

Comment 7

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> I stumbled upon an exception during a test run that looks like an NPE is
> being fired from the | finally | block because the cursor is null. Comment 0
> doesn't give enough context to know why this is an anti-pattern:

It's an anti-pattern because:

1. The only time you should get a null instead of a cursor is if you have a syntax error in your query string. That's not something we want to muffle.

2. Usually the pattern looks like this:

  Cursor c = null;
  try {
    c = cr.query(...);
    c.moveToFirst();
    ...
  } finally {
    if (c != null) {
      c.close();
    }
  }

which will result in a NPE regardless of that finally clause!

If you're going to null check in this scenario, this is how to do it correctly:

  Cursor c = cr.query(...);
  if (c == null) {
    Log.e(LOGTAG, "Oh no!");
    return 0;
  }

  try {
    c.moveToFirst();
    ...
  } finally {
    c.close();
  }