Closed Bug 760956 Opened 11 years ago Closed 7 years ago

Reimplement top sites database access to use a single cursor

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

Product:
Firefox for Android Graveyard
Component:
Awesomescreen
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(firefox16 affected, firefox17 affected, firefox22 affected, firefox23 affected, firefox30 affected, firefox44 affected, firefox46+ wontfix, firefox47+ fixed, fennec44+)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox22 --- affected
firefox23 --- affected
firefox30 --- affected
firefox44 --- affected
firefox46 + wontfix
firefox47 + fixed
fennec 44+ ---

People

(Reporter: scoobidiver, Assigned: ahunt)

References

Details

(Keywords: crash, Whiteboard: [native-crash][experience required])

Crash Data

Attachments

(12 files, 8 obsolete files)

logcat
68.96 KB, text/plain
Details
logs_url_crash
100.21 KB, text/plain
Details
Single cursor topsites generation preview patch
20.11 KB, patch
rnewman
: feedback+
Details | Diff | Splinter Review
MozReview Request: Bug 760956 - Part 1: Introduce variadic concatenateSelectionArgs r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Part 2: Introduce numbers table r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Part 3: Add getSuggestedSites to BrowserDB r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Part 4: Implement single-cursor topsite-retrieval in BrowserProvider r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Part 5: use BrowserProvider TopSites query instead of TopSitesCursorWrapper r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Part 6: Introduce basic topsites retrieval test r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Post: Cleanup TopSitesCursorWrapper remainders r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Post: remove unneeded getPinnedSites and getTopSites from BrowserDB r=me
58 bytes, text/x-review-board-request
rnewman
: review+
Details
MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman
58 bytes, text/x-review-board-request
rnewman
: review+
Details 
There are two crashes in 14.0b5, including bp-b5eb69ea-0b54-4371-9eb3-bcc852120602.

java.lang.IllegalStateException: attempt to re-open an already-closed object: android.database.sqlite.SQLiteQuery (mSql = SELECT _id, url, title, thumbnail FROM combined_with_images WHERE (((url LIKE ? OR title LIKE ?)) AND ((url NOT LIKE ?))) ORDER BY visits * MAX(1, 100 * 225 / ((date - 1338650633934) / 86400000*(date - 1338650633934) / 86400000 + 225)) DESC LIMIT 4) 
	at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java:34)
	at android.database.sqlite.SQLiteQuery.fillWindow(SQLiteQuery.java:67)
	at android.database.sqlite.SQLiteCursor.fillWindow(SQLiteCursor.java:297)
	at android.database.sqlite.SQLiteCursor.onMove(SQLiteCursor.java:261)
	at android.database.AbstractCursor.moveToPosition(AbstractCursor.java:188)
	at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:187)
	at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:187)
	at android.widget.CursorAdapter.getItemId(CursorAdapter.java:155)
	at android.widget.AdapterView.rememberSyncState(AdapterView.java:1129)
	at android.widget.AdapterView$AdapterDataSetObserver.onChanged(AdapterView.java:787)
	at android.database.DataSetObservable.notifyChanged(DataSetObservable.java:31)
	at android.widget.BaseAdapter.notifyDataSetChanged(BaseAdapter.java:50)
	at android.widget.CursorAdapter.changeCursor(CursorAdapter.java:260)
	at android.widget.SimpleCursorAdapter.changeCursor(SimpleCursorAdapter.java:321)
	at org.mozilla.gecko.AboutHomeContent$8.run(AboutHomeContent.java:354)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3687)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:842)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:600)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalStateException%3A+at+android.database.sqlite.SQLiteClosable.acquireReference%28SQLiteClosable.java%29
status-firefox16: --- → affected
status-firefox17: --- → affected
Version: Firefox 14 → Firefox 16
(In reply to Teodora Vermesan (:TeoVermesan) from comment #1)
> On Samsung Galaxy nexus, after installing the Nightly builds from 
> http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-fig-android/ 
> the app crashes with this crash signature.
> https://crash-stats.mozilla.com/report/index/358ef14a-8bc4-426c-aa9f-
> 13b712130628

i hit this too on the same fig build.  My STR was just pinching and zooming around 4 open tabs on nba.com, spurs.com

https://crash-stats.mozilla.com/report/index/f8bb63a7-0954-4450-a8cf-7a9ad2130628
Attached file logcat — 
logcat attached.

more STR, 
1) launched boston celtics website  (www.nba.com/celtics)
2) open new tab, and swiped the celtics site to close
3) flicked the content window back up to hide menu
4) Crash.
status-firefox22: --- → affected
status-firefox23: --- → affected
Version: Firefox 16 → Firefox 22
Still an issue on 30.

java.lang.IllegalStateException: attempt to re-open an already-closed object: SQLiteQuery: SELECT _id, url, title, MAX(display) AS display, bookmark_id, history_id FROM combined WHERE ((url NOT IN (SELECT url FROM bookmarks WHERE bookmarks.parent < ? AND bookmarks.deleted == 0)) AND ((url NOT LIKE ?))) GROUP BY url ORDER BY (CASE WHEN bookmark_id > -1 THEN 100 ELSE 0 END) + visits * MAX(1, 100 * 225 / ((date - 1393108635975) / 86400000*(date - 1393108635975) / 86400000 + 225))  DESC LIMIT 30
	at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java:55)
	at android.database.sqlite.SQLiteQuery.fillWindow(SQLiteQuery.java:58)
	at android.database.sqlite.SQLiteCursor.fillWindow(SQLiteCursor.java:166)
	at android.database.sqlite.SQLiteCursor.onMove(SQLiteCursor.java:139)
	at android.database.AbstractCursor.moveToPosition(AbstractCursor.java:214)
	at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:162)
	at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:162)
	at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:162)
	at org.mozilla.gecko.db.BrowserDB$TopSitesCursorWrapper.moveToPosition(BrowserDB.java:459)
	at android.support.v4.widget.CursorAdapter.getView(CursorAdapter.java:247)
	at android.widget.AbsListView.obtainView(AbsListView.java:2186)
	at android.widget.GridView.makeAndAddView(GridView.java:1341)
	at android.widget.GridView.makeRow(GridView.java:341)
	at android.widget.GridView.fillSpecific(GridView.java:543)
	at android.widget.GridView.layoutChildren(GridView.java:1240)
	at android.widget.AbsListView.onTouchModeChanged(AbsListView.java:3318)
	at
status-firefox30: --- → affected
See Also: → 1014603
Still not fixed, i get this crash 1-2 per day.

