ArrayIndexOutOfBoundsException at Tabs.getTabAt

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

unspecified
Firefox 12
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed)

Details

Attachments

(1 attachment)

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.
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/integration/mozilla-inbound/rev/94c669ea03c2
https://hg.mozilla.org/mozilla-central/rev/94c669ea03c2
Status: NEW → RESOLVED
Last Resolved: 6 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 8

6 years ago
Comment on attachment 585392 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #585392 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/58b188a5513d
status-firefox11: --- → fixed
status-firefox12: --- → fixed
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.