Last Comment Bug 888053 - Under certain conditions the top sites thumbnails leak database cursors
: Under certain conditions the top sites thumbnails leak database cursors
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Awesomescreen (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 25
Assigned To: Chris Kitching [:ckitching]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: 760394
  Show dependency treegraph
 
Reported: 2013-06-27 15:30 PDT by Chris Kitching [:ckitching]
Modified: 2013-07-10 08:53 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix for the Cursor leak described (2.34 KB, patch)
2013-06-27 15:33 PDT, Chris Kitching [:ckitching]
bnicholson: review+
Details | Diff | Splinter Review
Bug 888053 - Fix cursor leak in TopSitesView r=bnicholson (2.34 KB, patch)
2013-07-01 11:27 PDT, Chris Kitching [:ckitching]
bnicholson: review+
Details | Diff | Splinter Review

Description Chris Kitching [:ckitching] 2013-06-27 15:30:55 PDT
Starting with an Awesomescreen with one or more gaps in the top sites display (That is, you have not yet visited enough sites for Fennec to have images to display in all the slots), tap on such a slot to add a site there. If you select a site for which a thumbnail is not available, the database Cursor goes out of scope without having close() called on it, creating a memory leak.
The culprit seems to be an if statement in TopSitesView, around line 650, which causes the function to return early without closing the Cursor.
Comment 1 Chris Kitching [:ckitching] 2013-06-27 15:33:46 PDT
Created attachment 768615 [details] [diff] [review]
Fix for the Cursor leak described

The fix!
Comment 2 Brian Nicholson (:bnicholson) 2013-06-27 16:02:25 PDT
Comment on attachment 768615 [details] [diff] [review]
Fix for the Cursor leak described

Review of attachment 768615 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find!

::: mobile/android/base/widget/TopSitesView.java
@@ +662,5 @@
> +                                    bitmap = BitmapUtils.decodeByteArray(b);
> +                                }
> +                            }
> +                        } finally {
> +                            if(c != null) {

Nit: space before "("
Comment 3 Chris Kitching [:ckitching] 2013-07-01 11:27:28 PDT
Created attachment 769769 [details] [diff] [review]
Bug 888053 - Fix cursor leak in TopSitesView r=bnicholson

Nit-picked.
Comment 4 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:52:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/982d07833360
Comment 5 Ryan VanderMeulen [:RyanVM] 2013-07-03 11:35:49 PDT
https://hg.mozilla.org/mozilla-central/rev/982d07833360
Comment 6 Scoobidiver (away) 2013-07-10 08:53:45 PDT
Bug 760384 which depends on this bug is tracked for 23.0.

Note You need to log in before you can comment on or make changes to this bug.