Closed
Bug 1310621
Opened 8 years ago
Closed 8 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 Graveyard :: General, defect)
Tracking
(fennec52+, firefox52 fixed)
RESOLVED
FIXED
Firefox 52
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. =============================================================
Comment 1•8 years ago
|
||
There are 3 crashes all are from a Sony C6833 Xperia Z Ultra
Reporter | ||
Updated•8 years ago
|
Whiteboard: TPE-1 → [TPE-1]
Comment 2•8 years ago
|
||
Nevin will try to reproduce it.
Assignee: nobody → cnevinc
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
@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 hidden (mozreview-request) |
Reporter | ||
Comment 6•8 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8806237 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•8 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9722f4351c8d3281a87f1628743f4e846f04d5a1 (the try syntax was given by :sebastian over IRC a few days ago)
Reporter | ||
Comment 16•8 years ago
|
||
Okay, let's get this landed. :)
Flags: needinfo?(s.kaspari)
Keywords: regressionwindow-wanted → checkin-needed
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6225ca668757
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•