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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

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.

Comment 1

[Teodora Vermesan (:TeoVermesan)]

There is no sync set up. The crash is reproducible also when swiping left to go to the Synced Tabs Panel.

Comment 2

[Richard Newman [:rnewman]]

revealed by bug 1081287.

Comment 3

[Richard Newman [:rnewman]]

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

[Chris Kitching [:ckitching]]

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.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Don't use null as an EnumMap key. v1

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.

Comment 6

[: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.

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/8a6478ddd0e0

Comment 8

[Richard Newman [:rnewman]]

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.

Comment 9

[Nick Alexander :nalexander [he/him]]

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

[Nick Alexander :nalexander [he/him]]

tecgirl: this was the crash we were seeing on your Nightly.

Comment 11

[Chris Kitching [:ckitching]]

(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

[Nick Alexander :nalexander [he/him]]

(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 :(

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a6478ddd0e0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/735ab4890a4a

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed on:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 36.0a1 (2014-10-16)

Comment 16

[u279076]

Crash reproduced in Fennec Nightly 35.0a1 2014-10-13.
Crash does not reproduce in Fennec Aurora 35.0a2 2014-10-17.