Closed Bug 714762 Opened 8 years ago Closed 8 years ago

ArrayIndexOutOfBoundsException at Tabs.getTabAt

Categories

(Firefox for Android :: General, defect)

x86
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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.
Attachment #585392 - Flags: review?(bugmail.mozilla)
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.
Attachment #585392 - Flags: review?(bugmail.mozilla) → review+
(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
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/94c669ea03c2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Assignee: nobody → mark.finkle
Comment on attachment 585392 [details] [diff] [review]
patch

[Approval Request Comment]
Code cleanup. No risk
Prevents a possible fatal exception
Attachment #585392 - Flags: approval-mozilla-aurora?
Comment on attachment 585392 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #585392 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.