785945 - Awesomescreen entries are very slow to appear

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

I feel like awesomescreen performance on my default profile (on a Galaxy Nexus running JB) has gradually become worse, potentially because of an increasing history database.

This morning dbaron complained to me about performance on his phone this morning, and it took at least 5 seconds for the top sites tab to populate, and that's not acceptable. We should look into how to fix this.

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #0)
> I feel like awesomescreen performance on my default profile (on a Galaxy
> Nexus running JB) has gradually become worse, potentially because of an
> increasing history database.

We log the time it took to run the query on the content provider. See:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/AllPagesTab.java#160

What's the time you see in logcat? Two things might be causing the perf problem:

1. The obvious one: the database query is too slow.
2. On startup, we're flooding the background thread with runnables that are executed before the runnable that actually runs the awesomescreen query.

I reason I think 2 is happening is because the query tends to get much faster once the Fennec is fully loaded. Maybe this is just a result of SQLite memcaching results. Needs investigation.

> This morning dbaron complained to me about performance on his phone this
> morning, and it took at least 5 seconds for the top sites tab to populate,
> and that's not acceptable. We should look into how to fix this.

Ouch. Is that on startup or after Fennec is fully loaded?

Comment 2

[:Margaret Leibovic]

It varies, but it can be slow (Galaxy Nexus with JB). Here's a sample:
I/ALL_PAGES( 1633): Got cursor in 15593ms <-- after quitting/restarting
I/ALL_PAGES( 1633): Got cursor in 2828ms
I/ALL_PAGES( 1633): Got cursor in 2534ms
I/ALL_PAGES( 1633): Got cursor in 891ms
I/ALL_PAGES( 1633): Got cursor in 352ms
I/ALL_PAGES( 1633): Got cursor in 430ms
I/ALL_PAGES( 1633): Got cursor in 383ms
I/ALL_PAGES( 1633): Got cursor in 488ms
I/ALL_PAGES( 1633): Got cursor in 305ms
I/ALL_PAGES( 1633): Got cursor in 1373ms
I/ALL_PAGES( 1633): Got cursor in 661ms
I/ALL_PAGES( 1633): Got cursor in 1084ms
I/ALL_PAGES( 1633): Got cursor in 700ms
I/ALL_PAGES( 2508): Got cursor in 11133ms <-- after quitting/restarting
I/ALL_PAGES( 2508): Got cursor in 1046ms

Also, I was opening/closing the awesomescreen with logcat active, and I noticed that I would always see (at least) two messages being logged when opening the awesome screen. Looking at the code, it doesn't seem like that's expected, and I'm wondering if we could be accidentally issuing multiple queries (and if that would slow down the DB).

Playing around with a new profile, I'm seeing:
I/ALL_PAGES( 2731): Got cursor in 529ms <-- startup
I/ALL_PAGES( 2731): Got cursor in 20ms
I/ALL_PAGES( 2731): Got cursor in 49ms
I/ALL_PAGES( 2731): Got cursor in 7ms
I/ALL_PAGES( 2731): Got cursor in 40ms
I/ALL_PAGES( 2731): Got cursor in 29ms
I/ALL_PAGES( 2731): Got cursor in 7ms
I/ALL_PAGES( 2731): Got cursor in 13ms

This makes me think that a big history DB is probably what's really killing us. Maybe this is a reason to make bug 744961 more important. Or we should figure out how to optimize what we're doing right now.

Comment 3

[Richard Newman [:rnewman]]

> 2. On startup, we're flooding the background thread with runnables that are
> executed before the runnable that actually runs the awesomescreen query.

Might I suggest that we log from inside run() with a per-runnable ID? Seems like an easy hypothesis to test how startup is being scheduled, possibly with some parallelism wins to be found.