be0ee02f-b0c8-41c1-8f46-fe2af2150930
status-firefox44: --- → affected
Whiteboard: [native-crash] → [native-crash][experience required]
20,000 crashes with this signature in the last week.
tracking-fennec: --- → ?
Component: General → Awesomescreen
Crash Signature: [@ java.lang.IllegalStateException: at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java)] → [@ java.lang.IllegalStateException: at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java)] [@ java.lang.IllegalStateException: at android.database.sqlite.SQLiteClosable.acquireReference]
Ioana, can you help us get more information about affected devices or STR?
tracking-fennec: ? → 44+
Flags: needinfo?(mark.finkle)
Flags: needinfo?(ioana.chiorean)
Mike, this crash started spiking in 41. Maybe you can look into fixing this as part of the top sites work you're doing.
Flags: needinfo?(michael.l.comella)
Attached file logs_url_crash — 
Able to reproduce this on Samsung Galaxy S5 (Android 4.4.2) (samsung SM-G900F 19 (REL)) on Firefox 41.0.2
The crash is intermittent, but in order to reproduce this you have to perform some steps, not in a particular order:
-Tap on URL bar to open edit mode
-Edit the text in the URL bar then press back button to close edit mode
-Repeat this steps, then eventually Firefox will crash

https://crash-stats.mozilla.com/report/index/79f3f215-f2b4-45a8-aa09-cb5eb2151021
Flags: needinfo?(ioana.chiorean)
Mike, this is the #3 crash on beta and release, we should look into this.

I wonder if it's a similar OOM problem to what we were seeing in bug 760394.
Assignee: nobody → michael.l.comella
Richard previously added a see also to bug 1014603, in which I had previously done some analysis. In particular:
 * This method is called *twice* in some circumstances.

Is suspicious in TopSitesPanel.CursorLoaderCallbacks$onLoadFinishedAfterTransitions.
To state what I know:

