803791 - java.lang.IllegalArgumentException: Invalid size 0 at org.mozilla.gecko.mozglue.DirectBufferAllocator.allocate(DirectBufferAllocator.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

There's one crash in 19.0a1/20121019: bp-f5975657-043a-4757-97f3-562942121020.
It might be a regression from bug 779511.

java.lang.IllegalArgumentException: Invalid size 0
	at org.mozilla.gecko.mozglue.DirectBufferAllocator.allocate(DirectBufferAllocator.java:21)
	at org.mozilla.gecko.Tab.getThumbnailBuffer(Tab.java:161)
	at org.mozilla.gecko.GeckoApp.getAndProcessThumbnailForTab(GeckoApp.java:731)
	at org.mozilla.gecko.GeckoApp$14.run(GeckoApp.java:1222)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalArgumentException%3A+Invalid+size+0+at+org.mozilla.gecko.mozglue.DirectBufferAllocator.allocate%28DirectBufferAllocator.java%29

Comment 1

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

Bug 779511 affected the buffer de-allocation code paths, but shouldn't have touched the allocation code paths. I think this one is more likely a regression from bug 787765.

Comment 2

[Scoobidiver (away)]

There's another crash from a different user in 19.0a1/20121021 so it seems to be a recent regression.

Comment 3

[Robert Kaiser]

I encountered bp-d0a0ffad-a263-4891-bdf8-ea1652121101 when I entered about: in the location bar and pressed enter on the virtual keyboard.

Comment 4

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

Attachment: Catch exception

All the crashes in 20 and 21 seem to occur on gingerbread on startup, so I assume that we're setting the thumbnail width to zero because of some platform bug. Since it's such a low-volume crash now I can't really be bothered to dig deeper than this.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 704552 [details] [diff] [review]
Catch exception

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

::: mobile/android/base/ThumbnailHelper.java
@@ +107,3 @@
>  
>          int capacity = mWidth * mHeight * 2; // Multiply by 2 for 16bpp
>          if (mBuffer == null || mBuffer.capacity() != capacity) {

Can we just check capacity > 0 to avoid calling DirectBufferAllocator.allocate()? DirectBufferAllocator.java is our code and we are throwing the IllegalArgumentException because capacity == 0:

https://hg.mozilla.org/mozilla-central/file/70daa2f8de6a/mobile/android/base/mozglue/DirectBufferAllocator.java#l21

@@ +107,5 @@
>  
>          int capacity = mWidth * mHeight * 2; // Multiply by 2 for 16bpp
>          if (mBuffer == null || mBuffer.capacity() != capacity) {
>              if (mBuffer != null) {
>                  mBuffer = DirectBufferAllocator.free(mBuffer);

If you do add a capacity check and capacity == 0, should we still DirectBufferAllocator.free(mBuffer)?

Comment 6

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

(In reply to Chris Peterson (:cpeterson) from comment #5)
> Can we just check capacity > 0 to avoid calling
> DirectBufferAllocator.allocate()? DirectBufferAllocator.java is our code and
> we are throwing the IllegalArgumentException because capacity == 0:
> 
> https://hg.mozilla.org/mozilla-central/file/70daa2f8de6a/mobile/android/base/
> mozglue/DirectBufferAllocator.java#l21

We could but I don't see how it's any better. The capacity should never really be zero there so I feel it's more correct to let the exception get thrown than it is to check for zero, because that feels like it's masking the problem a bit more. And the exception happens very rarely, so in terms of efficiency it's better to catch the exception than to add a check that will almost always fail.

> 
> @@ +107,5 @@
> >  
> >          int capacity = mWidth * mHeight * 2; // Multiply by 2 for 16bpp
> >          if (mBuffer == null || mBuffer.capacity() != capacity) {
> >              if (mBuffer != null) {
> >                  mBuffer = DirectBufferAllocator.free(mBuffer);
> 
> If you do add a capacity check and capacity == 0, should we still
> DirectBufferAllocator.free(mBuffer)?

Either way when capacity == 0, we should end up with mBuffer as null, so it should never try to do the free.

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 704552 [details] [diff] [review]
Catch exception

OK

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9072000c5fae

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9072000c5fae

Comment 10

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

Comment on attachment 704552 [details] [diff] [review]
Catch exception

Requesting for beta based on IRC conversation with :bajaj.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): old code
User impact if declined: occasional crashes on startup. this is #14 topcrasher on beta right now.
Testing completed (on m-c, etc.): on m-c and aurora.
Risk to taking this patch (and alternatives if risky): really low-risk; the patch is small and low-impact and landed a while ago so it's well-baked
String or UUID changes made by this patch: none

Comment 11

[bhavana bajaj [:bajaj]]

Comment on attachment 704552 [details] [diff] [review]
Catch exception

 This crash signature has been rising in beta lately hence approving the low risk well baked patch on beta.

Request to land asap so that this can get into our beta which is going to build shortly . Thanks !

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/738ec64bd432