1325586 - Crash in java.lang.NullPointerException: key == null at android.util.LruCache.get(LruCache.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Carmen Fat [:cfat] - Ecosystem QA]

Environment: 
Device: Nexus 5 (Android 6.0.1);
Build: Nightly 53.0a1 (2016-12-22);

Steps to reproduce:
1. Launch Firefox;
2. Go to imdb.com;
3. Long tap in the search field to bring up the action bar; 
4. Choose "Add Search Engine"(the magnifying glass icon);
5. Go to Settings -> Search.

Expected result:
Search panel is displayed.

Actual result:
Firefox crashes.

Notes:
Crash report: https://crash-stats.mozilla.com/report/index/682d239a-a436-4546-b46b-48aad2161223

Comment 1

[u549602]

This is 100% reproducible with the 9gag.com search bar also added.

Comment 2

[Marcia Knous [:marcia]]

This is currently the #2 top crash on nightly fennec.

Comment 3

[Carmen Fat [:cfat] - Ecosystem QA]

Tested on:
 - Huawei Honor 5x (Android 5.1.1)

Regression Window performed:
Last known good build: 2016-12-18
First known bad build: 2016-12-19

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f22c6dc53276d9cb1e7365e47dc52657facc5e48&tochange=12f4a1387fbc20a4696cd0cc9ea1ae7cddeb213c

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Another search button/etc bug

Comment 5

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #4)
> Another search button/etc bug

Not sure what you tried to do, but I don't think you intended to connect a Firefox desktop Linux-only search bug with a java crash on fennec.

Comment 6

[Jing-wei Wu [:jwu]]

According to https://developer.android.com/reference/android/util/LruCache.html, null cannot be a key for LruCache. We should check if key is null before call LruCache.get(key);

Comment 7

[Jing-wei Wu [:jwu]]

Attachment: Bug 1325586 - Prevent creating IconDescriptor with null url,

Review commit: https://reviewboard.mozilla.org/r/104474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104474/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826515 [details]
Bug 1325586 - Prevent creating IconDescriptor with null url,

https://reviewboard.mozilla.org/r/104474/#review105324

This fixes the sympton, but looking at the bigger picture something is wrong here: We shouldn't end up with a 'null' value here in the first place.

It would be interesting to follow the path back and see how we got a null URL into the IconDescriptor object. Looking at it we maybe should at some @NonNull annotations to the IconDescriptor methods too. And it might be worth adding null checks into IconDescriptor so that we fail earlier and can fix the calling code that inserts those nulls.

Comment 9

[Jing-wei Wu [:jwu]]

Comment on attachment 8826515 [details]
Bug 1325586 - Prevent creating IconDescriptor with null url,

https://reviewboard.mozilla.org/r/104474/#review105324

After deep check, the crash is caused by SearchEnginePreference#setSearchEngineFromBundle(GeckoBundle). Because iconURI provided by GeckoBundle could be nullable and no null check before creating an IconDescriptor. 

I'll add null check before creating an IconDescriptor. Also adding @NonNull annotations to the IconDescriptor methods.

Comment 10

[Jing-wei Wu [:jwu]]

Comment on attachment 8826515 [details]
Bug 1325586 - Prevent creating IconDescriptor with null url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104474/diff/1-2/

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826515 [details]
Bug 1325586 - Prevent creating IconDescriptor with null url,

https://reviewboard.mozilla.org/r/104474/#review105934

Nice! This looks good.

I have only one doubt: Now we are displaying no icon, I guess? This is fine, but how does this behave in the search engine bar if you enter text and scroll to this search engine - do we show an icon here? Just reflag me for review on bugzilla after you have tried this and if you decide no further change is needed. :)

Comment 12

[Jing-wei Wu [:jwu]]

Attachment: device-2017-01-18-151626.png

You can see the attachment there is a default icon when the search engine. Do you think we should also show the default icon in search preference page?

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jing-wei Wu [:jwu] from comment #12)
> You can see the attachment there is a default icon when the search engine.
> Do you think we should also show the default icon in search preference page?

Okay! I just wanted to make sure that this is working too. I'll r+ the patch.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8826515 [details]
Bug 1325586 - Prevent creating IconDescriptor with null url,

https://reviewboard.mozilla.org/r/104474/#review106712

Comment 15

[Jing-wei Wu [:jwu]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=886b95bcdccee4651fc08af84a938817b8792dd8

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba650eee2f3b
Prevent creating IconDescriptor with null url, r=sebastian

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ba650eee2f3b

Comment 18

[Carmen Fat [:cfat] - Ecosystem QA]

Closing this issue since the "Add search engine" feature changed its format on the latest Firefox for Android versions and the original STR are no longer applicable. Please reopen the report if you have any concerns about it.