Closed
Bug 1240500
Opened 6 years ago
Closed 6 years ago
OOM at java.util.ArrayList.add(ArrayList.java) in SearchEngineRow
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Firefox for Android Graveyard
Awesomescreen
Tracking
(firefox46 fixed, firefox47 fixed, fennec45+)
RESOLVED
FIXED
Firefox 47
People
(Reporter: Margaret, Assigned: ahunt)
References
Details
(Keywords: crash)
Crash Data
Attachments
(3 files)
58 bytes,
text/x-review-board-request
|
Margaret
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
rnewman
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
2.41 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Updated•6 years ago
|
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+
Reporter | ||
Comment 2•6 years ago
|
||
I just experience this again on Nightly. While tabs were being restored, I opened a new tab and started typing, and then it crashed.
Assignee | ||
Comment 3•6 years ago
|
||
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?).
Assignee | ||
Comment 4•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/33179/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33179/
Attachment #8714600 -
Flags: review?(margaret.leibovic)
Comment 5•6 years ago
|
||
(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?
Reporter | ||
Updated•6 years ago
|
Attachment #8714600 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/281117771736a414087da02d430013e2b2d78019 Bug 1240500 - Don't repeatedly create ArrayLists when searching for occurrences r=margaret
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/281117771736
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
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 → ---
Assignee | ||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
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
Assignee | ||
Comment 14•6 years ago
|
||
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/
Assignee | ||
Comment 15•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36741/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36741/
Attachment #8723821 -
Flags: review?(rnewman)
Assignee | ||
Comment 16•6 years ago
|
||
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•6 years ago
|
||
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+
Assignee | ||
Comment 18•6 years ago
|
||
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/
Assignee | ||
Comment 19•6 years ago
|
||
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/
Reporter | ||
Comment 20•6 years ago
|
||
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.
Assignee | ||
Comment 21•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 23•6 years ago
|
||
(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.)
Assignee | ||
Comment 24•6 years ago
|
||
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec51ade02ea0 https://hg.mozilla.org/mozilla-central/rev/e93550cee488
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•6 years ago
|
||
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.
status-firefox46:
--- → affected
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+
Comment 30•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3691c3f188eb https://hg.mozilla.org/releases/mozilla-aurora/rev/52f0bd111184
Comment 31•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3691c3f188eb https://hg.mozilla.org/releases/mozilla-aurora/rev/52f0bd111184
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)
Assignee | ||
Comment 33•6 years ago
|
||
(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+
Comment 35•6 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/9e0d5e813a54
Flags: needinfo?(wkocher)
Updated•1 year 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
•