773177 - java.lang.NullPointerException: at org.mozilla.gecko.TabsTray$TabsAdapter.refreshTabsData(TabsTray.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

There are two crashes from two users in 16.0a1/20120711. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=02b26fb307b4&tochange=53860f11100c
It's likely a regression from bug 757198.

java.lang.NullPointerException
	at org.mozilla.gecko.TabsTray$TabsAdapter.refreshTabsData(TabsTray.java:196)
	at org.mozilla.gecko.TabsTray$TabsAdapter.access$200(TabsTray.java:144)
	at org.mozilla.gecko.TabsTray.show(TabsTray.java:113)
	at org.mozilla.gecko.TabsPanel.show(TabsPanel.java:148)
	at org.mozilla.gecko.BrowserApp.showTabs(BrowserApp.java:328)
	at org.mozilla.gecko.BrowserApp.showLocalTabs(BrowserApp.java:317)
	at org.mozilla.gecko.BrowserToolbar.toggleTabs(BrowserToolbar.java:347)
	at org.mozilla.gecko.BrowserToolbar.access$100(BrowserToolbar.java:51)
	at org.mozilla.gecko.BrowserToolbar$3.onClick(BrowserToolbar.java:151)
	at android.view.View.performClick(View.java:2585)
	at android.view.View$PerformClick.run(View.java:9299)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3691)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:912)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:670)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.TabsTray%24TabsAdapter.refreshTabsData%28TabsTray.java%29

Comment 1

[:Margaret Leibovic]

Attachment: patch

There used to be a null check where we were doing this in the TabsAdapter constructor:
https://hg.mozilla.org/mozilla-central/rev/90e6b6c68e63#l1.140

I omitted it because I didn't think it was necessary, but it looks like it is. I didn't think we could get into a state with 0 tabs, but it looks like we must be.

Alternately, we could update Tabs.getTabsInOrder() (and Tabs.getTabs()) to just return an empty array instead of returning null:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#209

We're probably already relying on that and doing null checks, but returning null like that seems like a foot-gun.

Comment 2

[:Margaret Leibovic]

Attachment: patch

Whoops, need to make sure we're still doing the other stuff in this method.

Comment 3

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

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

># HG changeset patch
># User Margaret Leibovic <[e-mail]>
># Date 1342104101 25200
># Node ID f356af3cb177a153db40afe354f6d6beb7b93f0b
># Parent  0c6f0add64b2e115d5f374396b0dd38a7472c367
>Bug 773177 - java.lang.NullPointerException: at org.mozilla.gecko.TabsTray$TabsAdapter.refreshTabsData(TabsTray.java)
>
>diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java
>--- a/mobile/android/base/TabsTray.java
>+++ b/mobile/android/base/TabsTray.java
>@@ -188,18 +188,21 @@ public class TabsTray extends LinearLayo
>             }
>         }
> 
>         private void refreshTabsData() {
>             // Store a different copy of the tabs, so that we don't have to worry about
>             // accidentally updating it on the wrong thread.
>             mTabs = new ArrayList<Tab>();
>             ArrayList<Tab> tabs = Tabs.getInstance().getTabsInOrder();
>-            for (Tab tab : tabs) {
>-                mTabs.add(tab);
>+
>+            if (tabs != null) {
>+                for (Tab tab : tabs) {
>+                    mTabs.add(tab);
>+                }
>             }
> 
>             notifyDataSetChanged(); // Be sure to call this whenever mTabs changes.
>             updateSelectedPosition();
>         }
> 
>         // Updates the selected position in the list so that it will be scrolled to the right place.
>         private void updateSelectedPosition() {

Comment 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34982c7bcd30

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34982c7bcd30