Last Comment Bug 714762 - ArrayIndexOutOfBoundsException at Tabs.getTabAt
: ArrayIndexOutOfBoundsException at Tabs.getTabAt
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: Firefox 12
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 06:06 PST by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-01-31 05:15 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch (697 bytes, patch)
2012-01-03 06:06 PST, Mark Finkle (:mfinkle) (use needinfo?)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 06:06:00 PST
Created attachment 585392 [details] [diff] [review]
patch

Found this in a Talos log:

01-02 22:37:00.567 E/GeckoApp( 1640): Exception handling message "DOMWindowClose":
01-02 22:37:00.567 E/GeckoApp( 1640): java.lang.ArrayIndexOutOfBoundsException
01-02 22:37:00.567 E/GeckoApp( 1640): 	at java.util.ArrayList.get(ArrayList.java:313)
01-02 22:37:00.567 E/GeckoApp( 1640): 	at org.mozilla.gecko.Tabs.getTabAt(Tabs.java:110)
01-02 22:37:00.567 E/GeckoApp( 1640): 	at org.mozilla.gecko.Tabs.getNextTab(Tabs.java:160)
01-02 22:37:00.567 E/GeckoApp( 1640): 	at org.mozilla.gecko.Tabs.closeTab(Tabs.java:139)
01-02 22:37:00.567 E/GeckoApp( 1640): 	at org.mozilla.gecko.GeckoApp.handleWindowClose(GeckoApp.java:1322)


We are checking the upper bound, but not the lower bound. This patch adds a check for the lower bound too.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-03 06:44:05 PST
Comment on attachment 585392 [details] [diff] [review]
patch

Not directly related to your fix, but doesn't this code also mean that if we get a DOMWindowClose on the only tab, then it won't actually close? getNextTab will return null and closeTab will bail out. That seems wrong; I would expect it to close.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 08:00:35 PST
(In reply to Kartikaya Gupta (:kats) from comment #1)
> Comment on attachment 585392 [details] [diff] [review]
> patch
> 
> Not directly related to your fix, but doesn't this code also mean that if we
> get a DOMWindowClose on the only tab, then it won't actually close?
> getNextTab will return null and closeTab will bail out. That seems wrong; I
> would expect it to close.

Right. We should not allow closing the only tab (I mean if there is only one tab). We never want the UI to be in a state where we have zero tabs. Same with the tabs in browser.js
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-03 08:28:49 PST
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Right. We should not allow closing the only tab (I mean if there is only one
> tab). We never want the UI to be in a state where we have zero tabs. Same
> with the tabs in browser.js

Why not? There are legitimate reasons to call window.close() from a website, and if it doesn't close the window I would consider that behaviour broken. If we have to close the last tab, we can replace it with a new tab that shows about:home or something to prevent the browser from hiding completely.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 08:34:50 PST
(In reply to Kartikaya Gupta (:kats) from comment #3)
> (In reply to Mark Finkle (:mfinkle) from comment #2)
> > Right. We should not allow closing the only tab (I mean if there is only one
> > tab). We never want the UI to be in a state where we have zero tabs. Same
> > with the tabs in browser.js
> 
> Why not? There are legitimate reasons to call window.close() from a website,
> and if it doesn't close the window I would consider that behaviour broken.
> If we have to close the last tab, we can replace it with a new tab that
> shows about:home or something to prevent the browser from hiding completely.

Yes, we could do something like clear the tab history and load "about:blank". My main point was, the entire application is designed to not allow the app to have zero tabs. We must have at least one tab. But yes, it could be an auto-opened, blank tab.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 09:23:40 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c669ea03c2
Comment 6 Marco Bonardo [::mak] 2012-01-04 04:44:04 PST
https://hg.mozilla.org/mozilla-central/rev/94c669ea03c2
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-06 06:22:41 PST
Comment on attachment 585392 [details] [diff] [review]
patch

[Approval Request Comment]
Code cleanup. No risk
Prevents a possible fatal exception
Comment 8 Alex Keybl [:akeybl] 2012-01-06 11:22:41 PST
Comment on attachment 585392 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-06 22:10:54 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/58b188a5513d
Comment 10 Cristian Nicolae (:xti) 2012-01-31 05:15:56 PST
Verified fixed on:

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a2)Gecko/20120130
Firefox/11.0a2 Fennec/11.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120130
Firefox/12.0a1 Fennec/12.0a1
Device: Samsung Galaxy S
OS: Android 2.2

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