about:home cursor is not closed on rotation or reloading

RESOLVED FIXED in Firefox 23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mfinkle, Assigned: sriram)

Tracking

15 Branch
Firefox 23
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

Comment 1

6 years ago
Created attachment 732062 [details] [diff] [review]
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.
Attachment #732062 - Flags: review?(mark.finkle)
(Assignee)

Comment 2

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

Updated

6 years ago
Attachment #732062 - Flags: review?(mark.finkle) → review+
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".
(Assignee)

Comment 4

6 years ago
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?
Yes, it is a problem, and yes, posting it to the background thread should work better.
(Assignee)

Comment 6

6 years ago
Pushed with a change to post it to BG thread: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2d05f0a053
(Assignee)

Comment 7

6 years ago
(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
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)
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)
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.
(Assignee)

Comment 11

6 years ago
Did a setAdapter(null) and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=43da0b155a59.
(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)
https://hg.mozilla.org/mozilla-central/rev/3bb88ea0881e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.