StrictMode violation: DatabaseObjectNotClosedException from AboutHomeContent.loadTopSites()

VERIFIED FIXED in Firefox 13

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
10 months ago

People

(Reporter: cpeterson, Assigned: sriram)

Tracking

Trunk
Firefox 14
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox15 verified, blocking-fennec1.0 beta+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
I intermittently hit this StrictMode warning when launching Fennec on my Galaxy Nexus:

E/StrictMode(17497): Finalizing a Cursor that has not been deactivated or closed. database = /data/data/org.mozilla.fennec_cpeterson/files/mozilla/j86osdsy.default/browser.db, table = history_with_images, query = SELECT _id, url, title, thumbnail FROM history_with_images WHERE ((deleted = 0) AND ((url NOT LIKE ? ) AND (url LIKE ? OR title LIKE ?))) ORDER BY visits * MAX(1, (date - 1331851335243) / 86400000 + 120) DESC LIMIT 4

E/StrictMode(17497): android.database.sqlite.DatabaseObjectNotClosedException: Application did not close the cursor or database object that was opened here
E/StrictMode(17497): 	at android.database.sqlite.SQLiteCursor.<init>(SQLiteCursor.java:98)
E/StrictMode(17497): 	at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:51)
E/StrictMode(17497): 	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1564)
E/StrictMode(17497): 	at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:354)
E/StrictMode(17497): 	at org.mozilla.fennec_cpeterson.db.BrowserProvider.query(BrowserProvider.java:1195)
E/StrictMode(17497): 	at android.content.ContentProvider$Transport.query(ContentProvider.java:178)
E/StrictMode(17497): 	at android.content.ContentResolver.query(ContentResolver.java:310)
E/StrictMode(17497): 	at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:123)
E/StrictMode(17497): 	at org.mozilla.gecko.db.LocalBrowserDB.getTopSites(LocalBrowserDB.java:150)
E/StrictMode(17497): 	at org.mozilla.gecko.db.BrowserDB.getTopSites(BrowserDB.java:115)
E/StrictMode(17497): 	at org.mozilla.gecko.AboutHomeContent.loadTopSites(AboutHomeContent.java:364)
E/StrictMode(17497): 	at org.mozilla.gecko.AboutHomeContent.access$400(AboutHomeContent.java:96)
E/StrictMode(17497): 	at org.mozilla.gecko.AboutHomeContent$9.run(AboutHomeContent.java:394)
E/StrictMode(17497): 	at android.os.Handler.handleCallback(Handler.java:605)
E/StrictMode(17497): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/StrictMode(17497): 	at android.os.Looper.loop(Looper.java:137)
E/StrictMode(17497): 	at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

E/StrictMode(17497): A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
E/StrictMode(17497): java.lang.Throwable: Explicit termination method 'close' not called
E/StrictMode(17497): 	at dalvik.system.CloseGuard.open(CloseGuard.java:184)
E/StrictMode(17497): 	at android.content.ContentResolver$CursorWrapperInner.<init>(ContentResolver.java:1581)
E/StrictMode(17497): 	at android.content.ContentResolver.query(ContentResolver.java:320)
E/StrictMode(17497): 	at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:123)
E/StrictMode(17497): 	at org.mozilla.gecko.db.LocalBrowserDB.getTopSites(LocalBrowserDB.java:150)
E/StrictMode(17497): 	at org.mozilla.gecko.db.BrowserDB.getTopSites(BrowserDB.java:115)
E/StrictMode(17497): 	at org.mozilla.gecko.AboutHomeContent.loadTopSites(AboutHomeContent.java:364)
E/StrictMode(17497): 	at org.mozilla.gecko.AboutHomeContent.access$400(AboutHomeContent.java:96)
E/StrictMode(17497): 	at org.mozilla.gecko.AboutHomeContent$9.run(AboutHomeContent.java:394)
E/StrictMode(17497): 	at android.os.Handler.handleCallback(Handler.java:605)
E/StrictMode(17497): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/StrictMode(17497): 	at android.os.Looper.loop(Looper.java:137)
E/StrictMode(17497): 	at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

Updated

5 years ago
blocking-fennec1.0: --- → ?
Assignee: nobody → sriram
blocking-fennec1.0: ? → beta+
(Assignee)

Comment 1

5 years ago
We should probably be moving the code out to something like TabsAccessor.

As per the documentation:
Warning: Do not call close() on cursor obtained from managedQuery(Uri, String[], String, String[], String), because the activity will do that for you at the appropriate time. However, if you call stopManagingCursor(Cursor) on a cursor from a managed query, the system will not automatically close the cursor and, in that case, you must call close().
(Assignee)

Comment 2

5 years ago
Created attachment 606746 [details] [diff] [review]
Patch: Option 1

This patch removes the managedQuery from the activity. The cursor is closed during onDestroy().

The cursor is _not_ closed for every cursor changed. It is left to the android to do GC.
Attachment #606746 - Flags: review?(mark.finkle)
(Assignee)

Comment 3

5 years ago
Created attachment 606748 [details] [diff] [review]
Patch: Option 2

This patch is same as option 1. In this case, every old cursor is closed and then assigned to the new cursor.
The cursor close _should_ happen in UI thread, as it takes care of the CursorAdapter we use. The cursor close triggers refreshing the adapter in UI thread. Closing the cursor in background thread will cause crashes due to thread safety.
Attachment #606748 - Flags: review?(mark.finkle)
Is there any benefit to Option 2 over Option 1?
(Assignee)

Comment 5

5 years ago
IIRC, close will trigger GC on the data held by cursor. The first option drops the data and GC will do it whenever it wants. Not sure if it can cause any leak. The second option explicitly informs GC that it's closing the cursor.
Attachment #606746 - Attachment is obsolete: true
Attachment #606746 - Flags: review?(mark.finkle)
Comment on attachment 606746 [details] [diff] [review]
Patch: Option 1

Let's go with the simple patch for now
Attachment #606746 - Flags: review+
Comment on attachment 606746 [details] [diff] [review]
Patch: Option 1

>     private void loadTopSites(final Activity activity) {
>-        if (mCursor != null)
>-            activity.stopManagingCursor(mCursor);
> 
>         // Ensure we initialize GeckoApp's startup mode in

Remove the extra blank line this creates
Attachment #606746 - Attachment is obsolete: false
Comment on attachment 606748 [details] [diff] [review]
Patch: Option 2

Let's wait and see if the GC can handle this well enough
Attachment #606748 - Attachment is obsolete: true
Attachment #606748 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/71416b7efdbd
https://hg.mozilla.org/mozilla-central/rev/71416b7efdbd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/releases/mozilla-aurora/rev/ddd5f1d10215
status-firefox13: --- → fixed
status-firefox14: --- → fixed
Verified fixed on:

Firefox 15.0a1 (2012-05-02)
Device: Samsung Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
status-firefox15: --- → verified
You need to log in before you can comment on or make changes to this bug.