931843 - java.lang.OutOfMemoryError @ android.graphics.BitmapFactory.nativeDecodeAsset in TabsAdapter.assignValues(TabsTray.java:259)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: logcat

E/GeckoAppShell(11013): java.lang.OutOfMemoryError
E/GeckoAppShell(11013): 	at android.graphics.BitmapFactory.nativeDecodeAsset(Native Method)
E/GeckoAppShell(11013): 	at android.graphics.BitmapFactory.decodeStream(BitmapFactory.java:515)
E/GeckoAppShell(11013): 	at android.graphics.BitmapFactory.decodeResourceStream(BitmapFactory.java:363)
E/GeckoAppShell(11013): 	at android.graphics.drawable.Drawable.createFromResourceStream(Drawable.java:835)
E/GeckoAppShell(11013): 	at android.content.res.Resources.loadDrawable(Resources.java:2986)
E/GeckoAppShell(11013): 	at android.content.res.Resources.getDrawable(Resources.java:1556)
E/GeckoAppShell(11013): 	at android.widget.ImageView.resolveUri(ImageView.java:646)
E/GeckoAppShell(11013): 	at android.widget.ImageView.setImageResource(ImageView.java:375)
E/GeckoAppShell(11013): 	at org.mozilla.gecko.TabsTray$TabsAdapter.assignValues(TabsTray.java:259)

Filing a bug against this as this happened on my Galaxy S4 with no other intensive applications running at all other than Nightly.

I had left my device idle for about twenty minutes, turned it on and got the application is not responding dialog.

E/dalvikvm-heap(11013): Out of memory on a 381904-byte allocation.
I/dalvikvm(11013): "main" prio=5 tid=1 RUNNABLE
I/dalvikvm(11013):   | group="main" sCount=0 dsCount=0 obj=0x417d2638 self=0x40ce3838
I/dalvikvm(11013):   | sysTid=11013 nice=0 sched=0/0 cgrp=apps handle=1075314684
I/dalvikvm(11013):   | state=R schedstat=( 0 0 0 ) utm=3181 stm=576 core=0
I/dalvikvm(11013):   at android.graphics.BitmapFactory.nativeDecodeAsset(Native Method)
I/dalvikvm(11013):   at android.graphics.BitmapFactory.decodeStream(BitmapFactory.java:515)
I/dalvikvm(11013):   at android.graphics.BitmapFactory.decodeResourceStream(BitmapFactory.java:363)
I/dalvikvm(11013):   at android.graphics.drawable.Drawable.createFromResourceStream(Drawable.java:835)
I/dalvikvm(11013):   at android.content.res.Resources.loadDrawable(Resources.java:2986)
I/dalvikvm(11013):   at android.content.res.Resources.getDrawable(Resources.java:1556)
I/dalvikvm(11013):   at android.widget.ImageView.resolveUri(ImageView.java:646)

Comment 1

[Aaron Train [:aaronmt]]

https://crash-stats.mozilla.com/report/list?product=FennecAndroid&version=FennecAndroid:27.0a1&query_search=signature&query_type=contains&reason_type=contains&date=2013-10-26&range_value=14&range_unit=days&hang_type=any&process_type=any&signature=java.lang.OutOfMemoryError%3A+at+android.graphics.BitmapFactory.nativeDecodeAsset%28Native+Method%29

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

There are crashes in 24 - 26 but the volume changed sharply up in 27. Marking 24 - 26 unaffected and 27 affected for the increased spike.

This does not seem specific to any one device. Nor do I expect URLs to be of value here.

Comment 3

[Lucas Rocha (:lucasr)]

Regression window would definitely be useful here. We haven't done any significant change in the tabs tray recently.

Comment 4

[Aaron Train [:aaronmt]]

Sriram forgot to comment here. Him and Chris Kitching were discussing on IRC that the signature is a mislead.

Comment 5

[Chris Kitching [:ckitching]]

