Closed Bug 803791 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
critical

Tracking

(firefox19 affected, firefox20 fixed, firefox21 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox19 --- affected
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: scoobidiver, Assigned: kats)

Details

(Keywords: crash, Whiteboard: [native-crash])

Crash Data

Attachments

(1 file)

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
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.
There's another crash from a different user in 19.0a1/20121021 so it seems to be a recent regression.
Crash Signature: [@ java.lang.IllegalArgumentException: Invalid size 0 at org.mozilla.gecko.mozglue.DirectBufferAllocator.allocate(DirectBufferAllocator.java)] → [@ java.lang.IllegalArgumentException: Invalid size 0 at org.mozilla.gecko.mozglue.DirectBufferAllocator.allocate(DirectBufferAllocator.java) ]
I encountered bp-d0a0ffad-a263-4891-bdf8-ea1652121101 when I entered about: in the location bar and pressed enter on the virtual keyboard.
Attached patch Catch exceptionSplinter Review
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.
Attachment #704552 - Flags: review?(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)?
Attachment #704552 - Flags: review?(cpeterson) → review-
(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 on attachment 704552 [details] [diff] [review]
Catch exception

OK
Attachment #704552 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/9072000c5fae
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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
Attachment #704552 - Flags: approval-mozilla-beta?
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 !
Attachment #704552 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.