The cursor which is closed is TopSitesCursorWrapper.topCursor [1]. This cursor is closed in TopSitesCursorWrapper.close [2] (also deactivated in TopSitesCursorWrapper.deactive but I don't think this error would occur in that case). TopSitesCursorWrapper is only created in `TopSitesLoader extends SimpleCursorLoader` [3]. TopSitesCursorWrapper is only closed in [4]:
  * SimpleCursorLoader.deliverResult – close a Cursor from a reset Loader or the old, unclosed Cursor
  * SimpleCursorLoader.onCanceled – close the argument Cursor if it hasn't already been closed
  * SimpleCursorLoader.onReset – close the member variable Cursor if 

Our bug, presumably, is in our Loader implementation (SimpleCursorLoader) or how we call access the Loaders (the stack trace comes from a CursorAdapter – you'd think this would just work). SimpleCursorLoader is originally from the Android framework and then modified by us – I wonder if it's changed?

Why do we use our own implementation anyway? Performance? Did we copy this before we had the support library available? Seems like a good place to start investigating.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java?rev=662e4aff03d4#649
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/TopSitesCursorWrapper.java?rev=346ddee185b9#576
[3]: https://mxr.mozilla.org/mozilla-central/search?string=new+topsitescursorwrapper&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[4]: https://mxr.mozilla.org/mozilla-central/search?string=close%28%29&find=mobile%2Fandroid%2Fbase%2Fhome%2FSimple&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
>   * SimpleCursorLoader.onReset – close the member variable Cursor if 

...if it is not already closed.
(In reply to Michael Comella (:mcomella) from comment #13)
> Why do we use our own implementation [of CursorLoader] anyway? Performance? Did we copy this
> before we had the support library available? Seems like a good place to
> start investigating.

It looks like CursorLoader talks directly to the ContentProvider whereas our SimpleCursorLoader takes a Cursor directly. Since TopSitesCursorWrapper wraps three Cursors, rather than running 3 Loaders and combining the output into TopSitesCursorWrapper, it also seems to make more sense to run one Loader on the combined output. However, since we do it this way, we can't just use the framework.

He mentioned there was a lot of legacy code before switching to Loaders (so perhaps this could be done more simply another way).

bnicholson, the reviewer for the original patch in bug 877870, confirmed that this hypothesis sounds right.
It looks like the CursorLoader hasn't changed since the previous iterations. However, we don't have an implementation of the cancelLoadInBackground [1] method which cancels an in progress ContentProvider query. There is some synchronization [2][3][4] that occurs around the internal variables it uses so we can infer cancelLoadInBackground will be called concurrently with the query. When that occurs, according to the docs [5], the query will throw OperationCanceledException which goes up the call stack:
 CursorLoader.loadInBackground
  AsyncTaskLoader.onLoadInBackground
   AsyncTaskLoader.doInBackground

which catches OperationCanceledException [6] and returns null rather than the returned Cursor.

I imagine the Adapter would see this null and either keep the old data or blank the data.

I have yet to analyze our case, but I imagine we return the Cursor anyway, which could get cancelled in the onReset method, but since it's not null, the Adapter tries to access it. 

[1]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java#81
[2]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java#84
[3]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java#52
[4]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java#74
[5]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java#74
[6]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/AsyncTaskLoader.java#57
fwiw, if we wanted to blindly copy this cancelLoadInBackground behavior, we could implement the method and pass a CancellationSignal into through `SimpleCursorLoader.loadCursor` [1][2] and pass it to the queries for each of the Cursors we wrap.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SimpleCursorLoader.java?rev=68f3b7ceb34f#44
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TopSitesPanel.java?rev=17bb5573ac93#536
In retrospect, if cancelling the ContentResolver.query didn't matter, then why would the framework CursorLoader do it? Since we also make ContentResolver queries, we also need to do this behavior.
Due to comment 18, rather than go with the perfect solution of researching further and definitely identifying this as the issue, I'm going to with the good solution of fixing this issue and seeing if this issue persists.
Bug 760956 - Add cancellation functionality to SimpleCursorLoader. r=margaret

This does not compile yet - the inherited abstract methods have not been
changed.
Bug 760956 - Add CancellationSignal to TopSitesLoader. r=margaret

TODO: Not sure what to do with suggestedSites' MatrixCursor - presumably we
need to null that.
Here is my WIP patch series, which does not yet compile (I still need to work on one more Loader & the two instances where the Loader supports MatrixCursor. My two concerns:
  1) Can a CancellationSignal be re-used? (I'll have a look at the source)
  2) The original loader cancels the query, but then jumps to some guarded code whereas we run the query, then do some stuff, then jump to that guarded code. Does that matter? What happens if the Cursor is cancelled while we "do some stuff"? Should we check if it's cancelled before returning? I thought about adding a clause at [0] to close the Cursor if the query was cancelled but then I'd expect the framework to already have done that. Notably, if the query is cancelled and the framework is already in guarded code, afaict the Cursor is returned meaning the cancel has no effect (unless it is handled somehow in the thread calling cancel). I need to investigate further.

Ideally, we'd also move the Loaders that can easily use the framework's CursorLoader to that class, however, this will break some of our tests. We could try to write a Loader which loads synchronously but there is also a LoaderTestCase [1] (but maybe we'll just borrow from the source [2] ;). However, this will add to the amount of time it takes to finalize this patch and the complexity of it.

[0]: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java#74
[1]: http://developer.android.com/reference/android/test/LoaderTestCase.html
[2]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/test-runner/src/android/test/LoaderTestCase.java#55
Margaret, Mike: I came upon this bug while looking at top crashers on FennecAndroid 44.0b2. This one is linked to top crash ranked #7. Do you think we could get the patches reviewed and landed in m-c soon? I would be happy to take a fix for this in Beta44 as well. Thanks!
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
(In reply to Ritu Kothari (:ritu) from comment #29)
> Margaret, Mike: I came upon this bug while looking at top crashers on
> FennecAndroid 44.0b2. This one is linked to top crash ranked #7. Do you
> think we could get the patches reviewed and landed in m-c soon? I would be
> happy to take a fix for this in Beta44 as well. Thanks!

I'm currently slated to try to land our A/B testing telemetry for 44 (attempting with bug 1232777 first, but potentially writing a larger telemetry system – bug 1205835) so I don't think I'll have time to finish this up for 44, but I'll take a look after the A/B testing bugs. It doesn't require my expertise though – someone else could probably take this over.

That being said, it's not an easy patch series to get right – I'm not sure I'd be comfortable uplifting to beta, though I can see the big gains here.
Flags: needinfo?(michael.l.comella)
Maybe Nick has time to help take a look.

Given that this is risky for uplift, it would be better to land a fix on m-c sooner rather than later, since it will need lead time to bake on the trains.
Flags: needinfo?(mark.finkle)
Flags: needinfo?(margaret.leibovic)
(Oops, failed to set NI)
Flags: needinfo?(nalexander)
I started to dig in again and made a realization: most of the stack traces revolve around TopSitesCursorWrapper. We can get a much simpler patch to uplift if we only focus on this cursor, as opposed to all of them (though if my hypothesis in comment 19 is correct, we should fix all of them). Perhaps the race condition occurs most frequently with TopSitesCursorWrapper because TopSites is the default home panel.

The reason I'm proposing this (rather than fixing all of them at once) is that I started to fix some cursors the "correct" way (by extending the framework's CursorLoader) and, as I should have suspected, it's taking longer than I'd like. The "hack" way carries a lot of complexity (see my current WIP patches on this bug) and has some unanswered questions (see comment 28).
I filed bug 1239491 to deal with the cursors that are not TopSitesCursorLoader.
Important observation: the built-in CursorLoader implementation (API 11+) [1] uses CancellationSignals while the CursorLoader support library implementation [2] does not (and if the support library implementation is used, the non-CancellationSignal version will be used *on all devices*).

Meaning perhaps the CancellationSignal is not an essential part of the implementation and my approach in comment 17 is wrong. That or it's a bug in the Android framework that was later fixed but forgotten in the support library and on GB (perhaps because CancellationSignal is 16+, or earlier with the support library).

Since I'm already so close, I might as well finish this and try it out though.

[1]: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/content/CursorLoader.java
[2]: http://androidxref.com/5.1.0_r1/xref/frameworks/support/v4/java/android/support/v4/content/CursorLoader.java
(In reply to Michael Comella (:mcomella) from comment #18)
> In retrospect, if cancelling the ContentResolver.query didn't matter, then
> why would the framework CursorLoader do it? Since we also make
> ContentResolver queries, we also need to do this behavior.

This could be an optimization to cancel background queries faster.
re comment 35, further proof that it's an optimization: if cancelling were an essential part of async loading, we'd want to throw `OperationCanceledException` if the signal is cancelled at any point during the `Loader.loadInBackground` call, however, we only throw during the actual DB query. [1]

An implementation where cancelling is essential would `synchronized(Loader)` and call `CancellationSignal.throwIfCanceled` before setting the reference to null [2].

Thoughts on where this bug could be coming from:
 * CursorLoader may be written to assume the query happens in one call (rather than our code where we do a lot of work before and after making the query, particularly when we combine cursors like in TopSitesCursorWrapper)
 * We're using the TopSitesLoader itself incorrectly
 * There is a bug in the Android framework

[1]: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/content/CursorLoader.java#64
[2]: http://androidxref.com/5.1.0_r1/xref/frameworks/base/core/java/android/content/CursorLoader.java#79
Comment on attachment 8690308 [details]
MozReview Request: Bug 760956 - Add SimpleCancellableCursorLoader; add warning to SimpleCursorLoader. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25835/diff/1-2/
Attachment #8690308 - Attachment description: MozReview Request: Bug 760956 - Add cancellation functionality to SimpleCursorLoader. r=margaret → MozReview Request: Bug 760956 - Add SimpleCancellableCursorLoader; add warning to SimpleCursorLoader. r=margaret
Attachment #8690308 - Flags: review?(margaret.leibovic)
Attachment #8690309 - Attachment description: MozReview Request: Bug 760956 - Add CancellationSignal to TopSitesLoader. r=margaret → MozReview Request: Bug 760956 - Move TopSitesLoader to SimpleCancellableCursorLoader. r=margaret
Attachment #8690309 - Flags: review?(margaret.leibovic)
Comment on attachment 8690309 [details]
MozReview Request: Bug 760956 - Move TopSitesLoader to SimpleCancellableCursorLoader. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25837/diff/1-2/
A better explanation of what I ended up doing in the commit comment on RB, pasted here for completeness:

> This bug was originally suspected to be caused by the lack of cancellation in
> SimpleCursorLoader but as comments 35-37 discovered, the Android framework has
> an implementation with canceling and without. As such, this bug is not
> necessarily caused by the lack of canceling. However, solution was pretty close
> to being complete so I figured we should land it and see if it helps the crash
> at all.

Since I'm not certain these changes will fix the issue, my proposed solution: land these two patches, see if the crash rate drops. If not, back this out to reduce the complexity of the code.

As this is the #7 top crasher, I feel we should pipeline and get more eye-time on this as we wait for the results of my patches. It'd probably be a good idea to get some fresh eyes on this (particularly someone who has more experience working with Loaders) and in any case, I expect to be busy with Telemetry. NI Margaret to raise awareness.
Flags: needinfo?(margaret.leibovic)
Attachment #8690308 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8690308 [details]
MozReview Request: Bug 760956 - Add SimpleCancellableCursorLoader; add warning to SimpleCursorLoader. r=margaret

https://reviewboard.mozilla.org/r/25835/#review27707
Attachment #8690309 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8690309 [details]
MozReview Request: Bug 760956 - Move TopSitesLoader to SimpleCancellableCursorLoader. r=margaret

https://reviewboard.mozilla.org/r/25837/#review27711
(In reply to Michael Comella (:mcomella) from comment #40)
> A better explanation of what I ended up doing in the commit comment on RB,
> pasted here for completeness:
> 
> > This bug was originally suspected to be caused by the lack of cancellation in
> > SimpleCursorLoader but as comments 35-37 discovered, the Android framework has
> > an implementation with canceling and without. As such, this bug is not
> > necessarily caused by the lack of canceling. However, solution was pretty close
> > to being complete so I figured we should land it and see if it helps the crash
> > at all.
> 
> Since I'm not certain these changes will fix the issue, my proposed
> solution: land these two patches, see if the crash rate drops. If not, back
> this out to reduce the complexity of the code.

Okay, let's land these patches and see what happens.

> As this is the #7 top crasher, I feel we should pipeline and get more
> eye-time on this as we wait for the results of my patches. It'd probably be
> a good idea to get some fresh eyes on this (particularly someone who has
> more experience working with Loaders) and in any case, I expect to be busy
> with Telemetry. NI Margaret to raise awareness.

Improving our crash rate is a top priority. Can you make a post to mobile-firefox-dev with the status of your investigation and ask for help looking into this?
Flags: needinfo?(margaret.leibovic) → needinfo?(michael.l.comella)
https://hg.mozilla.org/integration/fx-team/rev/dca605087871115e71dcdd54c8591ffe4ddbb052
Bug 760956 - Add SimpleCancellableCursorLoader; add warning to SimpleCursorLoader. r=margaret

https://hg.mozilla.org/integration/fx-team/rev/a8a02f40302c851ad5d5fd5ada3ddbdf050e608d
Bug 760956 - Move TopSitesLoader to SimpleCancellableCursorLoader. r=margaret
https://hg.mozilla.org/mozilla-central/rev/dca605087871
https://hg.mozilla.org/mozilla-central/rev/a8a02f40302c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(In reply to :Margaret Leibovic from comment #43)
> Improving our crash rate is a top priority. Can you make a post to
> mobile-firefox-dev with the status of your investigation and ask for help
> looking into this?

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-January/001687.html
Flags: needinfo?(michael.l.comella)
(In reply to Carsten Book [:Tomcat] from comment #45)
> https://hg.mozilla.org/mozilla-central/rev/dca605087871
> https://hg.mozilla.org/mozilla-central/rev/a8a02f40302c

This crash still happens on Nightly builds that include these changes, so I don't think that these help.

Here's the most recent report:
https://crash-stats.mozilla.com/report/index/9c4196d6-97d8-42ac-a19d-662bc2160117

I don't think we should uplift these patches if they don't do anything to improve the crash situation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to :Margaret Leibovic from comment #47)
> This crash still happens on Nightly builds that include these changes, so I
> don't think that these help.

As per comment 40, then I think we should back these out.
I'm no longer actively working in this to focus on telemetry.
Assignee: michael.l.comella → nobody
ahunt, can you pick this up? We need someone to make progress on this. There were implementation suggestions in the thread on mobile-firefox-dev.
Assignee: nobody → ahunt
Just a small update: I'm very close to having a patch for this - building the top+pinned sites list in SQL is complete, I just need to finish off the elimination of duplicate sites from the suggested list.
Here's a preview patch - it would be nice to check that I'm going down the right path with this.

I still need to:
- actually take into account the min/max limits for the topsites list (we currently show suggested sites in the list below the grid)
- probably tidy up the SQL statement generation
- tidy up in general?

(There will also be some followup patches removing the other getTopSites, and getPinnedSites from LocalBrowserDB, and removing TopSitesCursorWrapper.)
Attachment #8690308 - Attachment is obsolete: true
Attachment #8690309 - Attachment is obsolete: true
Attachment #8713415 - Flags: feedback?(rnewman)
Comment on attachment 8713415 [details] [diff] [review]
Single cursor topsites generation preview patch

Review of attachment 8713415 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java
@@ +173,5 @@
>      public abstract void unpinSite(ContentResolver cr, int position);
>  
>      public abstract boolean hideSuggestedSite(String url);
>      public abstract void setSuggestedSites(SuggestedSites suggestedSites);
> +    public abstract SuggestedSites getSuggestedSites();

For future reference: this is the kind of boundary at which it's nice to split patches. So we'd have something like:

  Bug 760956 - Part 1: add getSuggestedSites method to BrowserDB.
  Bug 760956 - Part 2: implement a dedicated getTopSites call in BrowserProvider.
  Bug 760956 - Part 3: add URI/ContentProvider interface to getTopSites.
  Bug 760956 - Part 4: use new top sites accessor in LocalBrowserDB.
  Bug 760956 - Part 5: clean up now-unused database code.

These commits would:

* Tell a story about how the implementation works.
* Be self-contained -- you'd be able to see what exactly is involved in exposing a CP's method through a URI.
* Shine a light on testing in each part. Can we add a trivial test for getSuggestedSites that makes sure we have the default set when accessed through the profile? Can we test BrowserProvider.getTopSites in the provider tests?

You don't have to go back and split it up now, but I figured this was a good illustration for the feedback!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ +675,5 @@
> +        final String[] pinnedSitesArgs = new String[] {
> +                String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID)
> +        };
> +
> +        db.beginTransaction();

Follow the pattern:

  db.beginTransaction();
  try {
      …
      db.setTransactionSuccessful();
  } finally {
      db.endTransaction();
  }

But! Note that we might already be in a transaction, so this becomes more complicated.

However, as I note below this can all be done in a single query, so no need to worry about transactions at all.

@@ +680,5 @@
> +
> +        // We cannot make this a real temporary table: temporary tables are dropped as soon as we endTransaction(),
> +        // however we're using this table in our resulting cursor, so we need to make it a normal table.
> +        db.execSQL("DROP TABLE IF EXISTS " + TABLE_IDS);
> +        db.execSQL("CREATE TABLE " + TABLE_IDS + " AS " +

Can we do this without a table altogether? That is: if you're creating a table here with a series, can we just use the same subexpression as a subquery?

Indeed, one could imagine that we do something like…

@@ +688,5 @@
> +                        " UNION ALL" +
> +                        " SELECT x+1 FROM series" +
> +                        " LIMIT ?" +
> +                        " )" +
> +                        " SELECT x AS " + Bookmarks.POSITION + " FROM series " +

Replace '10' here with the number of top sites.

Replace "SELECT position FROM foo" with selecting the indices for pinned sites.

sqlite> WITH RECURSIVE
   ...>   cnt(x) AS (VALUES(1) UNION ALL SELECT x+1 FROM cnt WHERE x<10)
   ...> SELECT x FROM cnt WHERE x NOT IN (SELECT position FROM foo)
   ...> ORDER BY x ASC;
x
----------
1
2
4
6
7
8
9
10
sqlite> SELECT position FROM foo;
position
----------
3
5


Now you can just join and union these. No temporary table needed.

@@ +696,5 @@
> +
> +        // Filter out: unvisited pages (history_id == -1) pinned (and other special) sites, deleted sites,
> +        // and about: pages.
> +        final String ignoreForTopSitesWhereClause =
> +                "(" + Combined.HISTORY_ID + " <> -1)" +

I prefer `IS NOT` for readability (and null safety).

@@ +735,5 @@
> +        StringBuilder suggestedSitesBuilder = new StringBuilder();
> +        // TODO: we should access the underlying data directly here
> +        // TODO 2: we should get the total number of pinned+top sites here, subtract that from limit, and fetch
> +        // that many suggested sites - that should ensure that we don't show suggested sites outside of the grid area.
> +        Cursor suggestedSitesCursor = suggestedSites.get(Integer.parseInt(limit));

You might consider changing the API here.

SuggestedSites builds and returns a MatrixCursor for the convenience of the code that we're deleting!

Much easier if we just have it return its internal structure, which is a Map<String, Site>, or spit out List<Site> instead.

@@ +745,5 @@
> +            if (suggestedSitesBuilder.length() > 0) {
> +                suggestedSitesBuilder.append(" UNION ALL");
> +            }
> +            suggestedSitesBuilder.append(" SELECT" +
> +                    " ? AS " + Bookmarks._ID + "," +

Pulling out the chunk that turns a list of sites into a complete SQL subquery would be clearer:

  private String getSuggestedSitesSQLForSites(List<Site> sites) {
      final StringBuilder builder = new StringBuilder();
      …
  }
Attachment #8713415 - Flags: feedback?(rnewman) → feedback+
I've just discovered that CTE's / the WITH clause aren't usable on Android <= 4.4 - which means our current approach to generating the sequence of topsites-ids isn't generally usable. (I've gotten a version without the temporary-id-table running locally, but that still requires the "WITH RECURSIVE" sequence generation in a subquery.)

SQLite only supports CTEs and WITH since 3.8.3:
https://sqlite.org/releaselog/3_8_3.html

And we only use SQLite > 3.8.3 on Android 5.0 or above:
http://stackoverflow.com/a/4377116
(3.8.4.3 since Android 5.0, 3.7.11 on 4.1 through 4.4, even older on older Android versions)

I'm not too sure what the best alternatives are for id generation - would it be a bad idea to manually generate something like "VALUES(1) UNION ALL VALUES(2)..." (possibly storing that in a temp table)?
Flags: needinfo?(nalexander) → needinfo?(rnewman)
As mentioned on IRC, let's go for the trivial, sad, and ultimately adequate approach of having a single permanent table of the integers from 0 to 50.

Alternatively, construct a single MatrixCursor in memory by walking a union of top sites and pinned sites (just what you have now, but without the merging and internal positioning), immediately closing the source cursor. That might also relieve pressure from CursorWindow allocations.
Flags: needinfo?(rnewman)
Ironically, I've just run into what seems to be the same crash while working on the single-query attempt to fix this bug.

As far as I can tell it happened when I opened a new tab, saw the old version of the top-sites page, and then pressed on one of the tiles while the tiles seemed to be reloading - which I think happens when our pageviews get updated (which triggers querying the same cursor again). (I was just opening new tabs, and each time opening a random page from the top/suggested sites grid.) It's quite intermittent.

02-12 16:42:00.319  14459-14459/org.mozilla.fennec_andrzejhunt E/InputEventReceiver﹕ Exception dispatching input event.
02-12 16:42:00.319  14459-14459/org.mozilla.fennec_andrzejhunt E/MessageQueue-JNI﹕ Exception in MessageQueue callback: handleReceiveCallback
02-12 16:42:00.322  14459-14459/org.mozilla.fennec_andrzejhunt E/MessageQueue-JNI﹕ java.lang.IllegalStateException: attempt to re-open an already-closed object: SQLiteQuery: SELECT _id, url, position AS title, position, history_id, 1 AS type FROM topsites LEFT OUTER JOIN ( SELECT count(free_ids.position) AS rowid, numbers.position AS position FROM numbers AS numbers LEFT OUTER JOIN  (SELECT position FROM numbers WHERE position NOT IN (SELECT position FROM bookmarks WHERE parent == ?)) AS free_ids ON numbers.position > free_ids.position WHERE numbers.position NOT IN (SELECT position FROM bookmarks WHERE parent == ?) GROUP BY numbers.position ORDER BY numbers.position ASC) AS id_results ON topsites.rowid = id_results.rowid UNION ALL SELECT _id, url, title, position, NULL AS history_id, 2 as type FROM bookmarks WHERE parent == ?  UNION ALL SELECT * FROM (SELECT _id, url, title, position, NULL AS history_id, 1 as type FROM (  SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position ) WHERE url NOT IN (SELECT url FROM topsites) AND url NOT IN (SELECT url FROM bookmarks WHERE parent == ?) LIMIT MAX(0, (6 - (SELECT COUNT(*) FROM topsites) - (SELECT COUNT(*) FROM bookmarks WHERE parent== -3)))  ) ORDER BY position
            at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java:55)
            at android.database.sqlite.SQLiteQuery.fillWindow(SQLiteQuery.java:58)
            at android.database.sqlite.SQLiteCursor.fillWindow(SQLiteCursor.java:152)
            at android.database.sqlite.SQLiteCursor.onMove(SQLiteCursor.java:124)
            at android.database.AbstractCursor.moveToPosition(AbstractCursor.java:214)
            at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:162)
            at android.support.v4.widget.CursorAdapter.getItemId(CursorAdapter.java:225)
            at android.widget.AdapterView.getItemIdAtPosition(AdapterView.java:778)
            at android.widget.AdapterView.setSelectedPositionInt(AdapterView.java:1185)
            at android.widget.AbsListView.onTouchUp(AbsListView.java:3837)
            at android.widget.AbsListView.onTouchEvent(AbsListView.java:3637)
            at android.view.View.dispatchTouchEvent(View.java:8471)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2399)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2092)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at org.mozilla.gecko.home.HomePager.dispatchTouchEvent(HomePager.java:342)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchTouchEvent(PhoneWindow.java:2369)
            at com.android.internal.policy.impl.PhoneWindow.superDispatchTouchEvent(PhoneWindow.java:1719)
            at android.app.Activity.dispatchTouchEvent(Activity.java:2742)
            at co
02-12 16:42:00.322  14459-14459/org.mozilla.fennec_andrzejhunt D/AndroidRuntime﹕ Shutting down VM
02-12 16:42:00.324  14459-14459/org.mozilla.fennec_andrzejhunt E/GeckoCrashHandler﹕ >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
    java.lang.IllegalStateException: attempt to re-open an already-closed object: SQLiteQuery: SELECT _id, url, position AS title, position, history_id, 1 AS type FROM topsites LEFT OUTER JOIN ( SELECT count(free_ids.position) AS rowid, numbers.position AS position FROM numbers AS numbers LEFT OUTER JOIN  (SELECT position FROM numbers WHERE position NOT IN (SELECT position FROM bookmarks WHERE parent == ?)) AS free_ids ON numbers.position > free_ids.position WHERE numbers.position NOT IN (SELECT position FROM bookmarks WHERE parent == ?) GROUP BY numbers.position ORDER BY numbers.position ASC) AS id_results ON topsites.rowid = id_results.rowid UNION ALL SELECT _id, url, title, position, NULL AS history_id, 2 as type FROM bookmarks WHERE parent == ?  UNION ALL SELECT * FROM (SELECT _id, url, title, position, NULL AS history_id, 1 as type FROM (  SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position UNION ALL SELECT ? AS _id, ? AS url, ? AS title, ? AS position ) WHERE url NOT IN (SELECT url FROM topsites) AND url NOT IN (SELECT url FROM bookmarks WHERE parent == ?) LIMIT MAX(0, (6 - (SELECT COUNT(*) FROM topsites) - (SELECT COUNT(*) FROM bookmarks WHERE parent== -3)))  ) ORDER BY position
            at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java:55)
            at android.database.sqlite.SQLiteQuery.fillWindow(SQLiteQuery.java:58)
            at android.database.sqlite.SQLiteCursor.fillWindow(SQLiteCursor.java:152)
            at android.database.sqlite.SQLiteCursor.onMove(SQLiteCursor.java:124)
            at android.database.AbstractCursor.moveToPosition(AbstractCursor.java:214)
            at android.database.CursorWrapper.moveToPosition(CursorWrapper.java:162)
            at android.support.v4.widget.CursorAdapter.getItemId(CursorAdapter.java:225)
            at android.widget.AdapterView.getItemIdAtPosition(AdapterView.java:778)
            at android.widget.AdapterView.setSelectedPositionInt(AdapterView.java:1185)
            at android.widget.AbsListView.onTouchUp(AbsListView.java:3837)
            at android.widget.AbsListView.onTouchEvent(AbsListView.java:3637)
            at android.view.View.dispatchTouchEvent(View.java:8471)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2399)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2092)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at org.mozilla.gecko.home.HomePager.dispatchTouchEvent(HomePager.java:342)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2405)
            at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2106)
            at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchTouchEvent(PhoneWindow.java:2369)
            at com.android.internal.policy.impl.PhoneWindow.superDispatchTouchEvent(PhoneWindow.java:1719)
            at android.a
