Closed Bug 1310621 Opened 3 years ago Closed 3 years ago

NullPointerException: Attempt to invoke interface method 'java.lang.Object java.util.List.get(int)' on a null object reference

Categories

(Firefox for Android :: General, defect, critical)

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
fennec 52+ ---
firefox52 --- fixed

People

(Reporter: sebastian, Assigned: cnevinchen)

Details

(Keywords: crash, Whiteboard: [TPE-1])

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-37806408-89df-47bb-a8c6-d1c442161013.
=============================================================
There are 3 crashes all are from a Sony C6833 Xperia Z Ultra
tracking-fennec: ? → 52+
Whiteboard: TPE-1
Whiteboard: TPE-1 → [TPE-1]
Nevin will try to reproduce it.
Assignee: nobody → cnevinc
Status: NEW → ASSIGNED
I guess the problem happened when suggestion result in onLoadFinished() callback completed after  fragment called onDestroy().At that time mSearchEngines was null, and mSearchEngines.get(0) in setSuggestions(ArrayList<String> suggestions) would caused NPE. 

I'll try to reproduce it in a very slow network. Or just make some delay in onLoadFinished() to see if it'll survived when the fragment is closed before the delay completes.
@Before
To force onDestroy() to be called, I turn on "Don't keep activities" in developer settings.
Then in SearchEngineSuggestionLoaderCallbacks, I add some delay(10s) for setSuggestions() in onLoadFinished().

@Test
Eneter "Apple" in the url. Before the suggestion result gets displayed, press the home button and leave the app. At this moment,  mSearchEngines is set to null in onDestroy()
Bring back the app to foreground. When loader's onLoaderFinished() gets called, the crash with the same symbol occurs.


====
Suggestion to fix this : don't assign null to mSearchEngines since its data is from Gecko and should not be mutated.
Comment on attachment 8805499 [details]
Bug 1310621 - Drop the suggestions if search engines are not available.

https://reviewboard.mozilla.org/r/89242/#review88552

Nice debugging and great that you found some steps to reproduce this!

This approach should work but I wonder if we should stop the loader (destroyLoader()) when the fragment is destroyed. I guess there's no need to continue loading suggestions after the fragment is gone. Can you try if this is an option here?

If this is not a valid approach then I guess I'd prefer that we still remove the search engines but check in setSuggestions() whether we have a search engine to attach them to. And if not then we just drop the suggestions.
Attachment #8805499 - Flags: review?(s.kaspari)
Since the loader will survived when the fragment/activity's configuration change(recreate), I'll go for your second approach. I'll update and submit again. Thanks!
Flags: needinfo?(s.kaspari)
Attachment #8806237 - Attachment is obsolete: true
Comment on attachment 8805499 [details]
Bug 1310621 - Drop the suggestions if search engines are not available.

https://reviewboard.mozilla.org/r/89242/#review89598

LGTM!

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:690
(Diff revision 3)
> +        // mSearchEngines may be null if the setSuggestions calls after onDestroy (bug 1310621).
> +        // So drop the suggestions if search engines are not available
> +        if (mSearchEngines != null && !mSearchEngines.isEmpty()) {
> -        mSearchEngines.get(0).setSuggestions(suggestions);
> +            mSearchEngines.get(0).setSuggestions(suggestions);
> -        mAdapter.notifyDataSetChanged();
> +            mAdapter.notifyDataSetChanged();
> -    }
> +        }
>  
> +    }

At first I thought there could be a race condition here: mSearchEngine is not null while you check it but is null when you access it the next time. However this method and onDestroy() should both run on the UI thread (there's an assertion in this method). So this shouldn't happen.
Attachment #8805499 - Flags: review?(s.kaspari) → review+
Do you already have level 1 commit access [1] to push the patch to try [2]?

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/
[2] https://wiki.mozilla.org/ReleaseEngineering/TryServer

(We will talk about this during our onboarding week too)
Flags: needinfo?(s.kaspari)
I don't have level 1 commit access yet. I think I should resolve more issues :-)
https://bugzilla.mozilla.org/show_bug.cgi?id=1313586
Flags: needinfo?(s.kaspari)
I try to push try for him but I can't do that from MozReview directly. Will ask around.

I will pull his patch my local repo and push to try from there.
Okay, let's get this landed. :)
Flags: needinfo?(s.kaspari)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6225ca668757
Drop the suggestions if search engines are not available. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6225ca668757
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.