898219 - NPE at BrowserSearch$SearchAdapter.getCount

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

From https://tbpl.mozilla.org/php/getParsedLog.php?id=25742166&tree=Fig&full=1

07-25 16:33:50.943 E/GeckoAppShell( 1789): java.lang.NullPointerException
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at org.mozilla.gecko.home.BrowserSearch$SearchAdapter.getCount(BrowserSearch.java:644)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.widget.AdapterView.checkFocus(AdapterView.java:689)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.widget.AdapterView$AdapterDataSetObserver.onInvalidated(AdapterView.java:813)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.database.DataSetObservable.notifyInvalidated(DataSetObservable.java:43)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.widget.BaseAdapter.notifyDataSetInvalidated(BaseAdapter.java:54)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.support.v4.widget.CursorAdapter.swapCursor(CursorAdapter.java:352)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.support.v4.widget.SimpleCursorAdapter.swapCursor(SimpleCursorAdapter.java:326)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at org.mozilla.gecko.home.BrowserSearch$CursorLoaderCallbacks.onLoaderReset(BrowserSearch.java:762)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.support.v4.app.LoaderManagerImpl$LoaderInfo.destroy(LoaderManager.java:339)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.support.v4.app.LoaderManagerImpl.doDestroy(LoaderManager.java:776)
07-25 16:33:50.943 E/GeckoAppShell( 1789): 	at android.support.v4.app.Fragment.onDestroy(Fragment.java:1140)

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Don't clear reference to mSearchEngines until onDestroy()

onDestroy() triggers onLoaderReset(), which calls mAdapter.swapCursor(null), which apparently ends up using getCount() in the adapter. Conceptually, I think mSearchEngines should be set/cleared in onCreate()/onDestroy(), but we do it now in onViewCreated() only because we need to ensure the view exists before the prompt is created. We can still create the initial array in onCreate(), though, so I think it makes sense to do so. The adapter relies on its existence, so it should fix this NPE if we wait until onDestroy() to clear it. This also means we shouldn't need the null check in setSuggestions(), which I didn't like having.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 781380 [details] [diff] [review]
Don't clear reference to mSearchEngines until onDestroy()

Review of attachment 781380 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/BrowserSearch.java
@@ +343,5 @@
>          getLoaderManager().restartLoader(SUGGESTION_LOADER_ID, null, mSuggestionLoaderCallbacks);
>      }
>  
>      private void setSuggestions(ArrayList<String> suggestions) {
> +        mSearchEngines.get(0).suggestions = suggestions;

Not a problem in your patch but I wonder: is mSearchEngines.get(0) guaranteed to be non-null?

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #2)
> Not a problem in your patch but I wonder: is mSearchEngines.get(0)
> guaranteed to be non-null?

Yeah, I think so. SearchLoader (which is the only place this method gets called) won't be triggered without a SuggestClient, and we don't create a SuggestClient unless we have a search engine with suggestions.

Comment 5

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/projects/fig/rev/d42a6a047a7a

Comment 6

[Ed Morley [:emorley]]

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