> @@ +735,5 @@
> > +        StringBuilder suggestedSitesBuilder = new StringBuilder();
> > +        // TODO: we should access the underlying data directly here
> > +        // TODO 2: we should get the total number of pinned+top sites here, subtract that from limit, and fetch
> > +        // that many suggested sites - that should ensure that we don't show suggested sites outside of the grid area.
> > +        Cursor suggestedSitesCursor = suggestedSites.get(Integer.parseInt(limit));
> 
> You might consider changing the API here.
> 
> SuggestedSites builds and returns a MatrixCursor for the convenience of the
> code that we're deleting!
> 
> Much easier if we just have it return its internal structure, which is a
> Map<String, Site>, or spit out List<Site> instead.
>

We're doing some filtering in SuggestedSites.get(), so we can't return the underlying list directly right now. I've filed Bug 1249018 to reorganise SuggestedSites to perform the filtering earlier, since that seems beyond the scope of this bug - for now we're going to have to create a copy of the list in SuggestedSites in any case while fileting, so I'm guessing there's no huge difference between returning a MatrixCursor, and returning a Map or List?
Blocks: 1249018
This allows us to more easily append multiple sets of args.

2-param version appendSelectionArgs since it's more efficient for that
case (and the vast majority of uses are with 2-params) - it's probably
simpler for development to have both versions have the same name, and
as far as I can tell the compiler will prefer the non varargs version
when possible.

