Closed Bug 1240500 Opened 4 years ago Closed 4 years ago

OOM at java.util.ArrayList.add(ArrayList.java) in SearchEngineRow

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
fennec 45+ ---

People

(Reporter: Margaret, Assigned: ahunt)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-8e345e4c-7d8e-4e72-80c6-e511a2160118.
=============================================================

mfinkle was looking into some places where we allocate way more ArrayLists than necessary, and I believe this is one of them. We create multiple ArrayList objects every time we update the search suggestions, which happens whenever the user types.

Let's instead try to re-use ArrayLists, or use null instead of an empty list if we're handling the case where there are no suggestions.

java.lang.OutOfMemoryError: Failed to allocate a 154589920 byte allocation with 16777216 free bytes and 79MB until OOM
	at java.util.ArrayList.add(ArrayList.java:118)
	at org.mozilla.gecko.home.SearchEngineRow.bindSuggestionView(SearchEngineRow.java:275)
	at org.mozilla.gecko.home.SearchEngineRow.updateFromSavedSearches(SearchEngineRow.java:314)
	at org.mozilla.gecko.home.SearchEngineRow.updateSuggestions(SearchEngineRow.java:401)
	at org.mozilla.gecko.home.BrowserSearch$SearchAdapter.bindView(BrowserSearch.java:1103)
	at org.mozilla.gecko.home.MultiTypeCursorAdapter.getView(MultiTypeCursorAdapter.java:62)
	at android.widget.AbsListView.obtainView(AbsListView.java:2346)
	at android.widget.ListView.makeAndAddView(ListView.java:1876)
	at android.widget.ListView.fillDown(ListView.java:702)
	at android.widget.ListView.fillFromTop(ListView.java:763)
	at android.widget.ListView.layoutChildren(ListView.java:1671)
	at android.widget.AbsListView.onLayout(AbsListView.java:2148)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1743)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1586)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1495)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:336)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:273)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1079)
	at org.mozilla.gecko.GeckoApp$MainLayout.onLayout(GeckoApp.java:2543)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1079)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:336)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:273)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1743)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1586)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1495)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:336)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:273)
	at com.android.internal.policy.PhoneWindow$DecorView.onLayout(PhoneWindow.java:2678)
	at android.view.View.layout(View.java:16636)
	at android.view.ViewGroup.layout(ViewGroup.java:5437)
	at android.view.ViewRootImpl.performLayout(ViewRootImpl.java:2171)
	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:1931)
	at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1107)
	at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:6013)
	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:858)
	at android.view.Choreographer.doCallbacks(Choreographer.java:670)
	at android.view.Choreographer.doFrame(Choreographer.java:606)
	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:844)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:148)
	at android.app.ActivityThread.main(ActivityThread.java:5417)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
