1253555 - [Static Analysis][Resource leak] In function BrowserProvider::getTopSites

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable |suggestedSitesCursor| can leak memory since it's not clased.
|suggestedSitesCursor| is allocated here:

>> final Cursor suggestedSitesCursor = suggestedSites.get(suggestedGridLimit);

Following the call path function that allocates object is: 

>>    public synchronized Cursor get(int limit, Locale locale, List<String> excludeUrls) {
>>        final MatrixCursor cursor = new MatrixCursor(COLUMNS);

|suggestedSitesCursor| should be closed in final block.

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?ahunt

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8726660 [details]
MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?ahunt

I'll redirect this review to ahunt. He has been working on this part of the code recently. Just to avoid conflicts.

Comment 3

[Andrzej Hunt :ahunt]

Comment on attachment 8726660 [details]
MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?ahunt

https://reviewboard.mozilla.org/r/38147/#review34689

Good catch! One minor nit below:

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:789
(Diff revision 1)
>  

This is where we should probably close the cursor instead.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:895
(Diff revision 1)
> +            if (suggestedSitesCursor != null) {

Can we move this up the method to where we're actually working with the cursor? (I've placed a comment where I think we should do the closing.)

We don't need the nullcheck since SuggestedSites.get() will always return a cursor (admittedly the docs aren't too clear on that). Moreover we're already accessing the cursor without a nullcheck, so we're guaranteed to have a non-null cursor at this point.

Note: this is actually a MatrixCursor, so we're not holding onto any SQL resources (that's entirely an implementation detail though, so we shouldn't rely on that) - closing the cursor also removes all ContentObservers etc.

Note 2: I'm actually going to be removing this cursor soon in Bug 1249018! Sorry that this won't live too long!

Comment 4

[Andi [:andi]]

Comment on attachment 8726660 [details]
MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?ahunt

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

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8726660 [details]
MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?ahunt

https://reviewboard.mozilla.org/r/38147/#review35903

Looks great.

Thank you!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0609edede2a

Comment 7

[Carsten Book [:Tomcat]]

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