Review commit: https://reviewboard.mozilla.org/r/35311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35311/
Attachment #8720455 - Flags: review?(rnewman)
We'll need this to provide a number sequence for the single-query/cursor TopSites
implementation.

Review commit: https://reviewboard.mozilla.org/r/35313/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35313/
Attachment #8720456 - Flags: review?(rnewman)
We'll need access to the SuggestedSites in BrowserProvider when assembling the
topsites query there, hence we need to allow access via BrowserDB.

Review commit: https://reviewboard.mozilla.org/r/35315/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35315/
Attachment #8720457 - Flags: review?(rnewman)
Note that this version only returns topsites, pinned sites, and suggested sites.
Blank tiles aren't supplied, and need to be added separately.

Review commit: https://reviewboard.mozilla.org/r/35317/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35317/
Attachment #8720458 - Flags: review?(rnewman)
This means we now have only one open cursor for the topsites query, instead of 2 real cursors (and one
MatrixCursor). We still need a MergeCursor and MatrixCursor to supply blank tiles, this will
disappear as soon as the user has sufficient history items to fill the suggested sites grid.

Review commit: https://reviewboard.mozilla.org/r/35319/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35319/
Attachment #8720459 - Flags: review?(rnewman)
This is a quick sanity check: a clean profile should return all the suggested
sites directly.

