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

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

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)

Comment 2

[:Margaret Leibovic]

I just experience this again on Nightly. While tabs were being restored, I opened a new tab and started typing, and then it crashed.

Comment 3

[Andrzej Hunt :ahunt]

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?).

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1240500 - Pre: whitespace cleanup and unnecessary comment cleanup r=me

Review commit: https://reviewboard.mozilla.org/r/33179/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33179/

Comment 5

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

(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?

Comment 6

[:Margaret Leibovic]

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.

Comment 7

[Andrzej Hunt :ahunt]

(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.

Comment 8

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/281117771736a414087da02d430013e2b2d78019
Bug 1240500 - Don't repeatedly create ArrayLists when searching for occurrences r=margaret

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Andrzej Hunt :ahunt]

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?

Comment 11

[Kevin Brosnan [:kbrosnan]]

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.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like this is still happening to me

Comment 13

[Andrzej Hunt :ahunt]

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.

Comment 14

[Andrzej Hunt :ahunt]

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/

Comment 15

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1240500 - Early return on empty pattern to avoid an infinte loop and OOM r=rnewman

Review commit: https://reviewboard.mozilla.org/r/36741/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36741/

Comment 16

[Andrzej Hunt :ahunt]

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 17

[Richard Newman [:rnewman]]

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.

Comment 18

[Andrzej Hunt :ahunt]

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 19

[Andrzej Hunt :ahunt]

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/

Comment 20

[:Margaret Leibovic]

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 21

[Andrzej Hunt :ahunt]

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/

Comment 22

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

(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.

Comment 23

[Andrzej Hunt :ahunt]

(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.)

Comment 24

[Andrzej Hunt :ahunt]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec51ade02ea0
https://hg.mozilla.org/mozilla-central/rev/e93550cee488

Comment 26

[Andrzej Hunt :ahunt]

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

Comment 27

[Liz Henry (:lizzard)]

Marking affected for 46, at least there is 1 crash signature in the last month here.

Comment 28

[Liz Henry (:lizzard)]

I do see more crashes for Fennec on aurora now that I look a bit deeper. around 10 per week.

Comment 29

[Liz Henry (:lizzard)]

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.

Comment 30

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3691c3f188eb
https://hg.mozilla.org/releases/mozilla-aurora/rev/52f0bd111184

Comment 31

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3691c3f188eb
https://hg.mozilla.org/releases/mozilla-aurora/rev/52f0bd111184

Comment 32

[Wes Kocher (:KWierso)]

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?

Comment 33

[Andrzej Hunt :ahunt]

Attachment: oom_1240500_beta.patch

(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.)

Comment 34

[Liz Henry (:lizzard)]

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.

Comment 35

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/9e0d5e813a54