803687 - Corrupt thumbnails after Fennec starts up with a page URL

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open Fennec
2) Open any page
3) Push home and kill Fennec
4) Reopen Fennec; the page from step 2 should load
5) Go to about:home
6) Open any page

The page in step 6 will have a corrupt thumbnail.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: screenshot of corrupt thumbnail

Comment 3

[Brian Nicholson (:bnicholson)]

Looks like this isn't limited to killing/restoring Fennec. Opening Fennec with some (non-about:home) page loaded first is sufficient.

Alternate STR:
1) Open Fennec from an external link
2) Go to about:home
3) Open any page

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

We only hit this if (sThumbnailWidth < 0) path in this one case. On subsequent hits, we use the cached value, which we're not rounding and which then causes a step mismatch between the buffer returned from Gecko and the Bitmap we make in Java.

Comment 5

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

Comment on attachment 673426 [details] [diff] [review]
Patch

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

Does this actually fix the corrupt thumbnail problem? I tried a similar patch locally earlier and saw it still happen, but it's possible I screwed up my test. Regardless the patch is good and at least takes care of bug 803690 if not this one.

Comment 6

[Wesley Johnston (:wesj)]

Fix works locally.

Comment 7

[Wesley Johnston (:wesj)]

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

Comment 8

[Brian Nicholson (:bnicholson)]

After pulling this change from inbound and rebuilding, I can still reproduce using the STR in comment 3.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. I tested one bug and not another.

We're smart about throwing away our buffer when the thumbnail size changes, but are keeping around the bitmap object and then trying to use our resized buffer with it.

Comment 11

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

Comment on attachment 674252 [details] [diff] [review]
Patch

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

r=me if you make getThumbnailBitmap() synchronized as well.

Comment 12

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

The above patch should also fix bug 802510.

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5da9c2f970b0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5da9c2f970b0

Comment 17

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

Do we still need to leave this bug open?

Comment 18

[Wesley Johnston (:wesj)]

Nope. Should be good now.

Comment 19

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

Comment on attachment 673426 [details] [diff] [review]
Patch

Un-obsoleting this patch as it did actually land and is a valid patch.