887820 - LocalBrowserDB.java has many functions with potentially unclosed cursors

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

This function doesn't close the cursor if getCount() throws some sort of SQL exception: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#496

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Ditto for isReadingListItem().

getUrlForKeyword could also have an unclosed cursor if the cursor.getString call throws.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

addBookmarkItem, getFaviconForUrl, getFaviconUrlForHistoryUrl, getThumbnailForUrl, isVisited, and getBookmarkForUrl all also have cases that can result in unclosed cursors.

Comment 3

[:Margaret Leibovic]

Chris, this sounds similar to one of the issues you were talking about yesterday. Maybe you would want to help fix it :)

Comment 4

[Chris Kitching [:ckitching]]

Attachment: Patch closing cursors properly in LocalBrowserDB and callers thereof

Turns out there were many instances of Cursors not being closed properly - while such things are generally caught and dealt with by the appropriate finaliser when the garbage collector runs, this is not something that is good to depend on - the database will sit there hogging resources relating to a defunct cursor until the finaliser eventually gets processed (And then you get an annoying strict mode error as well). 
Still. Here´s a patch fixing as many of these problems as I can find starting from the class mentioned. Enjoy.

Comment 5

[:Margaret Leibovic]

Comment on attachment 769161 [details] [diff] [review]
Patch closing cursors properly in LocalBrowserDB and callers thereof

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

Nice work, it's good to see this getting cleaned up. This generally looks good to me, but I'd like to see a new version before we land this.

