Closed Bug 1325586 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

53 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: cfat, Assigned: jwu)

Details

(Keywords: regression, reproducible)

Crash Data

Attachments

(2 files)

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
Crash Signature: [@ java.lang.NullPointerException: key == null at android.util.LruCache.get(LruCache.java) ]
This is 100% reproducible with the 9gag.com search bar also added.
This is currently the #2 top crash on nightly fennec.
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
Another search button/etc bug
See Also: → 1328104, 1326826
(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.
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
See Also: 1328104, 1326826
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);
Assignee: nobody → topwu.tw
Status: NEW → ASSIGNED
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.
Attachment #8826515 - Flags: review?(s.kaspari) → review-
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 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. :)
Attachment #8826515 - Flags: review?(s.kaspari)
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?
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
Comment on attachment 8826515 [details]
Bug 1325586 - Prevent creating IconDescriptor with null url,

https://reviewboard.mozilla.org/r/104474/#review106712
Attachment #8826515 - Flags: review+
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba650eee2f3b
Prevent creating IconDescriptor with null url, r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba650eee2f3b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.