(In reply to Margaret Leibovic [:margaret] from comment #2)

> Also, I was opening/closing the awesomescreen with logcat active, and I
> noticed that I would always see (at least) two messages being logged when
> opening the awesome screen. Looking at the code, it doesn't seem like that's
> expected, and I'm wondering if we could be accidentally issuing multiple
> queries (and if that would slow down the DB).

Add the object itself to the log output, see if it's the same AllPagesTab instance doing the logging -- maybe we're building the tab twice!

Failing that, perhaps something is causing a data change event that causes the cursor adapter to re-evaluate its filter, which will fetch a cursor again.

One way to tell is to attach a debugger and breakpoint there…


But yes, we should be spotting instances where it takes a ridiculous amount of time, or we run out of memory, and pruning history accordingly. We should attack both fronts, IMO.

Comment 4

[Richard Newman [:rnewman]]

Oh, another thing -- when we log that we grabbed a cursor, it should be cheap for us to also log how many rows Android thinks it has. That might be telling, even if we LIMIT the query.

Comment 5

[Matt Brubeck (:mbrubeck)]

Since our frecency code does date-sensitive calculations in its ORDER BY clause, it can't use an index, and must always do a full table scan (including frecency calculation for every row).

Desktop Firefox gets around this by storing the frecency score in the database and updating it on idle-daily.  However, this requires an expensive idle-daily job that we should try to avoid.  Bug 704025 proposes a frecency algorithm that is index-friendly without the need for daily updates.  Perhaps we should use this in Fennec.

If we can't use an index, then we will always slow down linearly as the number of history items and bookmarks grows, which means we will need to enforce strict limits on those numbers.

Comment 6

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

If a low risk patch comes in we can consider for Fx16, but that's doubtful.

Comment 7

[Richard Newman [:rnewman]]

The suggestion I casually discussed with mconnor (who still remembers the implementation of desktop awesomebar) and margaret was to pre-limit on the important variables inside the query, such that we only bother doing the ORDER BY on a small set of rows.

E.g., grab the N most recent, and the M most frequent (both of which should already be indexed), then order and limit outside that.

The theory is that no row that isn't either recent or frequent is going to have a good chance of being frecent.

This should be straightforward to (a) hack together, and (b) experimentally verify.

Comment 8

[Richard Newman [:rnewman]]

ph34r.

SELECT *,
       (visits * 
        MAX(1, (100 * 225) /
               (agedays * agedays + 225))) AS frecency
FROM (
SELECT *,
       ((date - strftime('%s000', 'now')) /
        (24 * 60 * 60 * 1000)) AS agedays FROM 
  (SELECT _id, guid, url, title, date, visits
   FROM history_with_images
   ORDER BY date DESC
   LIMIT 30)
UNION
SELECT *,
       ((date - strftime('%s000', 'now')) /
        (24 * 60 * 60 * 1000)) AS agedays FROM
 (SELECT _id, guid, url, title, date, visits
  FROM history_with_images
  ORDER BY visits DESC
  LIMIT 30)
)

ORDER BY frecency DESC;


Please excuse the annoying repetition. SQL doesn't allow self-reference between column aliases.

In English:

* Grab the top most recently visited pages;
* Grab the top most frequently visited pages;
* Merge them;
* Compute their age in days (surely we can do better with more granularity?);
* Compute the dynamic frecency on top, and sort.

Comment 9

[:Margaret Leibovic]

Attachment: WIP

I tried to hack my debug build to use my default browser.db, but I couldn't get it to work (I always got an exception about the db being locked). Instead, I ran testBrowserProviderPerf with and without this patch, but it regressed, so I don't know if this is actually a good fix :(

Without patch: 398, 395
With patch: 767, 770

I also noticed that testBrowserProviderPerf only measures a query with a filter string, so I decided to tweak the test to run with an empty filter string. The numbers without the patch match the numbers I was seeing with my slow default profile, so that seems to indicate we made a good testcase. But unfortunately, the numbers with my patch are pretty terrible :(

Without patch: 1344, 1341
With patch: 2576, 2629

Comment 10

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #9)

> With patch: 2576, 2629

Getting rid of the "GROUP BY"s improved this to 2029, 2060, but we'd need to do something to keep that functionality somehow.

Comment 11

[:Margaret Leibovic]

In case it helps with playing around with SQL, here's what the query looks like after all the concatenation (you'll have to replace ?s with filter strings):

SELECT *,
       ((CASE WHEN bookmark_id > -1 THEN 100 ELSE 0 END) +
        visits * MAX(1, (100 * 225) / (agedays * agedays + 225))) AS frecency
FROM (
SELECT *,
       ((date - strftime('%s000', 'now')) / 86400000) AS agedays FROM
  (SELECT _id,url,title,favicon,display,bookmark_id,history_id,bookmark_id,date,visits
   FROM combined_with_images
   WHERE (url LIKE ? OR title LIKE ?) ORDER BY date DESC LIMIT 100)
UNION
SELECT *,
       ((date - strftime('%s000', 'now')) / 86400000) AS agedays FROM
  (SELECT _id,url,title,favicon,display,bookmark_id,history_id,bookmark_id,date,visits
   FROM combined_with_images WHERE (url LIKE ? OR title LIKE ?)
   ORDER BY visits DESC LIMIT 100)
) ORDER BY frecency DESC;

Comment 12

[Richard Newman [:rnewman]]

Margaret: there are two things different in this query.

* We're not substituting System.currentTimeMillis(), so we're doing a bunch of strftime() calls.
* There's a lot more SQL to plan and parse.

So here are two things to try:

* Switch the timestamp calculation to concatenate the current timestamp into the string, just as we do in the existing query code.
* Define a view for each of the subqueries, see if that reduces SQL parsing overhead.

There's one additional point.

LIKE can't use an index.

That means that any query in which you expect to use string filtering won't be improved if you try to eliminate full table scans… y'can't, because SQLite already has to do a scan to implement LIKE.

We might only be able to get a win here for Top Sites -- use a low LIMIT and no LIKE.

(From the SQLite Optimization FAQ: "However, there are a number of cases in which an index can slow a query down: … If most or all of the table would need to be loaded anyway. (e.g. when an expression contains a LIKE operator)")

Comment 13

[Richard Newman [:rnewman]]

To be clear: the reason this query got slower is that we exchanged *one* full-table scan for *two* full-table scans. The reason it's quicker than running the query twice is that we do some of the work only on a small set of results.

If this were a full-fledged DB like Postgres, this is the point at which I'd suggest a trigram index.

Comment 14

[:Margaret Leibovic]

Comment on attachment 664729 [details] [diff] [review]
WIP

After some more investigation yesterday, we found that what's actually really killing us is our giant combined view. Just doing "SELECT <stuff we want> FROM combined_with_images ORDER BY date DESC LIMIT 100" was taking over 1000ms on my device. I'm going to investigate postponing the join on the images table until after we get the rows we want, and see if that improves things.

Comment 15

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

Just wanted to write this down: We talked previously about _not_ joining to favicons when doing the primary query. We would create the list of history/bookmarks with placeholder (or empty) images. As a row was being shown, we would start a runnable to load the favicon via the Favicon Java code, which could cache the favicons too.

This is similar to what Twitter does with avatars.

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> Just wanted to write this down: We talked previously about _not_ joining to
> favicons when doing the primary query. We would create the list of
> history/bookmarks with placeholder (or empty) images. As a row was being
> shown, we would start a runnable to load the favicon via the Favicon Java
> code, which could cache the favicons too.
> 
> This is similar to what Twitter does with avatars.

Margaret, I have a lot of code from Pattrn that I can provide as reference for this btw. Just let me know if you're interested :-)

Comment 17

[:Margaret Leibovic]

My priorities are changing, so unfortunately I won't have time to continue looking into this right now.

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Add a version of the combined view without the images table join

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: Add API to get multiple favicons at once from DB

Comment 20

[Lucas Rocha (:lucasr)]

Attachment: Load favicon images asynchronously in the All Pages tab

Comment 21

[Lucas Rocha (:lucasr)]

So, after quite a bit testing... I noticed that the presence of a large quantity of images in our DB can slow us down quite a bit. The problem obviously gets much worse when we store large images (I'm looking at you thumbnails).

So, as a first measure, we should not touch images on queries that we want to be fast. These patches change the "All Pages" tab in awesome screen to load favicon asynchronously. I'll do the same on about:home (where the images have even bigger impact).

It's hard to tell how much faster this will get—it's very annoying to test this kind of change as it involves DB schema upgrades and all. There's definitely some worthy gain as we're removing one table scan (from the images left join) and tons of large blobs from the query. We should probably land these in nightly and see how it goes.

These patches use LruCache which is available in Android's support library. For now, I'm simply copying LruCache into our repo. We can remove it once we get the build infra ready to use the support library.

Comment 22

[Lucas Rocha (:lucasr)]

Attachment: Copy LruCache into our repo

Temporary copy just until we get built with Android's support library.

Comment 23

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #22)
> Created attachment 674675 [details] [diff] [review]
> Copy LruCache into our repo
> 
> Temporary copy just until we get built with Android's support library.

Please file a follow-up to track removing this, and mark it as a dependency of the support library. It's one of those things that gets forgotten!

(And I'd love to know the bug # for the support library availability; there's a few things from there that s-i would like to use.)

Ta!

Comment 24

[Wesley Johnston (:wesj)]

That's in bug 759041. I can land that piece now if it helps the world.

Comment 25

[Lucas Rocha (:lucasr)]

Attachment: Add a LRU memory cache to Favicons and corresponding API

Comment 26

[Lucas Rocha (:lucasr)]

Attachment: Load favicon images asynchronously in the All Pages tab

Comment 27

[Lucas Rocha (:lucasr)]

Attachment: Add API to get multiple thumbnails at once from DB

Comment 28

[Lucas Rocha (:lucasr)]

Attachment: Load top site thumbnails asynchronously in about:home

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 675188 [details] [diff] [review]
Add a LRU memory cache to Favicons and corresponding API

I added some memory pressure bits so that we drop the favicon memory cache if we're under medium+ pressure. It would be nice if kats had a look.

Comment 30

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #25)
> Created attachment 675188 [details] [diff] [review]
> Add a LRU memory cache to Favicons and corresponding API

Not entirely sure how to properly size this cache here. It needs to be big enough to hold a page of awesomescreen results (up to 100 entries now, I'll probably reduce it to 50). Any input here on this regard is welcome.

Comment 31

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

Comment on attachment 675188 [details] [diff] [review]
Add a LRU memory cache to Favicons and corresponding API

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

::: mobile/android/base/MemoryMonitor.java
@@ +155,5 @@
>              }
>              ScreenshotHandler.disableScreenshot(false);
>              GeckoAppShell.geckoEventSync();
> +
> +            GeckoApp.mAppContext.getFavicons().clearMemCache();

I'd rather put this line above the geckoEventSync() so that it can run in parallel with gecko's onLowMemory handling.

Also, in general, I like use SoftReference to hold on to caches like this since then dalvik can drop the cache whenever it decides it needs more memory, rather than going to the extreme of having to send out memory-pressure events. Depends on how tightly we want to hold on to the cache, I guess.

Comment 32

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

Comment on attachment 674671 [details] [diff] [review]
Add API to get multiple favicons at once from DB

Long term, I am on the fence about using a batch-like API for getting the favicons all at once. But I am willing to see how this works and tweak it over time.

Comment 33

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

Comment on attachment 675188 [details] [diff] [review]
Add a LRU memory cache to Favicons and corresponding API

>diff --git a/mobile/android/base/MemoryMonitor.java b/mobile/android/base/MemoryMonitor.java

>+
>+            GeckoApp.mAppContext.getFavicons().clearMemCache();

We are trying to reduce usage of GeckoApp.mAppContext so we might want to think of other ways to do this in a followup bug.

Comment 34

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

Comment on attachment 675190 [details] [diff] [review]
Load favicon images asynchronously in the All Pages tab

>+    private List<String> getUrlsWithoutFavicon() {

>+            Favicons favicons = GeckoApp.mAppContext.getFavicons();

Will uses of mAppContext fail if the device has "Kill activities" setting on?

>+    public void storeFaviconsInMemCache(Cursor c) {

>+            Favicons favicons = GeckoApp.mAppContext.getFavicons();

Same

Comment 35

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #32)
> Comment on attachment 674671 [details] [diff] [review]
> Add API to get multiple favicons at once from DB
> 
> Long term, I am on the fence about using a batch-like API for getting the
> favicons all at once. But I am willing to see how this works and tweak it
> over time.

Two main reasons I implemented this using a batch-line API:

1. We touch the disk only once for the current result set in awesome screen (note that we only fetch the favicons from DB which are not in the memory cache yet)
2. Favicons are displayed immediately as you scroll the current result set (as opposed to a visible delay while scrolling, in case we loaded the images as you scroll)

In other words: we get the benefit of loading all the images asynchronously without continuously touching the disk nor having a strange delay to show favicons as you scroll the list.

Comment 36

[Lucas Rocha (:lucasr)]

Just some (non-scientific) benchmark results. This is based on the averaged telemetry data after running Fennec several times locally. I have a medium-sized database with a good number of favicons and thumbnails. The perf gain might be bigger on larger databases.

awesomescreen (time to show initial results):
 * Without the patches: 201.25ms
 * With the patches: 128.25ms
 * Result: ~36% faster

about:home (time to show top sites):
 * Without the patches: 1854.6ms
 * With the patches: 1236.2ms
 * Result: ~33% faster

So, this is already a substantial performance gain. I'll experiment with the ideas described in comment #8 to see how much faster we can get with a different query.

Comment 37

[Lucas Rocha (:lucasr)]

Attachment: Remove all uses of GeckoApp.mAppContext from Favicons

Comment 38

[Thomas Stache]

Are there try builds(In reply to Lucas Rocha (:lucasr) from comment #36)
> awesomescreen (time to show initial results):
>  * Without the patches: 201.25ms
>  * With the patches: 128.25ms
>  * Result: ~36% faster
> 
> about:home (time to show top sites):
>  * Without the patches: 1854.6ms
>  * With the patches: 1236.2ms
>  * Result: ~33% faster

Are there try builds for Fennec? My HTC One V shows waaay slower times using the nightly than what you see.
Is it possible to determine the size of my DB (that is synced over from desktop), i.e. number of history records, favicons, thumbnails?

Comment 39

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

Comment on attachment 675193 [details] [diff] [review]
Load top site thumbnails asynchronously in about:home

>diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml

>-      <item name="android:scaleType">centerCrop</item>
>+      <item name="android:scaleType">fitCenter</item>

What's the reason for this change?

Comment 40

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #39)
> Comment on attachment 675193 [details] [diff] [review]
> Load top site thumbnails asynchronously in about:home
> 
> >diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml
> 
> >-      <item name="android:scaleType">centerCrop</item>
> >+      <item name="android:scaleType">fitCenter</item>
> 
> What's the reason for this change?

Making images load asynchronously exposed this bug. The default image has to be center, no stretched in the thumbnail area. This wasn't visible before because we were replacing it immediately. Now this default image is visible until the DB query returns.

Comment 41

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #33)
> Comment on attachment 675188 [details] [diff] [review]
> Add a LRU memory cache to Favicons and corresponding API
> 
> >diff --git a/mobile/android/base/MemoryMonitor.java b/mobile/android/base/MemoryMonitor.java
> 
> >+
> >+            GeckoApp.mAppContext.getFavicons().clearMemCache();
> 
> We are trying to reduce usage of GeckoApp.mAppContext so we might want to
> think of other ways to do this in a followup bug.

I could add something like GeckoAppShell.getFavicons() but not sure if this would be an improvement.

Comment 42

[Lucas Rocha (:lucasr)]

Here's another update.

Margaret was using combined_with_images in the query suggested in comment #8 query which explains the performance hit. The query is supposed to be run on the history tablet instead. So, I tried several variations of the ideas in comment #8 but my conclusion is that there's no relevant performance gain from it. It would probably involve a more complex query setup in our ContentProvider which is not worth it (considering the performance results).

Given that we'll be expiring history entries quite aggressively now (see bug 744961), I think should just land these patches here and see how goes. I think the combination of less history entries and a query that doesn't touch images will give as a substantial performance boost both on awesomescreen and about:home.

Comment 43

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #34)
> Comment on attachment 675190 [details] [diff] [review]
> Load favicon images asynchronously in the All Pages tab
> 
> >+    private List<String> getUrlsWithoutFavicon() {
> 
> >+            Favicons favicons = GeckoApp.mAppContext.getFavicons();
> 
> Will uses of mAppContext fail if the device has "Kill activities" setting on?

Nice catch. Will test this before pushing.

Comment 44

[Richard Newman [:rnewman]]

Might I suggest an approach similar to MemoryMonitor.StorageReducer? That is, chain a context through to wherever the calling code or the persistent object are created?

Generally I would advise against pushing logic into GeckoAppShell just because it requires a context.

Comment 45

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #34)
> Comment on attachment 675190 [details] [diff] [review]
> Load favicon images asynchronously in the All Pages tab
> 
> >+    private List<String> getUrlsWithoutFavicon() {
> 
> >+            Favicons favicons = GeckoApp.mAppContext.getFavicons();
> 
> Will uses of mAppContext fail if the device has "Kill activities" setting on?

FYI: Tested this with "Don't keep activity" on and it works fine. I've filed bug 807279 to add an API to access Favicons that doesn't require mAppContext.

Comment 46

[Lucas Rocha (:lucasr)]

Decided to land this without the support library for now. I filed bug 807280 (with patch) as a follow-up to remove our local copy of LruCache.

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/082cf8d72554
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3b8237e76e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6604e038bc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e8f151ca67
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd27e755a83
https://hg.mozilla.org/integration/mozilla-inbound/rev/174634554a97
https://hg.mozilla.org/integration/mozilla-inbound/rev/49beb3801192
https://hg.mozilla.org/integration/mozilla-inbound/rev/a03d8d4db1c2

Comment 47

[Geoff Brown [:gbrown]]

As philor discussed on irc, we're seeing exceptions during robocop tests, which might be caused by this push. Here's a sample:

I/Robocop ( 9062): 5 INFO TEST-PASS | testHistory | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_blank_02.html should equal http://mochi.test:8888/tests/robocop/robocop_blank_02.html
D/Robocop ( 9062): waiting for DOMContentLoaded
D/AndroidRuntime( 9062): Shutting down VM
E/GeckoAppShell( 9062): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoAppShell( 9062): android.database.StaleDataException: Access closed cursor
E/GeckoAppShell( 9062): 	at android.database.AbstractWindowedCursor.checkPosition(AbstractWindowedCursor.java:217)
E/GeckoAppShell( 9062): 	at android.database.AbstractWindowedCursor.getString(AbstractWindowedCursor.java:41)
E/GeckoAppShell( 9062): 	at android.database.CursorWrapper.getString(CursorWrapper.java:135)
E/GeckoAppShell( 9062): 	at android.database.CursorWrapper.getString(CursorWrapper.java:135)
E/GeckoAppShell( 9062): 	at org.mozilla.gecko.AllPagesTab.getUrlsWithoutFavicon(AllPagesTab.java:738)
E/GeckoAppShell( 9062): 	at org.mozilla.gecko.AllPagesTab.loadFaviconsForCurrentResults(AllPagesTab.java:779)
E/GeckoAppShell( 9062): 	at org.mozilla.gecko.AllPagesTab.access$1700(AllPagesTab.java:61)
E/GeckoAppShell( 9062): 	at org.mozilla.gecko.AllPagesTab$AllPagesHandler.handleMessage(AllPagesTab.java:840)
E/GeckoAppShell( 9062): 	at android.os.Handler.dispatchMessage(Handler.java:99)
E/GeckoAppShell( 9062): 	at android.os.Looper.loop(Looper.java:123)
E/GeckoAppShell( 9062): 	at android.app.ActivityThread.main(ActivityThread.java:4627)
E/GeckoAppShell( 9062): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell( 9062): 	at java.lang.reflect.Method.invoke(Method.java:521)
E/GeckoAppShell( 9062): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:868)
E/GeckoAppShell( 9062): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:626)
E/GeckoAppShell( 9062): 	at dalvik.system.NativeStart.main(Native Method)
I/WindowManager( 1019): WIN DEATH: Window{485a22f0 org.mozilla.fennec/org.mozilla.fennec.App paused=false}
I/Process ( 1019): Sending signal. PID: 9130 SIG: 9

Comment 48

[Richard Newman [:rnewman]]

My guess: you've got a background task that loads the favicons, but you've already closed the AllPagesTab, and its mCursorAdapter has been destroyed, which closes -- but does not release the reference to! -- its inner cursor.

Basically you're racing the cleanup of the cursor.

You should do one or all of the following:

* Check isClosed before accessing the cursor.
* Discard the handle to the cursor after you close it. (Still subject to a race.)  Now you'll be checking for null.
* Subscribe to the invalidation event in the dataset observer, and recreate the cursor appropriately.
* Kill the background task that would use the cursor when the cursor adapter is 'invalidated'.

Comment 49

[Richard Newman [:rnewman]]

Note that the common solution is to c.requery(), but that's deprecated.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this was backed out due to Android bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2301d07ca953

Comment 51

[Lucas Rocha (:lucasr)]

Ok, this change fixes the issue (cancel any pending task to update favicons when the view is destroyed):

--- a/mobile/android/base/awesomebar/AllPagesTab.java
+++ b/mobile/android/base/awesomebar/AllPagesTab.java
@@ -151,16 +151,19 @@ public class AllPagesTab extends Awesome
         unregisterEventListener("SearchEngines:Data");
         if (adapter == null) {
             return;
         }
 
         Cursor cursor = adapter.getCursor();
         if (cursor != null)
             cursor.close();
+
+        mHandler.removeMessages(MESSAGE_UPDATE_FAVICONS);
+        mHandler.removeMessages(MESSAGE_LOAD_FAVICONS);
     }
 
     public void filter(String searchTerm) {
         AwesomeBarCursorAdapter adapter = getCursorAdapter();
         adapter.filter(searchTerm);
 
         filterSuggestions(searchTerm);
         if (mSuggestionsOptInPrompt != null) {

Comment 52

[Lucas Rocha (:lucasr)]

Sent new patches to Try, works fine.

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd817e8facb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d9ebf98fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/2acb72df1231
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2d68fe2229
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d964811d66e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9371388eeb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c260c5b70054
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b3dcdc5797

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd817e8facb6
https://hg.mozilla.org/mozilla-central/rev/dc9d9ebf98fe
https://hg.mozilla.org/mozilla-central/rev/2acb72df1231
https://hg.mozilla.org/mozilla-central/rev/bf2d68fe2229
https://hg.mozilla.org/mozilla-central/rev/3d964811d66e
https://hg.mozilla.org/mozilla-central/rev/6d9371388eeb
https://hg.mozilla.org/mozilla-central/rev/c260c5b70054
https://hg.mozilla.org/mozilla-central/rev/d0b3dcdc5797

Comment 56

[Cristian Nicolae (:xti)]

Awesomescreen entries are displayed immediately on the latest Nightly. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-12-06)
Device: Galaxy Tab2 7"
OS: Android 4.0.3