Last Comment Bug 736296 - StrictMode violation: DatabaseObjectNotClosedException from AboutHomeContent.loadTopSites()
: StrictMode violation: DatabaseObjectNotClosedException from AboutHomeContent....
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 14
Assigned To: Sriram Ramasubramanian [:sriram]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-15 15:51 PDT by Chris Peterson [:cpeterson]
Modified: 2012-05-02 06:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
beta+


Attachments
Patch: Option 1 (2.72 KB, patch)
2012-03-16 14:49 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Review
Patch: Option 2 (3.14 KB, patch)
2012-03-16 14:53 PDT, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Review

Description Chris Peterson [:cpeterson] 2012-03-15 15:51:54 PDT
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] 2012-03-16 13:36:59 PDT
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] 2012-03-16 14:49:17 PDT
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.
Comment 3 Sriram Ramasubramanian [:sriram] 2012-03-16 14:53:29 PDT
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.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-17 08:26:14 PDT
Is there any benefit to Option 2 over Option 1?
Comment 5 Sriram Ramasubramanian [:sriram] 2012-03-18 14:39:32 PDT
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?) 2012-03-26 20:32:35 PDT
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?) 2012-03-26 20:33:38 PDT
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?) 2012-03-26 20:34:23 PDT
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] 2012-03-27 14:32:11 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/71416b7efdbd
Comment 10 Ed Morley [:emorley] 2012-03-28 14:03:19 PDT
https://hg.mozilla.org/mozilla-central/rev/71416b7efdbd
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-04 10:15:02 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/ddd5f1d10215
Comment 12 Cristian Nicolae (:xti) 2012-05-02 06:36:02 PDT
Verified fixed on:

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

Note You need to log in before you can comment on or make changes to this bug.