Last Comment Bug 766865 - java.lang.IndexOutOfBoundsException: Invalid index <n>, size is <n> at java.util.ArrayList.throwIndexOutOfBoundsException(ArrayList.java) at org.mozilla.gecko.TabsTray$TabsAdapter.addTab(TabsTray.java)
: java.lang.IndexOutOfBoundsException: Invalid index <n>, size is <n> at java.u...
Status: RESOLVED FIXED
[native-crash]
: crash, regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 16 Branch
: ARM Android
: -- critical (vote)
: Firefox 16
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
Depends on:
Blocks: 765805
  Show dependency treegraph
 
Reported: 2012-06-20 23:42 PDT by Scoobidiver (away)
Modified: 2012-06-28 15:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (1.05 KB, patch)
2012-06-21 12:05 PDT, Matt Brubeck (:mbrubeck)
margaret.leibovic: review-
Details | Diff | Splinter Review
patch v2 (1.04 KB, patch)
2012-06-21 14:04 PDT, Matt Brubeck (:mbrubeck)
margaret.leibovic: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-06-20 23:42:33 PDT
There's one crash in 16.0a1/20120620: bp-f0dca13d-e00a-4618-858f-22afc2120620.

java.lang.IndexOutOfBoundsException: Invalid index -1, size is 4
	at java.util.ArrayList.throwIndexOutOfBoundsException(ArrayList.java:255)
	at java.util.ArrayList.add(ArrayList.java:147)
	at org.mozilla.gecko.TabsTray$TabsAdapter.addTab(TabsTray.java:224)
	at org.mozilla.gecko.TabsTray.onTabChanged(TabsTray.java:130)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:355)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:346)
	at org.mozilla.gecko.Tabs$1.run(Tabs.java:69)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:132)
	at android.app.ActivityThread.main(ActivityThread.java:4123)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:491)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:841)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:599)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/query/query?product=FennecAndroid&query_search=signature&query_type=contains&query=java.util.ArrayList.throwIndexOutOfBoundsException&do_query=1
Comment 1 :Margaret Leibovic 2012-06-21 11:10:10 PDT
This looks like a regression caused by http://hg.mozilla.org/mozilla-central/rev/b7d5c0ed0d26.
Comment 2 Matt Brubeck (:mbrubeck) 2012-06-21 12:05:20 PDT
Created attachment 635405 [details] [diff] [review]
patch

It looks like this can happen if a tab is added but then removed immediately, before the ADDED notification is processed.

It might also be possible for the index to be too large, if some other tabs was closed right after this one was added, before the notification is processed.  This patch checks for that scenario too.
Comment 3 :Margaret Leibovic 2012-06-21 12:26:35 PDT
Comment on attachment 635405 [details] [diff] [review]
patch

(In reply to Matt Brubeck (:mbrubeck) from comment #2)

> It might also be possible for the index to be too large, if some other tabs
> was closed right after this one was added, before the notification is
> processed.  This patch checks for that scenario too.

If some other tab was closed but this one was added, I believe we'd still want to be adding this new tab to the TabsAdapter. We could just make sure to add the tab at the end in this case, but this makes me worry that it's also possible to be inserting tabs at the wrong index (e.g, you have multiple tabs, we add a tab somewhere in the middle, and the first tab gets deleted before the ADDED notification gets processed).

It sounds to me like we need a more robust way to make sure we're adding the tab to the right place in TabsAdapter, but I'm not certain of a good approach. Perhaps add the index to the data passed along with the TabEvents.ADDED notification, but this still wouldn't help with races with CLOSED events. Maybe this is just something we can look into more when the adapter updates from bug 757198 finally land.

r- for now to make sure we add the new tab if it does indeed exist. We can just adjust the index to add it at the end if the index is too large.
Comment 4 Matt Brubeck (:mbrubeck) 2012-06-21 14:04:59 PDT
Created attachment 635453 [details] [diff] [review]
patch v2

(In reply to Margaret Leibovic [:margaret] from comment #3)
> r- for now to make sure we add the new tab if it does indeed exist. We can
> just adjust the index to add it at the end if the index is too large.

Fixed in this version.
Comment 5 Matt Brubeck (:mbrubeck) 2012-06-21 14:50:54 PDT
Comment on attachment 635453 [details] [diff] [review]
patch v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/039721acef47

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 765805
User impact if declined: crash when tabs open and close rapidly
Testing completed (on m-c, etc.): Landed on inbound June 21

Risk to taking this patch (and alternatives if risky): Low-risk mobile patch that just adds a bounds-check before an array insertion.

String or UUID changes made by this patch: None.
Comment 6 Ed Morley [:emorley] 2012-06-22 03:44:52 PDT
https://hg.mozilla.org/mozilla-central/rev/039721acef47
Comment 7 Alex Keybl [:akeybl] 2012-06-24 13:18:38 PDT
Comment on attachment 635453 [details] [diff] [review]
patch v2

[Triage Comment]
Low risk crash regression fix. Approved for Aurora 15.
Comment 8 Matt Brubeck (:mbrubeck) 2012-06-28 15:40:11 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/963b293cd4a6

Note You need to log in before you can comment on or make changes to this bug.