(In reply to Aaron Train [:aaronmt] from comment #4)
> Sriram forgot to comment here. Him and Chris Kitching were discussing on IRC
> that the signature is a mislead.

Probably. So far as I can tell (I *think* Sriram agreed) the memory leak is somewhere else - this crash signature just happens to be the allocation that failed because of the leak elsewhere.
I think Aaron managed to reproduce an OOM under similar circumstances with a different signature, too.

I suggest taking a heap dump to find the actual memory leak empirically, as the information here doesn't seem to be that helpful in finding the problem analytically.

Comment 6

[alco]

I am trying to understand the actual user impact, so if the signature is misleading could someone provide some info?

Comment 7

[Chris Kitching [:ckitching]]

(In reply to Alexandros Mioglou from comment #6)
> I am trying to understand the actual user impact, so if the signature is
> misleading could someone provide some info?

If you leave Firefox with the tab panel open for a long time it may crash with an OOM exception.
So, that's the user impact - probably unlikely to be run into very often in practice, but still not great.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Chris Kitching [:ckitching] from comment #7)
> (In reply to Alexandros Mioglou from comment #6)
> > I am trying to understand the actual user impact, so if the signature is
> > misleading could someone provide some info?
> 
> If you leave Firefox with the tab panel open for a long time it may crash
> with an OOM exception.
> So, that's the user impact - probably unlikely to be run into very often in
> practice, but still not great.

Does the number of tabs you have matter in this case?

Comment 9

[Chris Kitching [:ckitching]]

(In reply to Lucas Rocha (:lucasr) from comment #8)
> Does the number of tabs you have matter in this case?

Based on what (I think) Aaron said on IRC (He's the one who's actually been able to reproduce this) - it explodes sooner if you have more tabs open.
I think he also mentioned that all his tabs were about:home.
You can perhaps extract the information from logbot on #mobile.

Perhaps needinfo him?

Comment 10

[Aaron Train [:aaronmt]]

I've been able to reproduce bug 932054 which I am running into which I thought would reproduce this. This one is as I mentioned in comment #0 no particular steps from what I recall. I had left my device idle with only a handful of open tabs and came back to the home-screen with the application has closed (correction from comment #0 where I mentioned not responding, I meant closed). Nothing else was running on my device at the time so this is concerning that we have a leak somewhere.

Comment 11

[bhavana bajaj [:bajaj]]

Aaron are you able to reliably reproduce the issue on Fx27 ? Can we get a regression window in that case?

Comment 12

[Aaron Train [:aaronmt]]

(In reply to bhavana bajaj [:bajaj] from comment #11)
> Aaron are you able to reliably reproduce the issue on Fx27 ? Can we get a
> regression window in that case?

No, as mentioned, I hit the issue randomly when my device was idle. The one comment in the crash reports mentions 'opening a new tab'.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

It's dangerous to label a version 'unaffected' just because it doesn't have a spike.  If there have been crashes on that version we want to know about it and see at a glance, so updating the status to reflect that.  We can continue to monitor the spike in 27 and make a tracking decision if there is more information about a potential regressing bug here.

Comment 14

[Richard Newman [:rnewman]]

Attachment: Part 1: bandaid fix. v1

Here's the first step: let's not actually die hard when we can't draw a thumbnail. Maybe we're milliseconds away from Android GCing.

Comment 15

[Richard Newman [:rnewman]]

I cannot reproduce this, or any other memory-related issue, on a shitty Motorola 2.3 device or my HTC One, with any one of current Nightly, current fx-team dev build, or an Aurora developer build. They never get above ~4MB, and show no evidence of leaks (including with an hprof dump), even with 14+ tabs open.

There are plenty of awful memory culprits, like the duplicate strings we have per open page, and the swollen system classes (e.g., Bug 933554), but nothing that indicates a leak in our code.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #2)

> This does not seem specific to any one device.

It does seem to be API 15+, though, unless that's just Nightly and Aurora version bias…

Comment 17

[Richard Newman [:rnewman]]

wat

One instance of "android.graphics.Bitmap" loaded by "<system class loader>" occupies 2,396,240 (23.00%) bytes. The memory is accumulated in one instance of "byte[]" loaded by "<system class loader>".

Keywords
android.graphics.Bitmap
byte[]

Comment 18

[Richard Newman [:rnewman]]

20:59:17 < rnewman> 10-31 20:58:05.419 I/GeckoWAT(26403): Setting ThumbnailHelper width to 519
20:59:24 < rnewman>     public void setThumbnailWidth(int width) {
20:59:24 < rnewman>         mPendingWidth.set(IntSize.nextPowerOfTwo(width));
20:59:27 < rnewman> => 1024
21:00:08 < rnewman> so because I have an xhdpi device, and my thumbnails need to be 519 wide, we're taking 2MB screenshots.

1024x585x4 = 2396160.

We shouldn't make such stupidly huge thumbnails.

Comment 19

[Richard Newman [:rnewman]]

If I change .nextPowerOfTwo to .largestPowerOfTwoLessThan, that 2MB drops to 600KB, and I can't really see a difference in quality.

Furthermore, we're only showing 519x??? bitmaps on-screen. They shouldn't be somehow maintaining a reference to a 1024x??? source bitmap. So that's two bugs.

Comment 20

[Richard Newman [:rnewman]]

Three possibilities for drawing smaller screenshots (XHDPI device):

Next lower power of 2 (512):
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/512%20%28lower%20power%29.png

Next lower even number (518):
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/518%20%28actual%20even%29.png

Exact width (519):
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/519%20%28actual%29.png

Next higher power of 2, scaled down (1024):
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/1024%20%28higher%20power%29.png


1024 is definitely sharper, but I don't think it's worth 1.5MB. And as a follow-up I'd rather win sharpness by cropping a larger virtual canvas than by scaling down a huge image.

Comment 21

[Richard Newman [:rnewman]]

So that's part 1. The other part is: we're taking this large screenshot, then we're assigning the Bitmap to TSGIV.mThumbnail, so even if it gets scaled down for display, it never gets deallocated.

If we insist on taking large thumbnails, we should crop and scale them *before* we use them, and *before* we write them into the DB.

Comment 22

[Richard Newman [:rnewman]]

And on a lower DPI device (259x148 thumbnails), trying out a slightly different approach (never scale, 'cos after all the size we render to shouldn't need scaling):

Higher power (512):
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Original.png

Next higher even number, cropping rather than scaling (chopping off one pixel):
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Native%20even%20no%20scale.png

Exact number, cropping:
https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Native%20odd%20no%20scale.png

Comment 23

[Richard Newman [:rnewman]]

Early experimentation suggests that switching TSGIV.mThumbnail to just be a boolean works fine, and will help with leakage. More tomorrow.

Comment 24

[Richard Newman [:rnewman]]

Re dimensions and landscape: on my Moto RAZR:

  Portrait: 243x138
  Landscape: 296x169
  Transitions between the two: 453x258 or 156x89 (beats me how this is used)

These are the actual view sizes.

(Using centering instead of scaling naturally shows a white border around each thumbnail in the landscape position.)

Comment 25

[Richard Newman [:rnewman]]

Further notes: TopSitesGridView sets the thumbnail width every time its measurements are recalculated. This is to say: if you're in landscape mode, the screenshots you take will (if the sizes transit a power of 2) be larger. And the screenshots are not, of course, recomputed on rotation.

Here are the widths we decide are best to capture as I rotate my phone a few times.

10-31 11:24:49.273 I/GeckoWAT(21339): Setting ThumbnailHelper width to 172
10-31 11:24:49.445 I/GeckoWAT(21339): Setting ThumbnailHelper width to 312
10-31 11:24:52.781 I/GeckoWAT(21339): Setting ThumbnailHelper width to 469
10-31 11:24:52.914 I/GeckoWAT(21339): Setting ThumbnailHelper width to 259
10-31 11:27:13.062 I/GeckoWAT(21339): Setting ThumbnailHelper width to 172
10-31 11:27:13.265 I/GeckoWAT(21339): Setting ThumbnailHelper width to 312
10-31 11:27:22.210 I/GeckoWAT(21339): Setting ThumbnailHelper width to 469
10-31 11:27:22.398 I/GeckoWAT(21339): Setting ThumbnailHelper width to 259

So these are a little larger than the views themselves.

It would seem like the correct behavior here is to compute a single value that makes sense for either supported orientation, and to stick with that -- this value is capture-related, not display-related.

And that value *should* be the width of the largest possible thumbnail view.

Comment 26

[Richard Newman [:rnewman]]

Current Aurora (so much data that it sometimes stalls rendering about:home):

https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Aurora.png

The equivalent rendering to precisely the image size we need (looks great!):

https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Exact%20image%20view%20size.png

Scaling up on rotation:

https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Rotate%20-scale%20up.png

Native size for thumbnails captured during rotation:

https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Rotate%20-native.png

Scaling down when rotating back to vertical:

https://dl.dropboxusercontent.com/u/3911373/Fennec/Thumbnails/Low%20DPI/Unrotate%20-scale%20down.png

Comment 27

[Brian Nicholson (:bnicholson)]

Comment on attachment 825611 [details] [diff] [review]
Part 1: bandaid fix. v1

I'd rather just crash; the crash reporter lets us know that there's a potential problem--which there is--so we won't silently eat this exception and overlook the bug. Putting it another way: would we even be looking into this memory issue now if there was no crash reported? My guess is no.

Also, how useful is this band-aid? If we're on the verge of OOMing and we prevent it in this one place, aren't we just going to end up OOMing somewhere else?

Comment 28

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #27)

> I'd rather just crash; the crash reporter lets us know that there's a
> potential problem--which there is--so we won't silently eat this exception
> and overlook the bug. Putting it another way: would we even be looking into
> this memory issue now if there was no crash reported? My guess is no.

I generally concur (most of the point for me in a band-aid patch is in case a better solution cannot be found, and it looks like I subsequently found a smoking gun), though I wish there were a middle ground: a crash is small comfort for a user.
 
> Also, how useful is this band-aid? If we're on the verge of OOMing and we
> prevent it in this one place, aren't we just going to end up OOMing
> somewhere else?

Bitmaps seem to be somewhat privileged in Android, for one thing: we might have too many bitmaps but still be otherwise able to function. For another, it's quite conceivable that a memory pressure notification would be fired soon after the exception, and simply getting past this allocation would get us back to a good state.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

You could catch the exception and report it through telemetry. We'd get the same data and the users wouldn't have to deal with crashes.

Comment 30

[Richard Newman [:rnewman]]

Same app looks great on xhdpi device, too, and image sizes are dramatically smaller, which seems to translate into moar speed.

Comment 31

[Richard Newman [:rnewman]]

Next problem reported by the memory explorer: with 5 open tabs holding four open pages and about:home, we have a 41MB heap.

This is largely due to a chain:

Favicons -> sLoadTasks => a ton of task instances, each of which is retaining references to mPageURL, mFaviconURL, etc., and... mListener, which is either a TwoLinePageRow or… a BrowserApp inner class that's retaining a reference to a Tab, which is holding a huge mThumbnailBitmap.

This is retaining 29MB of byte[] in 42 instances, according to the heap analyzer.

Gonna go diving into this, looking for a couple of things:

* Whether we have some non-static inner classes that ought to be static
* Whether we're not discarding load tasks as soon as we ought.

Comment 32

[Richard Newman [:rnewman]]

Attachment: Part 2: record smaller thumbnails.

This is essentially exactly what we do now (complete with using whatever the predicted size is for if we're now landscape or portrait), but we use the exact size of the destination image view, rather than ~4x that size.

Comment 33

[Brian Nicholson (:bnicholson)]

Comment on attachment 826111 [details] [diff] [review]
Part 2: record smaller thumbnails.

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

These changes look fine to me if they work, but I don't fully understand the reasoning behind the fix in bug 776906. In turn, I don't understand why bug 776906 would no longer be an issue -- did something change? Kats should probably take a look at this.

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +32,5 @@
>  
> +    private static final ScaleType SCALE_TYPE_FAVICON   = ScaleType.CENTER;
> +    private static final ScaleType SCALE_TYPE_RESOURCE  = ScaleType.CENTER;
> +    private static final ScaleType SCALE_TYPE_THUMBNAIL = ScaleType.CENTER_CROP;
> +    

Nit: whitespace

Comment 34

[Richard Newman [:rnewman]]

Attachment: Part 3: slim down OnFaviconLoadedListener implementations. v1

This makes all of these inner classes static, and employs weak references and some inversion/runtime lookup techniques to avoid holding on to objects just because we're waiting to fetch their favicons.

This seems to dramatically drop memory pressure on my device (40MB -> 12MB).

Next up: attacking Favicons, and getting rid of some retained bitmap references.

Comment 35

[Richard Newman [:rnewman]]

To verify the removal of the "always even size" code:

15:03:29 < kats> rnewman: any device with less than 786 MB should also do the trick. or you could just hard-code GeckoAppShell.getScreenDepth() to return false

Comment 36

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 826130 [details] [diff] [review]
Part 3: slim down OnFaviconLoadedListener implementations. v1

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

Drive-by:

::: mobile/android/base/BrowserApp.java
@@ +1360,5 @@
>          Tabs.getInstance().loadUrl(ABOUT_HOME, Tabs.LOADURL_READING_LIST);
>      }
>  
> +    /* Favicon stuff. */
> +    private static class TabsFaviconLoadedListener implements OnFaviconLoadedListener {

Why not create something like:

private static final OnFaviconLoadedListener sTabsFaviconLoadedListener = new OFFL { ... };

And, use this variable below. That way, there is just one instance of it. I don't see a need for multiple instances whenever loadFavicon() is called.

::: mobile/android/base/Tabs.java
@@ +281,5 @@
>  
>      public synchronized Tab getTab(int id) {
> +        if (id == -1)
> +            return null;
> +        

Whitespace :D

::: mobile/android/base/home/TwoLinePageRow.java
@@ +47,5 @@
> +        private final WeakReference<FaviconView> view;
> +        public UpdateViewFaviconLoadedListener(FaviconView view) {
> +            this.view = new WeakReference<FaviconView>(view);
> +        }
> +        

Some more whitespace. :D

@@ +66,5 @@
>          }
> +    }
> +
> +    // Listener for handling Favicon loads.
> +    private final OnFaviconLoadedListener mFaviconListener;

This is what I meant in first comment, preferably static.

Comment 37

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

Comment on attachment 826111 [details] [diff] [review]
Part 2: record smaller thumbnails.

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

If I understood your IRC comment correctly, the even-number requirement still exists if GeckoAppShell.getScreenDepth() returns 16 rather than 24. Minusing for that. However you should check with wesj as to why the thumbnails were rounded up to the next power of two to begin with; that code originated in bug 787765.

Comment 38

[Richard Newman [:rnewman]]

Attachment: Part 2: record smaller thumbnails. v2

I've verified that this works on my HTC One forcing a 16-bit screen depth, and without. Rotation works, and everything seems generally very robust.

Comment 39

[Richard Newman [:rnewman]]

Attachment: Part 3: slim down OnFaviconLoadedListener implementations. v2

This seems to work fine, having banged on it pretty extensively.

Comment 40

[Richard Newman [:rnewman]]

I'm pretty sure there's one more small Part 4 here, but I'll get to that later.

Comment 41

[Richard Newman [:rnewman]]

Attachment: Part 4: don't use a 500KB bitmap as a boolean. v1

This doesn't seem to have any negative effects, and one less reference to a big Bitmap is a win in my book.

Comment 42

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

Comment on attachment 826190 [details] [diff] [review]
Part 2: record smaller thumbnails. v2

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

r=me with comments addressed and the commit message un-qfolded. Although I'm probably not qualified to review the TopSitesGrid* code

::: mobile/android/base/ThumbnailHelper.java
@@ +95,5 @@
>          requestThumbnailFor(tab);
>      }
>  
>      public void setThumbnailWidth(int width) {
> +        if (GeckoAppShell.getScreenDepth() == 16) {

I would rather invert this condition, so set the width if getScreenDepth() == 24. See https://bugzilla.mozilla.org/show_bug.cgi?id=803299#c34 for the rationale.

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +32,5 @@
>  
> +    private static final ScaleType SCALE_TYPE_FAVICON   = ScaleType.CENTER;
> +    private static final ScaleType SCALE_TYPE_RESOURCE  = ScaleType.CENTER;
> +    private static final ScaleType SCALE_TYPE_THUMBNAIL = ScaleType.CENTER_CROP;
> +    

Trailing whitespace

Comment 43

[Richard Newman [:rnewman]]

Clean try build.

https://tbpl.mozilla.org/?tree=Try&rev=87f0eb8b8d57

Comment 44

[Brian Nicholson (:bnicholson)]

Comment on attachment 826206 [details] [diff] [review]
Part 3: slim down OnFaviconLoadedListener implementations. v2

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

::: mobile/android/base/BrowserApp.java
@@ +749,5 @@
>              }
> +
> +            final OnFaviconLoadedListener listener = new GeckoAppShell.CreateShortcutFaviconLoadedListener(url, title);
> +            Favicons.getFaviconForSize(url,
> +                                       tab.getFaviconURL(),

Not a regression from your patch, but an observation: looking at Tab#mFaviconUrl, it appears to only be updated on Link:Favicon. So I assume this means that whenever we create a shortcut (or call getFaviconURL in general), we won't show favicons for pages that use <domain>/favicon.ico?

::: mobile/android/base/Tabs.java
@@ +485,5 @@
> +        // favicon is finally loaded, in which case we won't find the tab.
> +        // See also: Bug 920331.
> +        for (Tab tab : mOrder) {
> +            String tabURL = tab.getURL();
> +            if (pageURL.equals(tabURL)) {

What if I go to an anchor during the favicon load (e.g., http://www.google.com/ -> http://www.google.com/#foo=bar)? It would be nice if we could match against Tab#mFaviconUrl instead of Tab#mUrl; that will likely need changes to how we handle mFaviconUrl, though, since it looks like it only gets updated on Link:Favicon as mentioned above. Alternatively, we could try some URL canonicalization.

Could you file a follow-up if you don't want to fix this now?

::: mobile/android/base/home/TwoLinePageRow.java
@@ -44,3 @@
>          @Override
>          public void onFaviconLoaded(String url, String faviconURL, Bitmap favicon) {
> -            setFaviconWithUrl(favicon, faviconURL);

Make sure to remove setFaviconWithUrl if this was the only place it was called.

Comment 45

[Brian Nicholson (:bnicholson)]

Comment on attachment 825611 [details] [diff] [review]
Part 1: bandaid fix. v1

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

No need for the band-aid since you've fixed the underlying issue, right? If you still want to have safety checks here, could you look into the catch-and-report suggestion by Brad in comment 29?

Comment 46

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #44)

> Not a regression from your patch, but an observation: looking at
> Tab#mFaviconUrl, it appears to only be updated on Link:Favicon. So I assume
> this means that whenever we create a shortcut (or call getFaviconURL in
> general), we won't show favicons for pages that use <domain>/favicon.ico?

getFaviconForSize tries the provided favicon URL, and then falls back to a guessed URL, which is almost always going to be the icon we fetched if they didn't link to one.

If that doesn't work, we spawn a LoadFaviconTask which falls back to the page URL cache and then the DB:

            // Try to get the favicon URL from the memory cache.
            storedFaviconUrl = Favicons.getFaviconURLForPageURLFromCache(mPageUrl);

            // If that failed, try to get the URL from the database.
            if (storedFaviconUrl == null) {
                storedFaviconUrl = Favicons.getFaviconUrlForPageUrl(mPageUrl);
                if (storedFaviconUrl != null) {
                    // If that succeeded, cache the URL loaded from the database in memory.
                    Favicons.putFaviconURLForPageURLInCache(mPageUrl, storedFaviconUrl);
                }
            }

I'll file a follow-up to evaluate whether tracking the favicon URL and using the 'short' path in more cases is worthwhile…


> What if I go to an anchor during the favicon load (e.g.,
> http://www.google.com/ -> http://www.google.com/#foo=bar)? It would be nice
> if we could match against Tab#mFaviconUrl instead of Tab#mUrl; that will
> likely need changes to how we handle mFaviconUrl, though, since it looks
> like it only gets updated on Link:Favicon as mentioned above. Alternatively,
> we could try some URL canonicalization.

Yeah, this is another manifestation of an existing bug: see the "see also" comment (Bug 920331). Basically we should be comparing the favicon URL if it's set, and if not (and thus there's no directly linked icon) we should be comparing the URL more intelligently (same page modulo favicons and query params? -- 'equal').

Will leave another comment in that bug.


> Make sure to remove setFaviconWithUrl if this was the only place it was
> called.

Done!

Comment 47

[Richard Newman [:rnewman]]

Arranged into four beautiful and unique parts.

   https://hg.mozilla.org/integration/fx-team/rev/e90bea5b1f4d
   https://hg.mozilla.org/integration/fx-team/rev/293e2577e8aa
   https://hg.mozilla.org/integration/fx-team/rev/67b72c48f247
   https://hg.mozilla.org/integration/fx-team/rev/c7596e96c6e0

Comment 48

[Alex Keybl [:akeybl]]

We don't know how to reproduce this, and it's not a topcrash. Given current data, no need to track from our side.

Comment 49

[Richard Newman [:rnewman]]

Alex: FYI, I think we plan to uplift this, at least to 27. The memory gains are gigantic; I wouldn't be surprised if this closed a big swath of our OOM crashes.

I can certainly trivially reproduce a 40MB heap on my own device -- it's just hard to make a modern 2GB phone OOM reliably.

Comment 50

[Robert Kaiser]

I'm tempted to re-request tracking for comment #49, but maybe it's enough if you just request approval once you have verified that it sticks on m-c.

Comment 51

[Richard Newman [:rnewman]]

(In reply to Robert Kaiser (:[e-mail]) from comment #50)
> I'm tempted to re-request tracking for comment #49, but maybe it's enough if
> you just request approval once you have verified that it sticks on m-c.

Meh tracking, yay approval :P

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e90bea5b1f4d
https://hg.mozilla.org/mozilla-central/rev/293e2577e8aa
https://hg.mozilla.org/mozilla-central/rev/67b72c48f247
https://hg.mozilla.org/mozilla-central/rev/c7596e96c6e0

Comment 53

[Richard Newman [:rnewman]]

Comment on attachment 826190 [details] [diff] [review]
Part 2: record smaller thumbnails. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Various over time.

User impact if declined: 
  Worse performance, dramatically larger memory usage, particularly on hi-DPI devices, particularly with multiple tabs open.

Testing completed (on m-c, etc.): 
  Been baking in Nightly for a while. Hand-tested in a number of scenarios, including QA.

Risk to taking this patch (and alternatives if risky): 
  Areas of risk:
    * We touch thumbnailing code, possibly in ways that will impact devices with 16-bit displays.
    * It's not a totally trivial piece of work, so there could be bugs (as always). I'm fairly confident that we've flushed them on Nightly, though (see follow-up, already marked as a+).

  There's no real alternative to this work, though.

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

Comment 54

[bhavana bajaj [:bajaj]]

(In reply to Robert Kaiser (:[e-mail]) from comment #50)
> I'm tempted to re-request tracking for comment #49, but maybe it's enough if
> you just request approval once you have verified that it sticks on m-c.

:Kairo, Is there a way to confirm the uplift has helped ?

Comment 55

[bhavana bajaj [:bajaj]]

Comment on attachment 826190 [details] [diff] [review]
Part 2: record smaller thumbnails. v2

Approving as this is a memory and perf win especially on lower end devices. In the areas that are called out as risky, :rnewman can you please request qa testing(cc :kbrosnan) to catch fallout's sooner than later.

Comment 56

[Robert Kaiser]

(In reply to bhavana bajaj [:bajaj] from comment #54)
> (In reply to Robert Kaiser (:[e-mail]) from comment #50)
> > I'm tempted to re-request tracking for comment #49, but maybe it's enough if
> > you just request approval once you have verified that it sticks on m-c.
> 
> :Kairo, Is there a way to confirm the uplift has helped ?

On this particular signature, if we have seen it on Aurora, then that's surely verifiable. On the larger scale, I guess we can look at rates over time, but it might be hard to give a clear answer unless the effect is tremendous in stats.

Comment 57

[Robert Kaiser]

https://crash-stats.mozilla.com/report/list?product=FennecAndroid&signature=java.lang.OutOfMemoryError%3A+at+android.graphics.BitmapFactory.nativeDecodeAsset(Native+Method) says the signature is hitting all channels right now, so we should be at least verify the particular signature going away. On the larger effect, that might take a while until we even have enough data that we might be able to see anything, esp. due to the low ADI counts on Nightly and Aurora.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/132cf341abb6
https://hg.mozilla.org/releases/mozilla-aurora/rev/51b568709bb3
https://hg.mozilla.org/releases/mozilla-aurora/rev/644c588e3e36
https://hg.mozilla.org/releases/mozilla-aurora/rev/5437dcd8200e

Leaving status-firefox26 set to affected pending a decision on uplifting to beta. Please set it to wontfix if that decision is no.

Comment 59

[Richard Newman [:rnewman]]

Crashes for Nightly 28:

http://cow.org/r/9m6

(Looking for ones built after 2013-11-05; none so far.)