856767 - about:home cursor is not closed on rotation or reloading

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

I see this StrictMode violation when rotating or reloading about:home

E/StrictMode(13064): Finalizing a Cursor that has not been deactivated or closed. database = /data/data/org.mozilla.fennec_mfinkle/files/mozilla/2jpcyrig.default/browser.db, table = combined, query = SELECT _id, url, title FROM combined WHERE ((url NOT IN (SELECT url FROM bookmarks WHERE bookmarks.p
E/StrictMode(13064): android.database.sqlite.DatabaseObjectNotClosedException: Application did not close the cursor or database object that was opened here
E/StrictMode(13064): 	at android.database.sqlite.SQLiteCursor.<init>(SQLiteCursor.java:214)
E/StrictMode(13064): 	at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:53)
E/StrictMode(13064): 	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1356)
E/StrictMode(13064): 	at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:330)
E/StrictMode(13064): 	at org.mozilla.fennec_mfinkle.db.BrowserProvider.query(BrowserProvider.java:2639)
E/StrictMode(13064): 	at android.content.ContentProvider$Transport.query(ContentProvider.java:187)
E/StrictMode(13064): 	at android.content.ContentResolver.query(ContentResolver.java:262)
E/StrictMode(13064): 	at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:177)
E/StrictMode(13064): 	at org.mozilla.gecko.db.LocalBrowserDB.getTopSites(LocalBrowserDB.java:243)
E/StrictMode(13064): 	at org.mozilla.gecko.db.BrowserDB.getTopSites(BrowserDB.java:137)
E/StrictMode(13064): 	at org.mozilla.gecko.widget.TopSitesView.loadTopSites(TopSitesView.java:195)
E/StrictMode(13064): 	at org.mozilla.gecko.widget.AboutHomeContent.loadTopSites(AboutHomeContent.java:134)
E/StrictMode(13064): 	at org.mozilla.gecko.widget.AboutHomeContent.access$000(AboutHomeContent.java:34)
E/StrictMode(13064): 	at org.mozilla.gecko.widget.AboutHomeContent$2.run(AboutHomeContent.java:162)
E/StrictMode(13064): 	at android.os.Handler.handleCallback(Handler.java:587)
E/StrictMode(13064): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/StrictMode(13064): 	at android.os.Looper.loop(Looper.java:130)
E/StrictMode(13064): 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

I am guessing it is from the about:home redesign work in bug 852312

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Basically mTopSitesAdapter was with AboutHomeContent and never became null. But with the new changes, mTopSitesAdapter is moved to be with the TopSitesView. So, when removeAllViews() is called on AboutHomeContent, the old one becomes null and a new instance is created. So, the old cursor was never closed.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Note: Fragments have a destroy() method for them. So, when these individual views are made as fragments, that destroy() could be used -- performing the same set of operation. Until then, hmmm... , onDestroy().

Comment 3

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

Comment on attachment 732062 [details] [diff] [review]
Patch

I just realized that onDestroy will be called on the UI thread, leading to the same kind of bug as the one in bug 856739. We'll need to address that too.

File a followup or post a reason why closing a cursor in onDestroy is "OK".

Comment 4

[Sriram Ramasubramanian [:sriram]]

We've been closing the "oldCursor" on the UI thread since inception (Nolan, not you!). Is that a problem?
https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/widget/TopSitesView.java#l197

Should I use ThreadUtils and post it to the background thread?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Yes, it is a problem, and yes, posting it to the background thread should work better.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Pushed with a change to post it to BG thread: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2d05f0a053

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 732062 [details] [diff] [review]
> Patch
> 
> I just realized that onDestroy will be called on the UI thread, leading to
> the same kind of bug as the one in bug 856739. We'll need to address that
> too.
> 
> File a followup or post a reason why closing a cursor in onDestroy is "OK".

I don't think we can do this on the background thread. The views are getting removed on the background thread and causing a trouble. https://tbpl.mozilla.org/php/getParsedLog.php?id=21359900&tree=Mozilla-Inbound

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/97cfc16ba5dc

https://tbpl.mozilla.org/php/getParsedLog.php?id=21359900&tree=Mozilla-Inbound

