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)
Firefox for Android Graveyard
General
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)
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
Comment 2•8 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•8 years ago
|
tracking-fennec: ? → ---
Comment 3•8 years ago
|
||
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).
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11524fe96945
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•