Closed Bug 1264136 Opened 8 years ago Closed 8 years ago

NPE in TabMenuStripLayout:onPageSelected while closing last tab or opening new tab

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Grisha, Assigned: liuche)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

Attached file crash log
Seeing this on current Nighlty. At first, I couldn't get out of this state - essentially unable to switch away from the tabs screen without seeing the same crash. Clearing app data seems to have made this go away, and now I can't replicate the crash.

I had two different tabs open at the time.

Crash happens here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TabMenuStripLayout.java#99
For future reference, if you file a bug from crash-stats, it will auto-populate the crash signature field, and that will make this bug number appear in crash-stats next to the crash.
tracking-fennec: --- → ?
Crash Signature: [@ java.lang.NullPointerException: Attempt to invoke virtual method ''void android.widget.TextView.setTextColor(int)'' on a null object reference at org.mozilla.gecko.home.TabMenuStripLayout.onPageSelected(TabMenuStripLayout.java)]
Flags: needinfo?(s.kaspari)
Flags: needinfo?(liuche)
Keywords: crash
tracking-fennec: ? → ---
From the stack trace it looks like we are setting the pager to point to a page that doesn't exist. My assumption is that we migrate the panels (bug 1260321) but need to fix some index. I need to read more of the code in order to understand at what point we do migrate the old configuration and what index we end up using.
I also wonder if we should have migrated the "default panel" flag if the old sync/history panel has been the default (Maybe that's the issue here).
I originally added code to fix the default panel problem in the version 4 code, but mcomella pointed out that it's generally not good practice to change versions that are already in the wild.

I tested this by installing a pre-Combined-History build [1], setting Sync to be the default panel, upgrading through the migration that was landed in bug 1260321, verifying the crash when going to home panels, and upgrading to this version.

[1] http://people.mozilla.org/~liuche/bug-1260321/old-panels.apk
Flags: needinfo?(s.kaspari)
Flags: needinfo?(liuche)
Comment on attachment 8741197 [details]
MozReview Request: Bug 1264136 - NPE in TabMenuStripLayout:onPageSelected while closing last tab or opening new tab. r=sebastian

https://reviewboard.mozilla.org/r/46277/#review42919

LGTM.

I wonder if we should ask QA to test various upgrade scenarios with different panel configurations. We catched crashes but there could be errors that end up with a valid but unwanted configuration.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java:231
(Diff revision 1)
> +        // Make the History panel default. We can't modify existing PanelConfigs, so make a new one.
> +        final PanelConfig historyPanelConfig = createBuiltinPanelConfig(context, PanelType.COMBINED_HISTORY, EnumSet.of(PanelConfig.Flags.DEFAULT_PANEL));
> +        jsonPanels.put(historyIndex, historyPanelConfig.toJSON());

This is based on the assumption that either history or the sync panel have been the default before and that's why we don't have a default panel now, right?

I wonder if we should make the method name more explicit: I could something named ensureDefaultPanel() being useful for other future migrations too. But suddenly having the combined history panel as the default is not what you might expect. Also this might fail if the combined panel is hidden I guess (But it should always be active for the migration from 4 to 5 without a default - So no problem here specifically).

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java:234
(Diff revision 1)
> +        }
> +
> +        // Make the History panel default. We can't modify existing PanelConfigs, so make a new one.
> +        final PanelConfig historyPanelConfig = createBuiltinPanelConfig(context, PanelType.COMBINED_HISTORY, EnumSet.of(PanelConfig.Flags.DEFAULT_PANEL));
> +        jsonPanels.put(historyIndex, historyPanelConfig.toJSON());
> +        return jsonPanels;

I somehow missed this when reviewing the migration for version 4: We already manipulate the same JSONArray inside this method - There's no need to return and re-set it.
Attachment #8741197 - Flags: review?(s.kaspari) → review+
Assignee: nobody → liuche
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/11524fe96945
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.