736296 - StrictMode violation: DatabaseObjectNotClosedException from AboutHomeContent.loadTopSites()

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

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)

Comment 1

[Sriram Ramasubramanian [:sriram]]

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().

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: 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.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: 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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Is there any benefit to Option 2 over Option 1?

Comment 5

[Sriram Ramasubramanian [:sriram]]

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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 606746 [details] [diff] [review]
Patch: Option 1

Let's go with the simple patch for now

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 606748 [details] [diff] [review]
Patch: Option 2

Let's wait and see if the GC can handle this well enough

Comment 9

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/71416b7efdbd

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71416b7efdbd

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ddd5f1d10215

Comment 12

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 15.0a1 (2012-05-02)
Device: Samsung Galaxy Nexus
OS: Android 4.0.2