714762 - ArrayIndexOutOfBoundsException at Tabs.getTabAt

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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:[e-mail])]

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?)]

(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:[e-mail])]

(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?)]

(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?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94c669ea03c2

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/94c669ea03c2

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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]]

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

[Triage Comment]
Mobile only - approved for Aurora.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/58b188a5513d

Comment 10

[Cristian Nicolae (:xti)]

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