911604 - Non-default (=manually added) search engines not listed in search settings, can't be set as default

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox for Android nightly 2013-09-01, Android 4.1.2 (stock), Google Nexus S

The previously added search engines are listed in the awesomebar search, but not in the search settings where the default search engine can be set. So the custom ones can't be made default.

Comment 1

[Aaron Train [:aaronmt]]

Perhaps there was an issue in migration. Do you have steps to reproduce? Manually added search engines can be set as default in the new search settings just by tapping them and set as default. For example, I just went to Duckduckgo.com and added that search engine and then set it as default in the options.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: logcat

I updated from 2013-08-(something between 07 and 09) to 2013-09-01.

The issue persists after adding Duckduckgo, it is not listed in the search settings after adding it. Looking into the logcat, decodeByteArray() seems to fail, causing a null pointer exception. Related to bug 819895?

Comment 3

[Chris Kitching [:ckitching]]

(In reply to Archaeopteryx [:aryx] from comment #2)
> Created attachment 798606 [details]
> logcat
> 
> I updated from 2013-08-(something between 07 and 09) to 2013-09-01.
> 
> The issue persists after adding Duckduckgo, it is not listed in the search
> settings after adding it. Looking into the logcat, decodeByteArray() seems
> to fail, causing a null pointer exception. Related to bug 819895?

Are you using a debug build? If so, this smells like Bug 907917 - for a variety of weird reasons the assertion failure ultimately leads to death by NPE on the Java side.
So far as I can tell, the underlying cause has existed since 23, is an incorrect assertion, and is not obvious.

If you're not using a debug build this is sort of interesting. Perhaps related to some of the madness I've recently encountered in the Favicon system - under certain stupid conditions it might end up storing a null or corrupt Favicon in the database, and then try decoding that, causing the problem. It would be interesting to see the NPE, plus stack trace. It shouldn't be too problematic to add a test for such madness to the search settings thing. It might be preferable to make the caching system less awful (That's being worked on).

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

I am not using a debug build, just default mozilla-central nightly branch. After removing the "Gecko" filter in logcat, the only interesting new line I can find is
D/skia    (10749): --- SkImageDecoder::Factory returned null

The favicon of one search engine seems indeed to be corrupted, see https://bug819895.bugzilla.mozilla.org/attachment.cgi?id=690342 and also bug 819895 comment 5.

Comment 5

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #3)

> It would be interesting to see the NPE, plus stack trace. It
> shouldn't be too problematic to add a test for such madness to the search
> settings thing. It might be preferable to make the caching system less awful
> (That's being worked on).

From the attached log:

E/GeckoEventDispatcher( 5917): handleGeckoMessage throws java.lang.NullPointerException
E/GeckoEventDispatcher( 5917): java.lang.NullPointerException
E/GeckoEventDispatcher( 5917): 	at android.graphics.Bitmap.createScaledBitmap(Bitmap.java:461)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.preferences.SearchEnginePreference.setSearchEngineFromJSON(SearchEnginePreference.java:108)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.preferences.SearchPreferenceCategory.handleMessage(SearchPreferenceCategory.java:77)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:96)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:58)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2220)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:337)
E/GeckoEventDispatcher( 5917): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:174)

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Add null check before creating scaled bitmap.

It appears that this is caused by a corrupt/nonexistent Favicon being provided. For now, the simple solution seems to be just add a null check and don't do createScaledBitmap with null.
Once the Favicon caching madness has been demadnessified, I'll revisit this and have it either retry the download or display the default Favicon.

(In reply to :Margaret Leibovic from comment #5)
> From the attached log:
> 
> E/GeckoEventDispatcher( 5917): handleGeckoMessage throws
> java.lang.NullPointerException
> E/GeckoEventDispatcher( 5917): java.lang.NullPointerException
> E/GeckoEventDispatcher( 5917): 	at
> android.graphics.Bitmap.createScaledBitmap(Bitmap.java:461)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.preferences.SearchEnginePreference.
> setSearchEngineFromJSON(SearchEnginePreference.java:108)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.preferences.SearchPreferenceCategory.
> handleMessage(SearchPreferenceCategory.java:77)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:96)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:58)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2220)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:337)
> E/GeckoEventDispatcher( 5917): 	at
> org.mozilla.gecko.GeckoThread.run(GeckoThread.java:174)

My apologies. It appears I have forgotten how to read.

Comment 7

[:Margaret Leibovic]

Comment on attachment 799154 [details] [diff] [review]
Add null check before creating scaled bitmap.

This seems like a good workaround. Please also add a comment about why we need to do this null check.

Comment 8

[Chris Kitching [:ckitching]]

Attachment: V2 - Add null check before creating scaled bitmap - plus comment.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: V3 - Add null check before creating scaled bitmap - plus comments.

...A commit comment would probably also be useful...

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/31d86f50d437

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/31d86f50d437

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I assume this needs to be uplifted to Fx25, so please request approval.

Comment 13

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> I assume this needs to be uplifted to Fx25, so please request approval.

The search settings stuff is only in 26, right?

Comment 14

[Chris Kitching [:ckitching]]

(In reply to :Margaret Leibovic from comment #13)
> The search settings stuff is only in 26, right?

Correct.

Comment 15

[u421692]

Test case created in moztrap:
https://moztrap.mozilla.org/manage/case/10587/