776906 - Thumbnails are broken on Nexus 7

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: screenshot

See attachment

Comment 1

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

This should probably be fixed for 15 considering how popular these tablets are.

Comment 2

[Alex Keybl [:akeybl]]

Sriram - I think you've looked at thumbnail issues in the past. Would you mind taking a look at this?

Comment 5

[Aaron Train [:aaronmt]]

QA in the MV-office has this device.

Comment 6

[Aaron Train [:aaronmt]]

This also affects the tab thumbnail screenshots.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm seeing:

>>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 560 ("GeckoBackgroundThread")
java.lang.IllegalArgumentException: Invalid size 37286
         at org.mozilla.gecko.mozglue.DirectBufferAllocator.allocate(DirectBufferAllocator.java:21)
         at org.mozilla.gecko.Tab.getThumbnailBuffer(Tab.java:147)
         at org.mozilla.gecko.GeckoApp.getAndProcessThumbnailForTab(GeckoApp.java:629)
         at org.mozilla.gecko.GeckoApp$21.run(GeckoApp.java:1293)
         at android.os.Handler.handleCallback(Handler.java:615)
         at android.os.Handler.dispatchMessage(Handler.java:92)
         at android.os.Looper.loop(Looper.java:137)
         at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)
I/GeckoScreenshot(26907): rect: 0.000000, 0.000000, 980.000000, 481.583313
D/Zygote  (  124): Process 26907 terminated by signal (11)

Comment 8

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

^ This is a recent regression introduced yesterday. Tracked by bug 779826.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Kartikaya Gupta (:kats) from comment #8)
> ^ This is a recent regression introduced yesterday. Tracked by bug 779826.

Does this mean we should be looking to bug 779826 for uplift to deal with the regression on Beta?

Comment 10

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

(In reply to Lukas Blakk [:lsblakk] from comment #9)
> (In reply to Kartikaya Gupta (:kats) from comment #8)
> > ^ This is a recent regression introduced yesterday. Tracked by bug 779826.
> 
> Does this mean we should be looking to bug 779826 for uplift to deal with
> the regression on Beta?

No. The stack trace in comment #7 and my reply in comment 8 referred to m-c, not beta. It is unrelated to the broken thumbnails.

Comment 11

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

Attachment: Patch

This fixes the problem, but I don't know if there's a better solution. The problem is that on the N7, the thumbnail width is 181 pixels, which ends up creating a gfxImageSurface with stride length 362 to draw the thumbnail. gfxImageSurface::InitWithData calls cairo_image_surface_create_for_data which decides 362 is not aligned enough to be useful, and returns a nil surface with the CAIRO_STATUS_INVALID_STRIDE error code. All the drawing ends up being a no-op, no bytes are drawn into the thumbnail buffer, and the thumbnail ends up just being garbage data.

I would like to be able to detect this sort of problem more generally, but I don't think gfxImageSurface exposes error conditions from cairo_image_surface_create_for_data, so that should be fixed. Additionally, this patch only fixes it for the thumbnail case. It shouldn't happen for the low-res screenshot case because IIRC the buffer width is always a power of 2 for that, but if we ever use the TakeScreenshot code for anything else it might be a problem there too.

Comment 12

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

Attachment: Cleanup part 1

Some cleanup I found while looking at this

Comment 13

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

Attachment: Cleanup part 2

Some more cleanup, this saves an unnecessary PNG conversion.

Comment 14

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

Attachment: Patch (v2)

Oh, turns out gfxImageSurface has a CairoStatus() function that lets us check for failure. I added a check for that as well.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/82d357c6f409
https://hg.mozilla.org/integration/mozilla-inbound/rev/4599d8ff535c
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeda05c44f52

Comment 16

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

Comment on attachment 650970 [details] [diff] [review]
Patch (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: Thumbnails of tabs don't show up on the Nexus 7
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): This patch won't apply cleanly on aurora or beta, so there might be errors introduced during rebase. If so, there may be regressions in the thumbnail appearance. Affects mobile only.
String or UUID changes made by this patch: none

Comment 17

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

(Note: only the one patch needs uplifting. The other two patches are cleanup and don't need to be uplifted)

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/82d357c6f409
https://hg.mozilla.org/mozilla-central/rev/4599d8ff535c
https://hg.mozilla.org/mozilla-central/rev/aeda05c44f52

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 650970 [details] [diff] [review]
Patch (v2)

This is late in the cycle for a potential "rebase issues" patch, but it is mobile only and a pretty serious user-facing issue we're better not shipping with - approving for branches.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

adding qawanted to verify this is fixed once it lands on branches since we won't have a ton of time with a beta audience on this one.

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c236df02c24f
https://hg.mozilla.org/releases/mozilla-beta/rev/2df27b18881c

Comment 22

[Aaron Train [:aaronmt]]

Not sure why I removed the keyword; yes, it would be nice to get this verified before release.

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

Verified with a Nexus 7 - 
Fx17 today's Nightly
Fx16 today's Auroraly 
Fx15 Beta 7