Closed Bug 931843 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

27 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- affected
firefox27 - fixed
firefox28 - fixed
fennec 27+ ---

People

(Reporter: aaronmt, Assigned: rnewman)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, regression, Whiteboard: [qa+])

Crash Data

Attachments

(4 files, 3 obsolete files)

Attached file 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)
Version: Firefox 26 → Firefox 27
Severity: normal → critical
Crash Signature: [@ java.lang.OutOfMemoryError: at android.graphics.BitmapFactory.nativeDecodeAsset(Native Method)]
Keywords: crash
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.
Regression window would definitely be useful here. We haven't done any significant change in the tabs tray recently.
Sriram forgot to comment here. Him and Chris Kitching were discussing on IRC that the signature is a mislead.
(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.
I am trying to understand the actual user impact, so if the signature is misleading could someone provide some info?
(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.
(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?
(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?
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.
Aaron are you able to reliably reproduce the issue on Fx27 ? Can we get a regression window in that case?
(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'.
Keywords: steps-wanted
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.
Assignee: nobody → rnewman
tracking-fennec: ? → 27+
Attached patch Part 1: bandaid fix. v1 (obsolete) — Splinter Review
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.
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.
Attachment #825611 - Flags: review?(bnicholson)
(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…
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[]
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.
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.
Status: NEW → ASSIGNED
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.
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.
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
Early experimentation suggests that switching TSGIV.mThumbnail to just be a boolean works fine, and will help with leakage. More tomorrow.
Blocks: fennec-mem
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.)
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 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?
(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.
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.
Same app looks great on xhdpi device, too, and image sizes are dramatically smaller, which seems to translate into moar speed.
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.
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.
Attachment #826111 - Flags: review?(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
Attachment #826111 - Flags: review?(bugmail.mozilla)
Attachment #826111 - Flags: review?(bnicholson)
Attachment #826111 - Flags: feedback+
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.
Attachment #826130 - Flags: review?
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 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.
Depends on: 933992
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.
Attachment #826111 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #826111 - Attachment is obsolete: true
Attachment #826190 - Flags: review?(bugmail.mozilla)
This seems to work fine, having banged on it pretty extensively.
Attachment #826206 - Flags: review?(bnicholson)
Attachment #826130 - Attachment is obsolete: true
Attachment #826130 - Flags: review?
I'm pretty sure there's one more small Part 4 here, but I'll get to that later.
This doesn't seem to have any negative effects, and one less reference to a big Bitmap is a win in my book.
Attachment #826259 - Flags: review?(bnicholson)
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
Attachment #826190 - Flags: review?(bugmail.mozilla) → review+
Blocks: 932054
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.
Attachment #826206 - Flags: review?(bnicholson) → review+
Attachment #826259 - Flags: review?(bnicholson) → review+
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?
Attachment #825611 - Flags: review?(bnicholson) → review-
(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!
Blocks: 934605
We don't know how to reproduce this, and it's not a topcrash. Given current data, no need to track from our side.
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.
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.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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
Attachment #825611 - Attachment is obsolete: true
Depends on: 935157
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.
Attachment #826190 - Flags: approval-mozilla-aurora?
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 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.
Attachment #826190 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to bhavana bajaj [:bajaj] from comment #54)
> (In reply to Robert Kaiser (:kairo@mozilla.com) 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.
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.
Crashes for Nightly 28:

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

(Looking for ones built after 2013-11-05; none so far.)
You need to log in before you can comment on or make changes to this bug.