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

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Grisha, Assigned: liuche)

Tracking

({crash})

unspecified
Firefox 48
crash
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8740703 [details]
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

Updated

2 years ago
Duplicate of this bug: 1264265

Comment 2

2 years ago
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

Updated

2 years ago
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).
Created attachment 8741197 [details]
MozReview Request: Bug 1264136 - NPE in TabMenuStripLayout:onPageSelected while closing last tab or opening new tab. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/46277/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46277/
Attachment #8741197 - Flags: review?(s.kaspari)
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

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/11524fe96945
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.