765463 - java.lang.IllegalArgumentException: at java.nio.Buffer.limit(Buffer.java) or java.nio.Buffer.limitImpl(Buffer.java) or java.nio.Buffer.position(Buffer.java) or java.nio.Buffer.positionImpl(Buffer.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

It first appeared in 16.0a1/20120615144113 and there are currently 17 crashes.
The regression window is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=da8c6039c25e&tochange=4e3362864fbd
It's likely a regression from bug 755070.

java.lang.IllegalArgumentException
	at java.nio.Buffer.limit(Buffer.java:251)
	at org.mozilla.gecko.gfx.ScreenshotLayer$ScreenshotImage.copyBuffer(ScreenshotLayer.java:137)
	at org.mozilla.gecko.gfx.ScreenshotLayer$ScreenshotImage.setBitmap(ScreenshotLayer.java:144)
	at org.mozilla.gecko.gfx.ScreenshotLayer.setBitmap(ScreenshotLayer.java:53)
	at org.mozilla.gecko.gfx.LayerRenderer.setCheckerboardBitmap(LayerRenderer.java:138)
	at org.mozilla.gecko.ScreenshotHandler$1.run(GeckoAppShell.java:2338)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:123)
	at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

More reports at:
https://crash-stats.mozilla.com/query/query?product=FennecAndroid&version=ALL%3AALL&range_value=1&range_unit=weeks&query_search=signature&query_type=contains&query=java.nio.Buffer&do_query=1

Comment 1

[Martijn Wargers (dead)]

I can reproduce this on my Galaxy Nexus in current trunk build when visiting https://www.ziggo.nl/producten/alles-in-1/
I have to have set the uagent (with Phony) to phone Android, though.

Comment 2

[Martijn Wargers (dead)]

Hmm, and now I can't reproduce anymore.

Comment 3

[Cristian Nicolae (:xti)]

This crash has occurred on the latest Nightly build while I was trying to use the Google Vkb. I will try to find some certain steps for it:

https://crash-stats.mozilla.com/report/index/bp-8a9cb080-f141-479b-8035-940832120618

--
Firefox 16.0a1 (2012-06-18)
Device: Galaxy Nexus
OS: Android 4.0.2

Comment 4

[Aaron Train [:aaronmt]]

STR:

1. www.ups.com, pick your country
2. Tap into the top left package tracker, the virtual-keyboard is invoked

Leave device idle; track dozens of GeckoScreenshots in log-cat. Fennec eventually crashes

Comment 5

[Cristian Nicolae (:xti)]

Steps to reproduce:
1. Open Fennec
2. Go to google.com
3. Tap on Images from the top menu
4. Tap on Google's vkb button from the search input field
5. Insert a couple of chars from it and wait

Expected result:
No crash occurs after step 5.

Actual result:
This crash occurs after step 5. Here are some reports:

https://crash-stats.mozilla.com/report/index/bp-084b2cb6-5bca-4ded-afe2-dfe932120618
https://crash-stats.mozilla.com/report/index/bp-20f8fd34-e2c7-49a3-842b-cf99c2120618
https://crash-stats.mozilla.com/report/index/bp-bde4fb7b-89b8-4b77-9a0a-dbf482120618

Comment 6

[Aaron Train [:aaronmt]]

