Closed Bug 1081948 Opened 5 years ago Closed 5 years ago

crash in java.lang.NullPointerException: key == null at java.util.EnumMap.putImpl(EnumMap.java)

Categories

(Firefox for Android :: General, defect, critical)

35 Branch
All
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified
fennec 35+ ---

People

(Reporter: TeoVermesan, Assigned: rnewman)

Details

(Keywords: crash, topcrash-android-armv7)

Crash Data

Attachments

(1 file)

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.
There is no sync set up. The crash is reproducible also when swiping left to go to the Synced Tabs Panel.
revealed by bug 1081287.
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)
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: nobody → rnewman
Status: NEW → ASSIGNED
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 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+
tracking-fennec: --- → 35+
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?
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*.
tecgirl: this was the crash we were seeing on your Nightly.
(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!
(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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Attachment #8504126 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 36.0a1 (2014-10-16)
Crash reproduced in Fennec Nightly 35.0a1 2014-10-13.
Crash does not reproduce in Fennec Aurora 35.0a2 2014-10-17.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.