SQLiteBridge.getVersion leaks a cursor

RESOLVED FIXED in Firefox 32

Status

()

Firefox for Android
Data Providers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gcp, Assigned: Shashank VRSN Sabniveesu)

Tracking

Trunk
Firefox 32
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
OS: Linux → Android
Hardware: x86_64 → ARM
(Reporter)

Updated

4 years ago
Whiteboard: [mentor=gcp][lang=java][good first bug]
(Assignee)

Comment 1

4 years ago
Would a cursor.close() before the 'return' do?
(Reporter)

Comment 2

4 years ago
Yes, but you don't want to try to close it if it's null, so it'd have to go inside the if.
(Assignee)

Comment 3

4 years ago
Created attachment 8432093 [details] [diff] [review]
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?
Assignee: nobody → shashank
Attachment #8432093 - Flags: review?(gpascutto)
Flags: needinfo?(gpascutto)
(Reporter)

Comment 4

4 years ago
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+
(Reporter)

Updated

4 years ago
Flags: needinfo?(gpascutto)
(Assignee)

Comment 5

4 years ago
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?
(Assignee)

Comment 6

4 years ago
Created attachment 8432716 [details] [diff] [review]
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?
Attachment #8432093 - Attachment is obsolete: true
Attachment #8432093 - Flags: checkin?
Attachment #8432716 - Flags: checkin?
Flags: needinfo?(gpascutto)
(Reporter)

Comment 7

4 years ago
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+
(Reporter)

Updated

4 years ago
Flags: needinfo?(gpascutto)
Keywords: checkin-needed
(Reporter)

Comment 8

4 years ago
Created attachment 8433236 [details] [diff] [review]
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.
Attachment #8432716 - Attachment is obsolete: true
Attachment #8433236 - Flags: review+
(Reporter)

Comment 9

4 years ago
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=78395dea9803
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
Last Resolved: 4 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.