919768 - Slowdown and OOM hit visiting top-sites in about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: raw log (Nightly 09/23)

Currently I did a little experimenting on a fresh-profile opening about a dozen news article links off of http://hardocp.com on their front-page. I then began check on my top-sites and began pinning and unpinning all of the positions. In doing so the responsiveness and time it took to complete the action got slower and slower. Checking on log-cat revealed the following:

--

I/dalvikvm(16918): "ModernAsyncTask #5" prio=5 tid=21 RUNNABLE
I/dalvikvm(16918):   | group="main" sCount=0 dsCount=0 obj=0x42276a40 self=0x72e12b48
I/dalvikvm(16918):   | sysTid=17010 nice=10 sched=0/0 cgrp=apps/bg_non_interactive handle=1927032504
I/dalvikvm(16918):   | state=R schedstat=( 0 0 0 ) utm=255 stm=24 core=0
I/dalvikvm(16918):   at android.graphics.BitmapFactory.nativeDecodeByteArray(Native Method)
I/dalvikvm(16918):   at android.graphics.BitmapFactory.decodeByteArray(BitmapFactory.java:441)
I/dalvikvm(16918):   at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:108)
I/dalvikvm(16918):   at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:97)
I/dalvikvm(16918):   at org.mozilla.gecko.db.LocalBrowserDB.getFaviconForUrl(LocalBrowserDB.java:705)
I/dalvikvm(16918):   at org.mozilla.gecko.db.BrowserDB.getFaviconForUrl(BrowserDB.java:246)
I/dalvikvm(16918):   at org.mozilla.gecko.home.TopSitesPage$ThumbnailsLoader.loadInBackground(TopSitesPage.java:687)
I/dalvikvm(16918):   at org.mozilla.gecko.home.TopSitesPage$ThumbnailsLoader.loadInBackground(TopSitesPage.java:644)
I/dalvikvm(16918):   at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:240)
I/dalvikvm(16918):   at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
I/dalvikvm(16918):   at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
I/dalvikvm(16918):   at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
I/dalvikvm(16918):   at java.util.concurrent.FutureTask.run(FutureTask.java:234)
I/dalvikvm(16918):   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
I/dalvikvm(16918):   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
I/dalvikvm(16918):   at java.lang.Thread.run(Thread.java:841)
I/dalvikvm(16918): 
E/GeckoBitmapUtils(16918): decodeByteArray(bytes.length=157266, options= null) OOM!
E/GeckoBitmapUtils(16918): java.lang.OutOfMemoryError
E/GeckoBitmapUtils(16918):  at android.graphics.BitmapFactory.nativeDecodeByteArray(Native Method)
E/GeckoBitmapUtils(16918):  at android.graphics.BitmapFactory.decodeByteArray(BitmapFactory.java:441)
E/GeckoBitmapUtils(16918):  at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:108)
E/GeckoBitmapUtils(16918):  at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:97)
E/GeckoBitmapUtils(16918):  at org.mozilla.gecko.db.LocalBrowserDB.getFaviconForUrl(LocalBrowserDB.java:705)
E/GeckoBitmapUtils(16918):  at org.mozilla.gecko.db.BrowserDB.getFaviconForUrl(BrowserDB.java:246)
E/GeckoBitmapUtils(16918):  at org.mozilla.gecko.home.TopSitesPage$ThumbnailsLoader.loadInBackground(TopSitesPage.java:687)
E/GeckoBitmapUtils(16918):  at org.mozilla.gecko.home.TopSitesPage$ThumbnailsLoader.loadInBackground(TopSitesPage.java:644)
E/GeckoBitmapUtils(16918):  at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:240)
E/GeckoBitmapUtils(16918):  at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
E/GeckoBitmapUtils(16918):  at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
E/GeckoBitmapUtils(16918):  at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
E/GeckoBitmapUtils(16918):  at java.util.concurrent.FutureTask.run(FutureTask.java:234)
E/GeckoBitmapUtils(16918):  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
E/GeckoBitmapUtils(16918):  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
E/GeckoBitmapUtils(16918):  at java.lang.Thread.run(Thread.java:841)

--

This is reproducible on my Samsung Galaxy SIV (Android 4.3) which shouldn't have any speed problems doing this simple task.

Tested on Nightly (09/23)

Comment 1

[Aaron Train [:aaronmt]]

Oh this also happens just by re-visiting about:home top-sites and not doing any pinning/unpinning when meeting the conditions that triggered this

Comment 2

[Aaron Train [:aaronmt]]

Attachment: raw log more (Nightly 09/23)

reloading about:home

Comment 3

[Aaron Train [:aaronmt]]

Let me know if more is needed for this investigation.

Comment 4

[Richard Newman [:rnewman]]

https://rnewman.pastebin.mozilla.org/3173341

My log is telling me that rendering about:home is decoding thumbnails 16 times. That appears to be every page I've ever visited in this profile. And we OOM on every one.

Comment 5

[Richard Newman [:rnewman]]

I'm using an HTC One, which also should have no trouble with this.

This really ought to trigger low-memory cleanup, right?

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/home/TopSitesPage.java#l593

Comment 7

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Wow, this is fast.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Adding a short-circuit.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 812287 [details] [diff] [review]
Proposed patch. v2

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

Looks good.

::: mobile/android/base/home/TopSitesPage.java
@@ +594,2 @@
>              }
> +            Bundle bundle = new Bundle();

Newline before this line.

@@ +640,3 @@
>              try {
> +                while (cursor.moveToNext()) {
> +                    String url = cursor.getString(cursor.getColumnIndexOrThrow(Thumbnails.URL));

I like your approach of storing the index in a variable. Please use it here as well.

@@ +643,3 @@
>  
> +                    // This should never be null, but if it is...
> +                    final byte[] b = cursor.getBlob(cursor.getColumnIndexOrThrow(Thumbnails.DATA));

ditto.

@@ +646,5 @@
> +                    if (b == null) {
> +                        continue;
> +                    }
> +
> +                    final Bitmap bitmap = BitmapUtils.decodeByteArray(b);

We should probably be catching OOM in BitmapUtils. But I am fine with this approach too.

Comment 10

[Richard Newman [:rnewman]]

Gonna scope-creep this to fix a strictmode warning that seems to be wiping the first thumbnail shortly after redisplay. This might end up fixing Bug 922321, too.

Comment 11

[Richard Newman [:rnewman]]

I added some more commenting about the multi-load issue, and a couple of minor fixes.

https://hg.mozilla.org/integration/fx-team/rev/132dbcea6676

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/132dbcea6676

Comment 13

[Aaron Train [:aaronmt]]

Please get this on Aurora!

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 812287 [details] [diff] [review]
Proposed patch. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  about:home rewrite (fig).
 
User impact if declined: 
  Much slower rendering of about:home (including background OOMs) for users with more than six visited sites.

Testing completed (on m-c, etc.): 
  Just landed on m-c. Tested on own device. Waiting for some feedback from next Nightly.

Risk to taking this patch (and alternatives if risky): 
  Should be slim: trivial DB access improvement, safe logic cleanup, and limiting an iteration.

String or IDL/UUID changes made by this patch:
  None.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Richard Newman [:rnewman] from comment #14)

> Testing completed (on m-c, etc.): 
>   Just landed on m-c. Tested on own device. Waiting for some feedback from
> next Nightly.

Can you loop back on how the feedback results were after landing to Nightly before we consider this for uplift?

Comment 16

[Richard Newman [:rnewman]]

Have heard zero negative feedback and seen no problems in Nightly on my own device. No evidence of test instability. :thumbsup:

Comment 17

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e78a73708c7a