Closed Bug 1253555 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 123699)

Attachments

(1 file)

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 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.
Attachment #8726660 - Flags: review?(s.kaspari) → review?(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!
Attachment #8726660 - Flags: review?(ahunt)
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/
Attachment #8726660 - Attachment description: MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?sebastian → MozReview Request: Bug 1253555 - avoid memory leak on variable |suggestedSitesCursor|. r?ahunt
Attachment #8726660 - Flags: review?(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!
Attachment #8726660 - Flags: review?(ahunt) → review+
https://hg.mozilla.org/mozilla-central/rev/d0609edede2a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.