Closed
Bug 1081948
Opened 10 years ago
Closed 10 years ago
crash in java.lang.NullPointerException: key == null at java.util.EnumMap.putImpl(EnumMap.java)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox35 verified, firefox36 verified, fennec35+)
VERIFIED
FIXED
Firefox 36
People
(Reporter: TeoVermesan, Assigned: rnewman)
Details
(Keywords: crash, topcrash-android-armv7)
Crash Data
Attachments
(1 file)
3.71 KB,
patch
|
Margaret
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-71aaee26-cbcf-4a90-b9ca-a5b742141013. ============================================================= Steps to reproduce: 1. Open Firefox and open the tab tray 2. Go to the synced tabs and tap "Take me to my new panel" Expected results: - Synced Tabs Panel should appear. Actual results: - Firefox crashes.
Reporter | ||
Comment 1•10 years ago
|
||
There is no sync set up. The crash is reproducible also when swiping left to go to the Synced Tabs Panel.
Assignee | ||
Comment 2•10 years ago
|
||
revealed by bug 1081287.
Assignee | ||
Comment 3•10 years ago
|
||
java.lang.NullPointerException: key == null at java.util.EnumMap.putImpl(EnumMap.java:776) at java.util.EnumMap.put(EnumMap.java:632) at java.util.EnumMap.put(EnumMap.java:28) at org.mozilla.gecko.home.RemoteTabsPanel.updateUiFromAccount(RemoteTabsPanel.java:204) at org.mozilla.gecko.home.RemoteTabsPanel$AccountLoaderCallbacks.onLoadFinished(RemoteTabsPanel.java:207) at android.support.v4.app.LoaderManagerImpl$LoaderInfo.callOnLoadFinished(LoaderManager.java:427) at android.support.v4.app.LoaderManagerImpl$LoaderInfo.onLoadComplete(LoaderManager.java:395) at android.support.v4.content.Loader.deliverResult(Loader.java:104)
Comment 4•10 years ago
|
||
A comment in RemoteTabsPanel says: "We use containsKey rather than get because null is a valid key." ... Really? It seems like if you're using an enum, having null as a valid state is madness: just add a new enum constant to represent the new state. So, I suggest that's the way forward. Add a new enum value representing whatever state was previously represented by null.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
This patch switches us to using a separate field for tracking the fallback fragment, and tidies away some of the logic that existed for the old null case. Tested on device both with and without a Firefox Account.
Attachment #8504126 -
Flags: review?(margaret.leibovic)
Comment 6•10 years ago
|
||
Comment on attachment 8504126 [details] [diff] [review] Don't use null as an EnumMap key. v1 Review of attachment 8504126 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/RemoteTabsPanel.java @@ +188,1 @@ > // We use containsKey rather than get because null is a valid key. Update the comment.
Attachment #8504126 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8a6478ddd0e0
Assignee | ||
Updated•10 years ago
|
tracking-fennec: --- → 35+
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8504126 [details] [diff] [review] Don't use null as an EnumMap key. v1 Requesting uplift to Aurora 35 when the merge dance is done. Approval Request Comment [Feature/regressing bug #]: Bug 1081287. [User impact if declined]: Crashes in synced tabs panel. [Describe test coverage new/current, TBPL]: None. [Risks and why]: Very low risk. We switched from a data structure that allows null keys to one that doesn't. This patch avoids us needing null keys. [String/UUID change made/needed]: None.
Attachment #8504126 -
Flags: approval-mozilla-aurora?
Comment 9•10 years ago
|
||
I'm glad we didn't add a new enum constant (that was clearly not the right thing to do) but I'm frustrated that we made a hygienic change that broke *well commented working code*.
Comment 10•10 years ago
|
||
tecgirl: this was the crash we were seeing on your Nightly.
Comment 11•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #9) > I'm glad we didn't add a new enum constant (that was clearly not the right > thing to do) but I'm frustrated that we made a hygienic change that broke > *well commented working code*. Yeah, my bad, I should've spotted the comment mentioning the reliance on null when I wrote the earlier patch. And, yes, I don't have much domain-specific knowledge for this part of the code. Generally, though, enum-with-null is a weird thing to want. Sorry!
Comment 12•10 years ago
|
||
(In reply to Chris Kitching [:ckitching] from comment #11) > (In reply to Nick Alexander :nalexander from comment #9) > > I'm glad we didn't add a new enum constant (that was clearly not the right > > thing to do) but I'm frustrated that we made a hygienic change that broke > > *well commented working code*. > > Yeah, my bad, I should've spotted the comment mentioning the reliance on > null when I wrote the earlier patch. > > And, yes, I don't have much domain-specific knowledge for this part of the > code. Generally, though, enum-with-null is a weird thing to want. enum-with-null is reasonable in this use case, I think. But Java explicitly allows null for enum values, which really is terrible :) > Sorry! n/p; well commented < well tested :(
https://hg.mozilla.org/mozilla-central/rev/8a6478ddd0e0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•10 years ago
|
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
Updated•10 years ago
|
Attachment #8504126 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Comment 15•10 years ago
|
||
Verified as fixed on: Device: Alcatel One Touch (Android 4.1.2) Build: Firefox for Android 36.0a1 (2014-10-16)
Comment 16•10 years ago
|
||
Crash reproduced in Fennec Nightly 35.0a1 2014-10-13. Crash does not reproduce in Fennec Aurora 35.0a2 2014-10-17.
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
•