Closed Bug 856767 Opened 9 years ago Closed 9 years ago

about:home cursor is not closed on rotation or reloading

Categories

(Firefox for Android Graveyard :: General, defect)

15 Branch
x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: mfinkle, Assigned: sriram)

References

Details

Attachments

(1 file)

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
Attached patch PatchSplinter Review
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)
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().
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".
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.
Pushed with a change to post it to BG thread: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2d05f0a053
(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.
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.