1018089 - SQLiteBridge.getVersion leaks a cursor

Status: RESOLVED FIXED

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

Description

[Gian-Carlo Pascutto [:gcp]]

Bug 726821 switched SQLiteBridge to use Cursors, but it's actually leaking a Cursor here:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sqlite/SQLiteBridge.java#223

Comment 1

[Shashank VRSN Sabniveesu]

Would a cursor.close() before the 'return' do?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Yes, but you don't want to try to close it if it's null, so it'd have to go inside the if.

Comment 3

[Shashank VRSN Sabniveesu]

Attachment: BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp

I am not sure how to test if the submitted patch solves the issue reported. Any suggestions?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8432093 [details] [diff] [review]
BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp

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

Looks good. Maybe change the patch description to be more descriptive, i.e. "Close the SQLiteBridge Cursor in getVersion." or "Plug Cursor leak in SQLiteBridge.getVersion" or something. If you're done, add a checkin-needed keyword to the bug and the sheriff will commit it for you.

Note sure if this is testable. The caller is here: http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SQLiteBridgeContentProvider.java#87 and seems to run whenever form history, the homescreen or saved passwords are used.

Unclosed cursors are a potential problem because they can hold an underlying handle (like a SQLite query result) and exhaust those before the Java memory runs low and Garbage Collection is triggered. In this case, it's not actually an issue because of the specifics of how SQLiteBridge and MatrixBlobCursor work - but it *could become one* should the implementation ever change.

So, basically there's no good way to test this right now. I'm going to file a follow-up bug to add detection & warning for unclosed SQLiteBridge Cursors, just like Android itself does for it's own SQLiteCursors class.

Comment 5

[Shashank VRSN Sabniveesu]

Comment on attachment 8432093 [details] [diff] [review]
BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp

># HG changeset patch
># Parent 162d32ed3b9c7071cbb394ed7b54ef3dbf1dc5c5
># User Shashank Sabniveesu <[e-mail]>
>BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp
>
>diff --git a/mobile/android/base/sqlite/SQLiteBridge.java b/mobile/android/base/sqlite/SQLiteBridge.java
>--- a/mobile/android/base/sqlite/SQLiteBridge.java
>+++ b/mobile/android/base/sqlite/SQLiteBridge.java
>@@ -214,16 +214,17 @@ public class SQLiteBridge {
>     public int getVersion()
>                throws SQLiteBridgeException {
>         Cursor cursor = internalQuery("PRAGMA user_version", null);
>         int ret = -1;
>         if (cursor != null) {
>             cursor.moveToFirst();
>             String version = cursor.getString(0);
>             ret = Integer.parseInt(version);
>+	    cursor.close();
>         }
>         return ret;
>     }
> 
>     // Do an SQL query, substituting the parameters in the query with the passed
>     // parameters. The parameters are subsituded in order, so named parameters
>     // are not supported.
>     private Cursor internalQuery(String aQuery, String[] aParams)

Comment 6

[Shashank VRSN Sabniveesu]

Attachment: BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp

I am submitting a new patch with an updated description. Do I need to set an 'r?' again?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8432716 [details] [diff] [review]
BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp

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

No, once you have an r+ you can set the review+ on updated versions of the patch yourself. Unless you end up changing it significantly, in which case you'd want to ask a re-review, but that's obviously not the case here.

checkin-needed is a keyword, not a patch flag. I'll set it for you.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch v2. Close the SQLiteBridge Cursor in getVersion r=gcp

Patch didn't apply to current m-c because some code has moved around, rebasing.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=78395dea9803

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/509dcb2fd26b