(In reply to Cristian Nicolae (:xti) from comment #5)
> https://crash-stats.mozilla.com/report/index/bp-084b2cb6-5bca-4ded-afe2-
> dfe932120618
> https://crash-stats.mozilla.com/report/index/bp-20f8fd34-e2c7-49a3-842b-
> cf99c2120618
> https://crash-stats.mozilla.com/report/index/bp-bde4fb7b-89b8-4b77-9a0a-
> dbf482120618

bug 765712

Comment 7

[Cristian Nicolae (:xti)]

Simpler STR:

1. Go to google.com (classic version)
2. Put the app in background
3. Go to Android settings and wait

Note:
First, it will take some time until the first crash, but then, those crashes will be more and more frequently until there will be a crash after each 10s or less.

Comment 8

[Chris Peterson [:cpeterson]]

blassey, this Fennec 16 topcrash looks like fallout from the new screenshot code (bug 755070).

Comment 10

[Martijn Wargers (dead)]

http://www.neowin.net also seems to cause this crash after a while.

Comment 11

[alex_mayorga]

Dropping my crash here as I might have STR bp-2303d363-45f2-45ca-8fca-c24842120621

Comment 12

[alex_mayorga]

bp-b43291c5-68d7-4d3d-bb80-fc4402120624

Crashing while login or ordering from www.atumesa.com from an Xperia pro.

Comment 13

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

Attachment: patch

not sure why I did (rect.bottom - 1) here

Comment 14

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

Comment on attachment 636206 [details] [diff] [review]
patch

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

I don't think this is right. The code in AndroidBridge::TakeScreenshot calls notifyScreenShot with the parameters (dstX, dstY, dstX + dstW, dstY + dstH) which end up becoming the left, top, right, and bottom of the rect. If dstX = 0, dstY = 0, dstW = 10, and dstH = 10, then rect.right + rect.bottom * stride = 10 + 10 * 10 = 110, which exceeds the 100 pixels (dstW * dstH) that were actually painted. That's why I suggested using rect.bottom - 1, but I guess that's not right either for some cases.

Comment 15

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

Attachment: Slightly tested patch

buffer.left and buffer.right need to be multiplied by 2 as well to account for 16bpp. This version should in theory never throw an exception because of the clamping. If things don't screenshot fully then there are errors elsewhere (probably in the slicing code, as mentioned on IRC).

Comment 16

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

Comment on attachment 636319 [details] [diff] [review]
Slightly tested patch

This prevents the crash using the STR AaronMT provided (go to ups.com and pick a country if needed, put focus in the tracking id textbox, and wait).

The crash is prevented by the clamping, since the rect is still (0, 0, 0, 0) and end is still calculated to be 1024.

Comment 17

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

Also I see scheduleCheckerboardScreenshotEvent getting called with these parameters when this happens:
sx = 18, sy = 135, sw = 1, sh = 15, dx = 9, dy = 239, dw = 0, dh = 26

Comment 18

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

Attachment: Patch

This one also kills the useless screenshotting when the dest area has zero width or height.

Comment 19

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

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

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

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +129,5 @@
>                  super.finalize();
>              }
>          }
>  
> +        void copyBuffer(ByteBuffer src, ByteBuffer dst, Rect rect, int bufferWidth) {

r- for this change, just pass the stride

@@ +134,5 @@
> +            int start = (rect.left + rect.top * bufferWidth) * 2; // 2 for 16bpp
> +            int end = (rect.right + (rect.bottom - 1) * bufferWidth) * 2; // 2 for 16bpp
> +            // clamp stuff just to be safe
> +            start = Math.max(0, Math.min(dst.limit(), Math.min(src.limit(), start)));
> +            end = Math.max(start, Math.min(dst.limit(), Math.min(src.limit(), end)));

I think you actually want to clamp to src.capacity() here, since you'll be setting the src buffer's limit below and we only need to make sure that that call is valid.

Comment 20

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

Attachment: Patch (v2)

Comment 21

[Marco Zehe (:MarcoZ)]

Since the June 27 build, accessibility's Explore By Touch also triggers this bug. STR:

1. With ICS, TalkBack and Explore By Touch enabled in Accessibility settings, go to http://www.marcozehe.de.
2. Start sliding your finger down on the right-hand side until you reach the "Quick navigation keys now in Firefox for Android nightly builds" heading.
3. Slide down just a little bit more.

Result: Crash: bp-638e5071-5049-4766-9511-425a92120627

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa4b9a0d764

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7fa4b9a0d764