Closed Bug 1018089 Opened 6 years ago Closed 6 years ago

SQLiteBridge.getVersion leaks a cursor

Categories

(Firefox for Android :: Data Providers, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: gcp, Assigned: shashank)

Details

(Whiteboard: [mentor=gcp][lang=java][good first bug])

Attachments

(1 file, 2 obsolete files)

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
OS: Linux → Android
Hardware: x86_64 → ARM
Whiteboard: [mentor=gcp][lang=java][good first bug]
Would a cursor.close() before the 'return' do?
Yes, but you don't want to try to close it if it's null, so it'd have to go inside the if.
I am not sure how to test if the submitted patch solves the issue reported. Any suggestions?
Assignee: nobody → shashank
Attachment #8432093 - Flags: review?(gpascutto)
Flags: needinfo?(gpascutto)
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.
Attachment #8432093 - Flags: review?(gpascutto) → review+
Flags: needinfo?(gpascutto)
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 <shashank@linux.com>
>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)
Attachment #8432093 - Attachment description: BUG 1018089 - close the cursor while returning r=gcp → BUG 1018089 - Close the SQLiteBridge Cursor in getVersion r=gcp
Attachment #8432093 - Flags: checkin?
I am submitting a new patch with an updated description. Do I need to set an 'r?' again?
Attachment #8432093 - Attachment is obsolete: true
Attachment #8432093 - Flags: checkin?
Attachment #8432716 - Flags: checkin?
Flags: needinfo?(gpascutto)
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.
Attachment #8432716 - Flags: checkin? → review+
Flags: needinfo?(gpascutto)
Keywords: checkin-needed
Patch didn't apply to current m-c because some code has moved around, rebasing.
Attachment #8432716 - Attachment is obsolete: true
Attachment #8433236 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/509dcb2fd26b
Keywords: checkin-needed
Whiteboard: [mentor=gcp][lang=java][good first bug] → [mentor=gcp][lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/509dcb2fd26b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=gcp][lang=java][good first bug][fixed-in-fx-team] → [mentor=gcp][lang=java][good first bug]
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.