787318 - Tabs thumbnails are not updated

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Feher]

Aurora 17.0a2 2012-08-31
Samsung Galaxy Tab (Android 3.1)

Steps to reproduce:
1. Load any page (Ex http://ftp.mozilla.org/pub/mozilla.org/mobile/)
2. In the same tab load another page. 
3. Open Tab menu.

Expected results:
The tab thumbnail is updated the correct background is displayed.

Actual results:
The tab thumbnail is not updated the same background is displayed.

Note:
The issue is not reproducible on other devices: Samsung Galaxy Tab 2 7.0 (4.0.4), HTC Desire Z (Android 2.3.3), Asus Transformer TF101 (Android 4.0.3)

Comment 1

[Aaron Train [:aaronmt]]

Works for me; same device and build, same tabs.

Comment 2

[Paul Feher]

Attachment: Log file

Comment 3

[Paul Feher]

The issue is still reproducible using
Aurora 17.0a2 2012-09-02
Samsung Galaxy Tab (Android 3.1)

Please see the video and the attached log file.
http://youtu.be/7tbM0u4HBIo

Comment 4

[Gian-Carlo Pascutto [:gcp]]

I'm seeing this bug and similar ones a lot on tablet, but not on the phone. Taking.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Typical scenario, opening reddit and letting it load, then going to the awesomebar and loading Anandtech shows this at the end of the log:

I/GeckoApp( 4607): Got message: Content:StateChange
I/GeckoApp( 4607): State - 786448
I/GeckoApp( 4607): Got a document stop
I/GeckoApp( 4607): Return 
I/GeckoApp( 4607): getAndProcessThumbnailForTab AnandTech
I/GeckoApp( 4607): handleThumbnailData AnandTech
I/GeckoApp( 4607): tab AnandTech shouldUpdateThumbnail: true
I/GeckoApp( 4607): processThumbnail AnandTech

So we seem to understand the need for updating the thumbnail, but TabsTray is still showing the old one. It did update the Tab title, though.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

The problem seems to be that Android is silently failing to replace the first image set by another Bitmap with RGB_565 config. If I add an explicit conversion to ARGB_8888 it starts working. Given that this has only been observed on Honeycomb, I'll make the workaround conditional.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Workaround for failing image update

Either ImageView is silently failing to update more than once from a RGB_565 bitmap, or there is some bug in bitmap caching. Forcing the format to RGB_888 works around the problem. Only use it where we have seen the problem so far.

Comment 8

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

Comment on attachment 676145 [details] [diff] [review]
Patch 1. Workaround for failing image update

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

::: mobile/android/base/Tab.java
@@ +184,5 @@
> +                        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB
> +                            && Build.VERSION.SDK_INT <= Build.VERSION_CODES.HONEYCOMB_MR2) {
> +                            // Bug 787318: required to work around a bug in ImageView
> +                            // or Bitmap caching on Honeycomb Tablets
> +                            Bitmap temp = b.copy(Bitmap.Config.ARGB_8888, false);

I think you need b.recycle() here to free the data.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>I think you need b.recycle() here to free the data

We can't do the recycle() there because the Bitmap being passed in is the per-tab-Thumbnail-Bitmap we keep alive the entire time the tab lives. We also can't easily free the copy we make because the code that actually deals with it treats it as a Drawable instead of a Bitmap.

A fix would likely be to change Tab.getThumbnail() to create a new Bitmap and recycle the old one (which I strongly suspect would make this bug disappear in the first place) - but that's a performance penalty on newer Androids that otherwise work fine.

recycle() is optional, so at worst the thumbnail pixel data will live a bit longer than it does now.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Alternate with recycling

I think this is the best we can do wrt recycling, though personally I'd be happy to take the first patch and let the GC deal with it, too.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Should probably add a comment like this:

// If the Bitmap this drawable points to is the per-tab one we use for Gecko screenshotting, leave it alone. If it is an ARGB_8888 copy we made here, we can recycle it before replacing it.

Comment 12

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

Comment on attachment 676184 [details] [diff] [review]
Patch 1. v2 Alternate with recycling

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

Might it be better to work around this bug by always drawing the images as 8888 for HC?

Also, is it the format of the image that matters or the fact that we're reusing the same bitmap every time?

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v3 More alternate, just reallocate the bitmap

Comment 14

[Gian-Carlo Pascutto [:gcp]]

>Might it be better to work around this bug by always drawing the images as 8888 
>for HC?

Not sure what you mean exactly.

>Also, is it the format of the image that matters or the fact that we're reusing 
>the same bitmap every time?

I checked closer and it's the Bitmap that we reuse and copy pixel data in (as comment 9 suspected). Anyway, this gave me an idea for an alternate patch, that I think is a lot better and cleaner.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b93a413268f7

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b93a413268f7

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 676540 [details] [diff] [review]
Patch 1. v3 More alternate, just reallocate the bitmap

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown. Has been there for a long time.
User impact if declined: Tab thumbnails won't update when surfing on a HC tablet.
Testing completed (on m-c, etc.): On m-c for a day.
Risk to taking this patch (and alternatives if risky): No impact outside HoneyComb. Worst thing would be a race condition, but that can't happen in theory. So looks low risk to me.

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 676540 [details] [diff] [review]
Patch 1. v3 More alternate, just reallocate the bitmap

Approving on aurora as it is a low risk fix impacting HC only

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Cristian Nicolae (:xti)]

I cannot reproduce this issue anymore on the latest Nightly and Aurora builds. The thumbnails for each opened page are changing as expected. Closing bug as verified fixed on:

Firefox 19.0a1 (2012-11-05)
Device: Galaxy S2
OS: Android 4.0.3

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Note that the original issue was reported on a Galaxy Tab running Honeycomb and the fix is Honeycomb-specific.

Comment 22

[Cristian Nicolae (:xti)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #21)
> Note that the original issue was reported on a Galaxy Tab running Honeycomb
> and the fix is Honeycomb-specific.

It was verified on the Galaxy Tab 10.1 (Android 3.1) too. I just wanted to be sure that's everything fine both on phones and tablets. Thank you for pointing this out