Closed Bug 776906 Opened 7 years ago Closed 7 years ago

Thumbnails are broken on Nexus 7

Categories

(Firefox for Android :: Toolbar, defect)

15 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox15 + verified
firefox16 --- verified
firefox17 --- verified
fennec 15+ ---

People

(Reporter: ibarlow, Assigned: kats)

References

Details

(Whiteboard: [jellybean])

Attachments

(4 files, 1 obsolete file)

Attached image screenshot
See attachment
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [jellybean]
Component: General → Graphics, Panning and Zooming
This should probably be fixed for 15 considering how popular these tablets are.
Sriram - I think you've looked at thumbnail issues in the past. Would you mind taking a look at this?
Assignee: nobody → sriram
Duplicate of this bug: 778266
Duplicate of this bug: 779375
QA in the MV-office has this device.
This also affects the tab thumbnail screenshots.
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)
^ This is a recent regression introduced yesterday. Tracked by bug 779826.
(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?
(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.
Assignee: sriram → bugmail.mozilla
tracking-fennec: ? → 15+
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #650964 - Flags: review?(blassey.bugs)
Attached patch Cleanup part 1Splinter Review
Some cleanup I found while looking at this
Attachment #650965 - Flags: review?(blassey.bugs)
Attached patch Cleanup part 2Splinter Review
Some more cleanup, this saves an unnecessary PNG conversion.
Attachment #650967 - Flags: review?(blassey.bugs)
Attached patch Patch (v2)Splinter Review
Oh, turns out gfxImageSurface has a CairoStatus() function that lets us check for failure. I added a check for that as well.
Attachment #650964 - Attachment is obsolete: true
Attachment #650964 - Flags: review?(blassey.bugs)
Attachment #650970 - Flags: review?(blassey.bugs)
Attachment #650965 - Flags: review?(blassey.bugs) → review+
Attachment #650967 - Flags: review?(blassey.bugs) → review+
Attachment #650970 - Flags: review?(blassey.bugs) → review+
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
Attachment #650970 - Flags: approval-mozilla-beta?
Attachment #650970 - Flags: approval-mozilla-aurora?
(Note: only the one patch needs uplifting. The other two patches are cleanup and don't need to be uplifted)
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.
Attachment #650970 - Flags: approval-mozilla-beta?
Attachment #650970 - Flags: approval-mozilla-beta+
Attachment #650970 - Flags: approval-mozilla-aurora?
Attachment #650970 - Flags: approval-mozilla-aurora+
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.
Keywords: qawanted
Keywords: qawanted
Not sure why I removed the keyword; yes, it would be nice to get this verified before release.
Keywords: qawanted
Verified with a Nexus 7 - 
Fx17 today's Nightly
Fx16 today's Auroraly 
Fx15 Beta 7
You need to log in before you can comment on or make changes to this bug.