Andi [:andi] Assignee
	Description • 9 years ago

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.

	Andi [:andi] Assignee
	Comment 1 • 9 years ago

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

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 2 • 9 years ago

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.

	Andrzej Hunt :ahunt
	Comment 3 • 9 years ago

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!

	Andi [:andi] Assignee
	Comment 4 • 8 years ago

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/

	Andrzej Hunt :ahunt
	Comment 5 • 8 years ago

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!

	Pulsebot
	Comment 6 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 7 • 8 years ago
bugherder

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