Also, after we land this patch, you should send a post to [e-mail] describing the patterns we should follow to make sure we close cursors correctly. I wouldn't want us to re-introduce more of these bugs :)

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +873,2 @@
>          try {
> +            if (c.moveToFirst()) {

I would prefer to see us just return early if (!c.moveToFirst()), to avoid an extra level of indentation below.

::: mobile/android/base/db/BrowserDB.java
@@ +332,5 @@
>          public void setPinnedSites(Cursor c) {
>              mPinnedSites = new SparseArray<PinnedSite>();
> +
> +            try {
> +                if (c != null && c.getCount() > 0) {

You could just return up above if (c == null), similarly to what you did in AllPagesTab, since that would let you avoid doing the null check in the finally block as well.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +212,5 @@
>                  cursor = cr.query(uri, columns, constraint, null, null);
>                  count = cursor.getCount();
> +            } finally {
> +                if (cursor != null)
> +                    cursor.close();

I might argue that the change in here is out of scope for this patch, since it isn't changing the behavior, but I think it's nice cleanup to make the try block only as big as it needs to be, so we can keep this in the patch.

@@ +547,5 @@
>      }
>  
>      @Override
>      public String getUrlForKeyword(ContentResolver cr, String keyword) {
> +        Cursor cursor = null;

Nit: Since you're touching all the lines in here anyway, let's rename this variable to c to be consistent with the most of the other methods in here.

@@ +559,5 @@
> +                              null);
> +
> +            if (cursor.moveToFirst()) {
> +                url = cursor.getString(cursor.getColumnIndexOrThrow(Bookmarks.URL));
> +            }

Style nit: In our Java code we generally do like to brace single-line if statements, but it appears they're not braced in this file, so we should be consistent with that.

@@ +1270,5 @@
>              Log.e(LOGTAG, "NullPointerException in isVisited");
> +        } finally {
> +            if (c != null) {
> +                c.close();
> +            }

Same nit here about the braces.

@@ +1277,5 @@
>          return (count > 0);
>      }
>  
>      public Cursor getBookmarkForUrl(ContentResolver cr, String url) {
>          Cursor c = cr.query(bookmarksUriWithLimit(1),

Why didn't you create a try/finally around this?

::: mobile/android/base/widget/TopSitesView.java
@@ -307,5 @@
>          Map<String, Bitmap> thumbnails = new HashMap<String, Bitmap>();
>  
>          try {
> -            if (c == null || !c.moveToFirst())
> -                return thumbnails;

I think we should just keep the early return as it is. Actually that looks like the only change you made in here, and that's out of scope for this bug, since the finally block here still closes the cursor.

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Patch destroying less blame and less sanity.

Thanks for the feedback! I appear to have had a moment of madness while considering what happens when you return from inside a try. Whoops!

The change for which you asked why I didn`t create a try..finally is sort of interesting. That method is returning a Cursor to be used by the caller, so we do not want to close it before returning it, or the caller can`t do what they want to do with it. It might be considered better practice to not pass Cursors around in this way (because when doing so it can be quite easy to forget to close them) but to instead have methods like this one consume the Cursor and spit out some datastructure that isn`t a Cursor (So doesn`t have database resources allocated behind it). Depending on what you`re trying to do with it this may be either less efficient or just not possible.

Anyway - in this case a try..finally would not be appropriate because we are not consuming the Cursor here and wanting to make sure it is closed (quite the opposite, in fact - we want to create it and then leave it unclosed so the caller can consume it). In general this is not sufficient to warrant not having a try - if we had some code along the lines of:

Cursor c = cr.query(...);
c.doSomething();
c.doSomethingElse();
...
return c;

We would need to wrap this in a try..catch block in the event that doSomething() or doSomethingElse() throw an exception. If they did so, we would jump out of our function after creating the Cursor but before returning it to the caller normally. Returning a Cursor is only okay if the caller is going to handle it properly and close it when they`re done with it - but if our function gets interrupted by an exception like this then our Cursor gets created but never handed to the caller, so it never gets closed by them, so it leaks, causing sadness.

However, in this particular case all we have is:

Cursor = cr.query(...);

if (c != null && c.getCount() == 0) {
    c.close();
    c = null;
}
return c;

Under normal circumstances, query returns a Cursor and the if statement bins it if it had zero rows (Because we`re not interested in such results). No problem - the Cursor is either closed right away if it had zero rows or it is closed by the caller later (Under our assumption that the calling code is written correctly (The fact we have to make this assumption is why passing Cursors around like this is kinda risky)).
In the event that an exception is thrown from query, we actually don`t care (Much) - if query had allocated some database resources internally before it threw an exception, it`s its job to free them as appropriate on error - such things are not in our scope. We only get access to the Cursor (And thus the database resources it represents that we are so interested in freeing at the proper time) after query() has returned successfully. If query has thrown an exception, we don`t have a Cursor to call free on (The assignment in our method never even happened), so there`s nothing we can meaningfully put into any catch or finally block on this code from the point of view of avoiding Cursor leaks.

I`ve applied all your other corrections to this new patch - I`m now trampling slightly less of your blame. Enjoy!

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Bug 887820 - Closing cursors in LocalBrowserDB and its users. r=margaret.leibovic

Pruning annoying concatenated commit messages...

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 769788 [details] [diff] [review]
Bug 887820 - Closing cursors in LocalBrowserDB and its users. r=margaret.leibovic

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

Nice work! I did a quick drive-by look at your patch and it looks good overall. Found a few nits, listed below.

::: mobile/android/base/db/BrowserDB.java
@@ +331,5 @@
>  
>          public void setPinnedSites(Cursor c) {
>              mPinnedSites = new SparseArray<PinnedSite>();
> +
> +            if(c == null)

nit: space before '('

::: mobile/android/base/db/LocalBrowserDB.java
@@ +558,5 @@
> +                         new String[] { keyword },
> +                         null);
> +
> +            if (c.moveToFirst())
> +                url = c.getString(c.getColumnIndexOrThrow(Bookmarks.URL));

You can just return c.getString directly here, don't need the local variable at all.

@@ +772,5 @@
> +                         new String[] { uri },
> +                         null);
> +
> +            if (c.moveToFirst())
> +                faviconUrl = c.getString(c.getColumnIndexOrThrow(History.FAVICON_URL));

Can return directly here

Comment 10

[:Margaret Leibovic]

Comment on attachment 769788 [details] [diff] [review]
Bug 887820 - Closing cursors in LocalBrowserDB and its users. r=margaret.leibovic

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

Clearing review to see a new patch that address's kats's comments. You can flag either one of us for the final review.

::: mobile/android/base/db/BrowserDB.java
@@ +332,5 @@
>          public void setPinnedSites(Cursor c) {
>              mPinnedSites = new SparseArray<PinnedSite>();
> +
> +            if(c == null)
> +                return;

Also nit: Add braces around this if statement to be consistent with the if statement down below.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Bug 887820 - Closing cursors in LocalBrowserDB and its users. r=margaret.leibovic

Comment 12

[:Margaret Leibovic]

Comment on attachment 770283 [details] [diff] [review]
Bug 887820 - Closing cursors in LocalBrowserDB and its users. r=margaret.leibovic

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

You can just upload a new version of this patch with these comments addressed, then mark the bug for check-in.

However, before doing that, I think you should push this patch to try server to make sure it doesn't break anything. Have you gotten your level 1 commit access yet? There are details about try server here:
https://wiki.mozilla.org/ReleaseEngineering/TryServer

::: mobile/android/base/db/LocalBrowserDB.java
@@ +484,5 @@
>                           new String[] { Bookmarks._ID },
>                           Bookmarks.PARENT + " = ?",
>                           new String[] { String.valueOf(Bookmarks.FIXED_READING_LIST_ID) },
>                           null);
> +            return  c.getCount();

Nit: Only include one space between return and c.

Also, it looks like you forgot to add a return 0 down at the bottom of this method.

Comment 13

[Chenxia Liu [:liuche]]

try build: https://tbpl.mozilla.org/?tree=Try&rev=35c16c5a09d1

Comment 14

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #13)
> try build: https://tbpl.mozilla.org/?tree=Try&rev=35c16c5a09d1

Looks like there was a problem with the trychooser syntax... no unit tests ran.

I usually do:
try: -b o -p android,android-armv6,android-noion,android-x86 -u all -t none

Comment 15

[Stefan Mirea [:smirea]]

pushed to try for :ckitching
https://tbpl.mozilla.org/?tree=Try&rev=fe663fd0688d

Comment 16

[Chris Kitching [:ckitching]]

Attachment: 770283: Bug 887820 - Closing cursors in LocalBrowserDB and its users. r=margaret.leibovic (Denitted)

Removed that space. Try seems not to have caught fire. Charrge!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e996d5cd61

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Stefan Mirea [:smirea] from comment #15)
> pushed to try for :ckitching
> https://tbpl.mozilla.org/?tree=Try&rev=fe663fd0688d

For the record, using '-p all' was unnecessary since this is Android-only code. Please be considerate of that in the future to save build and test resources.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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