Duplicate of this bug: 1238880
Crash Signature: [@ java.lang.OutOfMemoryError: Failed to allocate a 154589920 byte allocation with 16777216 free bytes and 79MB until OOM at java.util.ArrayList.add(ArrayList.java)] → [@ java.lang.OutOfMemoryError: Failed to allocate a 154589920 byte allocation with 16777216 free bytes and 79MB until OOM at java.util.ArrayList.add(ArrayList.java)] [@ java.lang.OutOfMemoryError: (Heap Size=183996KB, Allocated=111918KB) at java.util.Arr…
Assignee: nobody → ahunt
tracking-fennec: ? → 45+
I just experience this again on Nightly. While tabs were being restored, I opened a new tab and started typing, and then it crashed.
I have a patch which would avoid creating new Lists by instead storing the list as a member variable (we then clear the list as soon as we no longer need the data in it).

I'm not entirely sure I fully understand what's going on, but it seems like this will at least save us from having too many ArrayLists floating around. 

he biggest issue I can see is that we never explicitly shrink the capacity of the list - that means we keep the full-sized List around until SearchEngineRow is killed, i.e. when BrowserSearch is killed (SearchEngineRow is an Android widget, so Android should take care of releasing that whenever BrowserSearch is killed?).
(In reply to Andrzej Hunt :ahunt from comment #3)
> I have a patch which would avoid creating new Lists by instead storing the
> list as a member variable (we then clear the list as soon as we no longer
> need the data in it).
> 
> I'm not entirely sure I fully understand what's going on, but it seems like
> this will at least save us from having too many ArrayLists floating around. 
> 
> he biggest issue I can see is that we never explicitly shrink the capacity
> of the list - that means we keep the full-sized List around until
> SearchEngineRow is killed, i.e. when BrowserSearch is killed
> (SearchEngineRow is an Android widget, so Android should take care of
> releasing that whenever BrowserSearch is killed?).

This is something we could test out with Leak Canary using RefWatcher. Maybe we could come up with a plan to try something out?
Attachment #8714600 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8714600 [details]
MozReview Request: Bug 1240500 - Pre: whitespace cleanup and unnecessary comment cleanup r=me

https://reviewboard.mozilla.org/r/33179/#review29995

This seems like a strict improvement, so we can land this and see how it affects the crash rate.
(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Andrzej Hunt :ahunt from comment #3)
> > I have a patch which would avoid creating new Lists by instead storing the
> > list as a member variable (we then clear the list as soon as we no longer
> > need the data in it).
> > 
> > I'm not entirely sure I fully understand what's going on, but it seems like
> > this will at least save us from having too many ArrayLists floating around. 
> > 
> > he biggest issue I can see is that we never explicitly shrink the capacity
> > of the list - that means we keep the full-sized List around until
> > SearchEngineRow is killed, i.e. when BrowserSearch is killed
> > (SearchEngineRow is an Android widget, so Android should take care of
> > releasing that whenever BrowserSearch is killed?).
> 
> This is something we could test out with Leak Canary using RefWatcher. Maybe
> we could come up with a plan to try something out?

I've been investigating with LeakCanary, but couldn't find any leaks so far - as suggested on IRC I'll try to land some patches so we can still keep track of this in future.
https://hg.mozilla.org/integration/fx-team/rev/281117771736a414087da02d430013e2b2d78019
Bug 1240500 - Don't repeatedly create ArrayLists when searching for occurrences r=margaret
https://hg.mozilla.org/mozilla-central/rev/281117771736
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
The only recent instances of this crash seem to be with old builds ( <= 26th Jan), so it seems like this patch helped - but I don't see any similar instances in aurora, beta, or release. Would we still want to uplift this?
I see bp-f0cbb10c-4521-4cf9-8458-9e9112160208 with a build ID 20160206030207 (2016-02-06-03:02:07) which has email, URL and logcat.

https://crash-stats.mozilla.com/search/?product=FennecAndroid&signature=~java.util.ArrayList.add%28ArrayList.java%29&version=47.0a1&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

The crash seems a bit rare and spkiey to say if this helped. 19 crashes in 14 days of which 4 days have the fix. Jan-28 accounts for 9 of the crashes.
Looks like this is still happening to me
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm not able to reproduce myself, but it seems like this would happen if search suggestions arrive and are processed, after the search field has been emptied. Which would result in an infinite loop that keeps expanding the ArrayList, leading to OOM.
Attachment #8714600 - Attachment description: MozReview Request: Bug 1240500 - Don't repeatedly create ArrayLists when searching for occurrences r=margaret → MozReview Request: Bug 1240500 - Pre: whitespace cleanup and unnecessary comment cleanup r=me
Comment on attachment 8714600 [details]
MozReview Request: Bug 1240500 - Pre: whitespace cleanup and unnecessary comment cleanup r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33179/diff/1-2/
We do hide the search suggestions when the search field is emptied, so we shouldn't really be calling that code with an empty input in the first place - but that looks like that would be a much more complex fix.

(I *think* we just hide the search result panel when the urlbar is empty, and it lives on in the background. This also means we can still see outdated search suggestions imediately after starting typing again, between the point where the search-panel is shown and when we actually refresh the suggestions. We should probably clear that up.)
Comment on attachment 8723821 [details]
MozReview Request: Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman

https://reviewboard.mozilla.org/r/36741/#review33297

::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java:177
(Diff revision 1)
> +        if (patternLength == 0) {

Move up to line 173 as:

```
if (TextUtils.isEmpty(pattern)) {
    return;
}
```

which buys us null-safety, too.
Attachment #8723821 - Flags: review?(rnewman) → review+
Comment on attachment 8714600 [details]
MozReview Request: Bug 1240500 - Pre: whitespace cleanup and unnecessary comment cleanup r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33179/diff/2-3/
Comment on attachment 8723821 [details]
MozReview Request: Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36741/diff/1-2/
https://reviewboard.mozilla.org/r/36741/#review33305

::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java:46
(Diff revision 2)
> +import ch.boye.httpclientandroidlib.util.TextUtils;

Is this the import you intended? We probably want android.text.TextUtils.
Comment on attachment 8723821 [details]
MozReview Request: Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36741/diff/2-3/
(In reply to :Margaret Leibovic from comment #20)
> https://reviewboard.mozilla.org/r/36741/#review33305
> 
> ::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java:46
> (Diff revision 2)
> > +import ch.boye.httpclientandroidlib.util.TextUtils;
> 
> Is this the import you intended? We probably want android.text.TextUtils.

Agreed. I think I saw this in a patch Sebastian created too. Must be an IDE issue.
(In reply to Mark Finkle (:mfinkle) from comment #22)
> (In reply to :Margaret Leibovic from comment #20)
> > https://reviewboard.mozilla.org/r/36741/#review33305
> > 
> > ::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java:46
> > (Diff revision 2)
> > > +import ch.boye.httpclientandroidlib.util.TextUtils;
> > 
> > Is this the import you intended? We probably want android.text.TextUtils.
> 
> Agreed. I think I saw this in a patch Sebastian created too. Must be an IDE
> issue.

Yup - I noticed that the ch.boye.httpclientandroidlib.util.TextUtils is listed before android.util.TextUtils when asking the IDE to import "TextUtils". (In AS type TextUtils.something(), alt+enter -> ch.boye.httpclientandroidlib.util.TextUtils is selected by default, you have to then move down to android.util.TextUtils which is the second item, press enter to add the import.)
https://hg.mozilla.org/integration/fx-team/rev/ec51ade02ea0e58031f3a051c54413e516e865f6
Bug 1240500 - Pre: whitespace cleanup and unnecessary comment cleanup r=me

https://hg.mozilla.org/integration/fx-team/rev/e93550cee4886e3904ca91842ed477924365e2e9
Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman
https://hg.mozilla.org/mozilla-central/rev/ec51ade02ea0
https://hg.mozilla.org/mozilla-central/rev/e93550cee488
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Comment on attachment 8723821 [details]
MozReview Request: Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman

This seems to be the 7th highest crasher in Aurora - I also can't see any examples of this happening on Nightly since it landed there (there are still new crash reports, but they're all with old build-ids) - hence I'd like to request approval-mozilla-aurora.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Crashes might occur when attempting to search in Firefox
[Describe test coverage new/current, TreeHerder]: Manual testing.
[Risks and why]: low risk - this patch adds a single string-length check.
[String/UUID change made/needed]: none
Attachment #8723821 - Flags: approval-mozilla-aurora?
Marking affected for 46, at least there is 1 crash signature in the last month here.
I do see more crashes for Fennec on aurora now that I look a bit deeper. around 10 per week.
Crash Signature: , Allocated=111918KB) at java.util.ArrayList.add(ArrayList.java)] → , Allocated=111918KB) at java.util.ArrayList.add(ArrayList.java)] [@ java.lang.OutOfMemoryError: at java.util.ArrayList.add(ArrayList.java) ]
Comment on attachment 8723821 [details]
MozReview Request: Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman

Crash fix, seems to have worked in nightly; please uplift to aurora.
Attachment #8723821 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I had to back these out from aurora for android build failures like: https://treeherder.mozilla.org/logviewer.html#?job_id=2076777&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/d249b179929d


IIRC, I had to do resolve some merge conflicts that I thought were simple enough for me to handle, so it might have been from that. Either way, can you take a look at this and post a rebased patch for aurora?
Flags: needinfo?(ahunt)
(In reply to Wes Kocher (:KWierso) from comment #32)
> I had to back these out from aurora for android build failures like:
> https://treeherder.mozilla.org/logviewer.html#?job_id=2076777&repo=mozilla-
> aurora
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/d249b179929d
> 
> 
> IIRC, I had to do resolve some merge conflicts that I thought were simple
> enough for me to handle, so it might have been from that. Either way, can
> you take a look at this and post a rebased patch for aurora?

Here's a rebased / fixed patch for beta (as far as I can tell 46 turned into beta earlier today) - do I need to re-request approval for beta now because of the new release?

(It looks like we changed the method signature between 46 and 47 in a different bug, but we still need essentially the same fix in both, but returning an empty array in the 46 version instead of returning early before filling a shared/member-variable array in 47.)
Flags: needinfo?(ahunt) → needinfo?(wkocher)
Comment on attachment 8727496 [details] [diff] [review]
oom_1240500_beta.patch

[Triage Comment]

OK to uplift for beta 46.  This should end up in the beta 2 build next week.
Attachment #8727496 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.