We should probably add further tests making sure that history items appear
in front of the suggested sites, that pinned sites work correctly,
and that suggested sites don't appear outside of the grid.

Review commit: https://reviewboard.mozilla.org/r/35321/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35321/
Attachment #8720460 - Flags: review?(rnewman)
Attachment #8720455 - Flags: review?(rnewman) → review+
Comment on attachment 8720455 [details]
MozReview Request: Bug 760956 - Part 1: Introduce variadic concatenateSelectionArgs r=rnewman

https://reviewboard.mozilla.org/r/35311/#review32569
Comment on attachment 8720456 [details]
MozReview Request: Bug 760956 - Part 2: Introduce numbers table r=rnewman

https://reviewboard.mozilla.org/r/35313/#review32571

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:510
(Diff revision 1)
> +        StringBuilder sequenceBuilder = new StringBuilder();

`final`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:518
(Diff revision 1)
> +        db.execSQL("CREATE TABLE " + BrowserContract.Numbers.TABLE_NAME + " AS" +

`db.bulkInsert(values)`.
Attachment #8720456 - Flags: review?(rnewman) → review+
Comment on attachment 8720457 [details]
MozReview Request: Bug 760956 - Part 3: Add getSuggestedSites to BrowserDB r=rnewman

https://reviewboard.mozilla.org/r/35315/#review32575
Attachment #8720457 - Flags: review?(rnewman) → review+
Comment on attachment 8720458 [details]
MozReview Request: Bug 760956 - Part 4: Implement single-cursor topsite-retrieval in BrowserProvider r=rnewman

https://reviewboard.mozilla.org/r/35317/#review32577

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:658
(Diff revision 1)
> +        // 3. Join these, so that each topsite is given it's resulting position

Nit: "its"

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:744
(Diff revision 1)
> +            if (suggestedSitesBuilder.length() > 0) {

Use a `firstClause` boolean flag instead of checking length. Eschew obfuscation.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:748
(Diff revision 1)
> +                                                 " ? AS " + Bookmarks._ID + "," +

Line these up.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:754
(Diff revision 1)
> +                                                                    suggestedSitesCursor.getString(suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks._ID)),

Compute each index once, not N times.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:772
(Diff revision 1)
> +            db.execSQL("CREATE TABLE " + TABLE_TOPSITES + " AS" +

Just create the table once, truncate, and `INSERT INTO` here instead?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:811
(Diff revision 1)
> +        Cursor c = db.rawQuery(

`final`
Attachment #8720458 - Flags: review?(rnewman) → review+
Comment on attachment 8720459 [details]
MozReview Request: Bug 760956 - Part 5: use BrowserProvider TopSites query instead of TopSitesCursorWrapper r=rnewman

https://reviewboard.mozilla.org/r/35319/#review32583

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1705
(Diff revision 1)
> -        int pinnedCount = pinnedSites.getCount();
> +        // It's possible that we will retrieve less sites than are required to fill the top-sites panel - in this case

less -> fewer

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1711
(Diff revision 1)
> -        Cursor suggestedSites = null;
> +        if (blanksRequired > 0) {

Invert and early return, no else needed.
Attachment #8720459 - Flags: review?(rnewman) → review+
https://reviewboard.mozilla.org/r/35319/#review32585

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1711
(Diff revision 1)
> -        Cursor suggestedSites = null;
> +        if (blanksRequired > 0) {

Per discussion: let's add only one blank tile if we can. Make sure antlam's happy.
Attachment #8720460 - Flags: review?(rnewman) → review+
Comment on attachment 8720460 [details]
MozReview Request: Bug 760956 - Part 6: Introduce basic topsites retrieval test r=rnewman

https://reviewboard.mozilla.org/r/35321/#review33143

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestTopSites.java:64
(Diff revision 1)
> +        Cursor c = cr.query(uri,

`final`

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestTopSites.java:81
(Diff revision 1)
> +        c.close();

`finally`
Attachment #8720461 - Flags: review?(rnewman) → review+
Comment on attachment 8720461 [details]
MozReview Request: Bug 760956 - Post: Cleanup TopSitesCursorWrapper remainders r=rnewman

https://reviewboard.mozilla.org/r/35323/#review33147
Comment on attachment 8720462 [details]
MozReview Request: Bug 760956 - Post: remove unneeded getPinnedSites and getTopSites from BrowserDB r=me

https://reviewboard.mozilla.org/r/35325/#review33151
Attachment #8720462 - Flags: review+
Comment on attachment 8720565 [details]
MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman

https://reviewboard.mozilla.org/r/35365/#review33153

I'd like to see a very trivial test that:

* Opens a database
* Opens a transaction
* Creates and populates a temp table
* Queries it
* Ends the transaction and lets the cursor escape
* Tests reading the cursor, perhaps even from another thread.

That test will break if our assumptions about populating cursors are ever violated.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:807
(Diff revision 1)
> -            db.setTransactionSuccessful();
> +            c = db.rawQuery(

`final Cursor c = ...`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:845
(Diff revision 1)
> +            c.getCount();

We discussed using something `moveToFirst` here instead of `getCount`.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:847
(Diff revision 1)
> +            db.setTransactionSuccessful();

Add `return c;`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:851
(Diff revision 1)
>          return c;

`return null`
Attachment #8720565 - Flags: review?(rnewman)
Comment on attachment 8720455 [details]
MozReview Request: Bug 760956 - Part 1: Introduce variadic concatenateSelectionArgs r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35311/diff/1-2/
Comment on attachment 8720456 [details]
MozReview Request: Bug 760956 - Part 2: Introduce numbers table r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35313/diff/1-2/
Comment on attachment 8720457 [details]
MozReview Request: Bug 760956 - Part 3: Add getSuggestedSites to BrowserDB r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35315/diff/1-2/
Comment on attachment 8720458 [details]
MozReview Request: Bug 760956 - Part 4: Implement single-cursor topsite-retrieval in BrowserProvider r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35317/diff/1-2/
Comment on attachment 8720459 [details]
MozReview Request: Bug 760956 - Part 5: use BrowserProvider TopSites query instead of TopSitesCursorWrapper r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35319/diff/1-2/
Comment on attachment 8720460 [details]
MozReview Request: Bug 760956 - Part 6: Introduce basic topsites retrieval test r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35321/diff/1-2/
Comment on attachment 8720461 [details]
MozReview Request: Bug 760956 - Post: Cleanup TopSitesCursorWrapper remainders r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35323/diff/1-2/
Comment on attachment 8720462 [details]
MozReview Request: Bug 760956 - Post: remove unneeded getPinnedSites and getTopSites from BrowserDB r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35325/diff/1-2/
Comment on attachment 8720565 [details]
MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35365/diff/1-2/
Attachment #8720565 - Attachment description: MozReview Request: Bug 760956 - Part 7: use a true TEMP table by forcing cursor compilation before end of transaction r=rnewman → MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman
Attachment #8720565 - Flags: review?(rnewman)
Comment on attachment 8720456 [details]
MozReview Request: Bug 760956 - Part 2: Introduce numbers table r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35313/diff/2-3/
Comment on attachment 8720457 [details]
MozReview Request: Bug 760956 - Part 3: Add getSuggestedSites to BrowserDB r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35315/diff/2-3/
Comment on attachment 8720458 [details]
MozReview Request: Bug 760956 - Part 4: Implement single-cursor topsite-retrieval in BrowserProvider r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35317/diff/2-3/
Comment on attachment 8720459 [details]
MozReview Request: Bug 760956 - Part 5: use BrowserProvider TopSites query instead of TopSitesCursorWrapper r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35319/diff/2-3/
Comment on attachment 8720460 [details]
MozReview Request: Bug 760956 - Part 6: Introduce basic topsites retrieval test r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35321/diff/2-3/
Comment on attachment 8720461 [details]
MozReview Request: Bug 760956 - Post: Cleanup TopSitesCursorWrapper remainders r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35323/diff/2-3/
Comment on attachment 8720462 [details]
MozReview Request: Bug 760956 - Post: remove unneeded getPinnedSites and getTopSites from BrowserDB r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35325/diff/2-3/
Comment on attachment 8720456 [details]
MozReview Request: Bug 760956 - Part 2: Introduce numbers table r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35313/diff/2-3/
Comment on attachment 8720565 [details]
MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35365/diff/2-3/
Hardware: ARM → All
Summary: java.lang.IllegalStateException: at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java) → Reimplement top sites database access to use a single cursor
Version: 22 Branch → Trunk
Comment on attachment 8720565 [details]
MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman

https://reviewboard.mozilla.org/r/35365/#review33423
Attachment #8720565 - Flags: review?(rnewman) → review+
Comment on attachment 8720456 [details]
MozReview Request: Bug 760956 - Part 2: Introduce numbers table r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35313/diff/3-4/
Comment on attachment 8720457 [details]
MozReview Request: Bug 760956 - Part 3: Add getSuggestedSites to BrowserDB r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35315/diff/3-4/
Comment on attachment 8720458 [details]
MozReview Request: Bug 760956 - Part 4: Implement single-cursor topsite-retrieval in BrowserProvider r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35317/diff/3-4/
Comment on attachment 8720459 [details]
MozReview Request: Bug 760956 - Part 5: use BrowserProvider TopSites query instead of TopSitesCursorWrapper r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35319/diff/3-4/
Comment on attachment 8720460 [details]
MozReview Request: Bug 760956 - Part 6: Introduce basic topsites retrieval test r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35321/diff/3-4/
Comment on attachment 8720461 [details]
MozReview Request: Bug 760956 - Post: Cleanup TopSitesCursorWrapper remainders r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35323/diff/3-4/
Comment on attachment 8720462 [details]
MozReview Request: Bug 760956 - Post: remove unneeded getPinnedSites and getTopSites from BrowserDB r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35325/diff/3-4/
Comment on attachment 8720565 [details]
MozReview Request: Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35365/diff/3-4/
Tracking this since this is a topcrash in 46 fennec and it looks like a fix may be coming soon!
status-firefox47: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
https://hg.mozilla.org/integration/fx-team/rev/f0d9ab6ffd64bd61e90dc2a13120df985c91c823
Bug 760956 - Part 1: Introduce variadic concatenateSelectionArgs r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/d6e7971cd98b8db5ed78421652e52848d29f71fa
Bug 760956 - Part 2: Introduce numbers table r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/633a0cff7f55ec82a3b75b22e424dc5f68779912
Bug 760956 - Part 3: Add getSuggestedSites to BrowserDB r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/613f1eb88443821b3a7c3787d14e1f0238e4a4d0
Bug 760956 - Part 4: Implement single-cursor topsite-retrieval in BrowserProvider r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/86d110d3d0eb17bfbaa3f8d4a7287466621ad710
Bug 760956 - Part 5: use BrowserProvider TopSites query instead of TopSitesCursorWrapper r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/94b25f1233ddb1df2975a39e02ee8637846ba25b
Bug 760956 - Part 6: Introduce basic topsites retrieval test r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/045c87312d9a21b4b6b9c3fb8f84e680e6203ee8
Bug 760956 - Post: Cleanup TopSitesCursorWrapper remainders r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/f0b63f1b4e32534ea919c25289818e8728d0de3f
Bug 760956 - Post: remove unneeded getPinnedSites and getTopSites from BrowserDB r=me

https://hg.mozilla.org/integration/fx-team/rev/446932a345fe1631718eb0b6bcb3e098c2958265
Bug 760956 - Part 7: only add single blank tile to top sites grid r=rnewman
I think this is too risky to uplift. I would be fine with letting this ride the trains.
if this has been implemented in Nightly, i seem to be hitting a bug and a crash.  

BUG:  
Can't pin sites. 
Try edit/pin a site on the topsites icons thing and it seems to disappear  

CRASH:  
Tap and hold any site on the list below the top sites icons.   
https://crash-stats.mozilla.com/report/index/dca6fc89-37ec-4c12-a78f-0f4ba2160301
Depends on: 1252500
Blocks: 1252499
Depends on: 1252501
Maybe this should be backed out and reland in 6 days when we have branched for 48 development cycle. Filed bug 1252500 for the crash. Bug 1252501 is for the top sites.
Careful: it's hard to back out database migrations.
(In reply to Kevin Brosnan [:kbrosnan] from comment #116)
> Maybe this should be backed out and reland in 6 days when we have branched
> for 48 development cycle. Filed bug 1252500 for the crash. Bug 1252501 is
> for the top sites.

If we do a backout, we should avoid backing out Part 2 since that involves a database migration (it only adds a new table, and hence doesn't affect the remaining functionality - hence that shouldn't be an issue to leave committed).

However I've got patches for the crashes, which only leaves Bug 1252501 to fix (edit/pin sites bug), which I'm not able to reproduce yet.
Depends on: 1252610
Depends on: 1252613
Depends on: 1254468
(In reply to :Margaret Leibovic from comment #114)
> I think this is too risky to uplift. I would be fine with letting this ride
> the trains.

FYI I came accross this bug by my sister, 100% repro crashing stable Firefox for Android by opening 4 tabs at once on Sony Xperia Z2 Tab device. 

Looking at https://crash-stats.mozilla.com/report/list?product=FennecAndroid&signature=java.lang.IllegalStateException%3A+at+android.database.sqlite.SQLiteClosable.acquireReference%28SQLiteClosable.java%29 I see around 41000 reported crashes over the last 7 days.
(In reply to Alexandre LISSY :gerard-majax from comment #119)
> (In reply to :Margaret Leibovic from comment #114)
> > I think this is too risky to uplift. I would be fine with letting this ride
> > the trains.
> 
> FYI I came accross this bug by my sister, 100% repro crashing stable Firefox
> for Android by opening 4 tabs at once on Sony Xperia Z2 Tab device. 
> 
> Looking at
> https://crash-stats.mozilla.com/report/
> list?product=FennecAndroid&signature=java.lang.
> IllegalStateException%3A+at+android.database.sqlite.SQLiteClosable.
> acquireReference%28SQLiteClosable.java%29 I see around 41000 reported
> crashes over the last 7 days.

Bug 1254468 is far more likely to solve this issue - this bug is an improvement over our previous topsites loading (less cursors == less resources used) but doesn't actually solve the underlying problem (i.e. that the Cursor is being closed too early).
Are we planning to uplift this? It's #2 top crash in 46 beta.
Flags: needinfo?(margaret.leibovic)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #122)
> Are we planning to uplift this? It's #2 top crash in 46 beta.

I'm not convinced this is the silver bullet to fix that crash, and this is a big risky patch series, so I don't think we should uplift it.
Flags: needinfo?(margaret.leibovic)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.