57 INFO TEST-PASS | testBookmarksTab | check item can be retrieved - android.widget.RelativeLayout@484c85d8
waitForText timeout on Open in New Tab
58 INFO TEST-PASS | testBookmarksTab | Bookmark Name was changed - true should equal true
59 INFO TEST-PASS | testBookmarksTab | edit item can be retrieved - android.widget.RelativeLayout@484c85d8
waitForText timeout on Open in New Tab
60 INFO TEST-PASS | testBookmarksTab | check item can be retrieved - android.widget.RelativeLayout@48489188
waitForText timeout on Open in New Tab
61 INFO TEST-PASS | testBookmarksTab | Bookmark Link was changed - true should equal true
62 INFO TEST-PASS | testBookmarksTab | edit item can be retrieved - android.widget.RelativeLayout@48489188
waitForText timeout on Open in New Tab
63 INFO TEST-PASS | testBookmarksTab | check item can be retrieved - android.widget.RelativeLayout@484c85d8
waitForText timeout on Open in New Tab
64 INFO TEST-PASS | testBookmarksTab | Bookmark Keyword was changed - true should equal true
65 INFO TEST-PASS | testBookmarksTab | second list item can be retrieved - android.widget.RelativeLayout@484c85d8
66 INFO TEST-PASS | testBookmarksTab | second list item can be retrieved - android.widget.RelativeLayout@484c85d8
67 INFO TEST-PASS | testBookmarksTab | Removed bookmark has been deleted - false should equal false
68 INFO TEST-END | testBookmarksTab | finished in 264211ms
69 INFO TEST-START | Shutdown
70 INFO Passed: 66
71 INFO Failed: 0
72 INFO Todo: 0
73 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:04:58.455080
INFO | zombiecheck | Reading PID log: /tmp/tmpUvpnK6pidlog
PROCESS-CRASH | java-exception | android.view.ViewRoot$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.	at android.view.ViewRoot.checkThread(ViewRoot.java:2802)

Comment 9

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

Full exception:

04-02 13:43:03.073 E/GeckoAppShell( 2322): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 10 ("GeckoBackgroundThread")
04-02 13:43:03.073 E/GeckoAppShell( 2322): android.view.ViewRoot$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.ViewRoot.checkThread(ViewRoot.java:2802)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.ViewRoot.requestLayout(ViewRoot.java:594)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:254)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:254)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:254)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.ScrollView.requestLayout(ScrollView.java:1200)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:254)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.view.View.requestLayout(View.java:8125)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.AbsListView.requestLayout(AbsListView.java:993)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.AdapterView$AdapterDataSetObserver.onInvalidated(AdapterView.java:814)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.DataSetObservable.notifyInvalidated(DataSetObservable.java:43)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.BaseAdapter.notifyDataSetInvalidated(BaseAdapter.java:54)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.widget.CursorAdapter$MyDataSetObserver.onInvalidated(CursorAdapter.java:391)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.DataSetObservable.notifyInvalidated(DataSetObservable.java:43)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.AbstractCursor.deactivateInternal(AbstractCursor.java:89)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.AbstractCursor.close(AbstractCursor.java:108)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.sqlite.SQLiteCursor.close(SQLiteCursor.java:500)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.CursorWrapper.close(CursorWrapper.java:45)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.content.ContentResolver$CursorWrapperInner.close(ContentResolver.java:1355)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.CursorWrapper.close(CursorWrapper.java:45)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.database.CursorWrapper.close(CursorWrapper.java:45)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at org.mozilla.gecko.widget.TopSitesView$3.run(TopSitesView.java:150)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.os.Handler.handleCallback(Handler.java:587)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.os.Handler.dispatchMessage(Handler.java:92)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at android.os.Looper.loop(Looper.java:123)
04-02 13:43:03.073 E/GeckoAppShell( 2322): 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I think we just need to disconnect the view from the cursor before closing the cursor. This should be doable with either setAdapter(null), or if that doesn't work, do "Cursor cursor = mTopSitesAdapter.swapCursor(null);" and then close the cursor on the background thread.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Did a setAdapter(null) and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=43da0b155a59.

Comment 12

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> Did a setAdapter(null) and pushed to try:
> https://tbpl.mozilla.org/?tree=Try&rev=43da0b155a59.

I see some green there. (after some infra quicks)

Comment 13

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb88ea0881e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bb88ea0881e