[Static Analysis][Resource leak] In Function createThreadCursor::run and getThreadFromCursorAndNotify

RESOLVED INVALID

Status

()

Firefox for Android
General
RESOLVED INVALID
2 years ago
7 months ago

People

(Reporter: andi, Unassigned)

Tracking

(Blocks: 1 bug, {coverity})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: CID 119596, CID 119595 )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The Static Analysis tool Coverity added cursor has memory leak in the following block code:

>>          if (cursor == null || !cursor.moveToFirst()) {
>>            notifyCursorDone(mRequestId);
>>            return;
>>          }

this could happen only if the second part of if evaluates to true. If so we should call cursor.close()
(Reporter)

Updated

2 years ago
Summary: [Static Analysis][Resource leak] In Function createThreadCursor → [Static Analysis][Resource leak] In Function createThreadCursor::run
(Reporter)

Comment 1

2 years ago
Created attachment 8718375 [details]
MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/34535/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34535/
Attachment #8718375 - Flags: review?(mark.finkle)
(Reporter)

Comment 2

2 years ago
Comment on attachment 8718375 [details]
MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34535/diff/1-2/
(Reporter)

Comment 3

2 years ago
Comment on attachment 8718375 [details]
MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34535/diff/2-3/
Attachment #8718375 - Attachment description: MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run. r?mfinkle → MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mfinkle
(Reporter)

Updated

2 years ago
Summary: [Static Analysis][Resource leak] In Function createThreadCursor::run → [Static Analysis][Resource leak] In Function createThreadCursor::run and getThreadFromCursorAndNotify
Whiteboard: CID 119596 → CID 119596, CID 119595
(Reporter)

Comment 4

2 years ago
I've updated the patch since same issue, cursor was out of context before closing it.
Comment on attachment 8718375 [details]
MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella

Passing to Mike. He's a better reviewer for this.
Attachment #8718375 - Flags: review?(mark.finkle) → review?(michael.l.comella)
Comment on attachment 8718375 [details]
MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella

https://reviewboard.mozilla.org/r/34535/#review31797

::: mobile/android/base/java/org/mozilla/gecko/GeckoSmsManager.java:1068
(Diff revision 3)
>      msgCursor = cr.query(kSmsContentUri,

We reassign the cursor here without closing the previous cursor – can you make sure that gets closed? It should be in a `try {} finally { Cursor.close(); }` style block.

::: mobile/android/base/java/org/mozilla/gecko/GeckoSmsManager.java:1082
(Diff revision 3)
> +    msgCursor.close();

As per the other comment, this should be in a `try { } finally { Cursor.close(); }` block. It might be easier to use two separate variables for the Cursors.

::: mobile/android/base/java/org/mozilla/gecko/GeckoSmsManager.java:1104
(Diff revision 3)
> +            if (cursor != null )

This should also be in a `finally` block but I'm not sure how feasible it is in this case.
Attachment #8718375 - Flags: review?(michael.l.comella)
(Reporter)

Comment 7

2 years ago
Comment on attachment 8718375 [details]
MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34535/diff/3-4/
Attachment #8718375 - Attachment description: MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mfinkle → MozReview Request: Bug 1247636 - close cursor before returning from createThreadCursor::run and getThreadFromCursorAndNotify. r?mcomella
Attachment #8718375 - Flags: review?(michael.l.comella)
(Reporter)

Comment 8

2 years ago
Hello Michael i've updated the bug with your suggestions but regarding your 3rd comment:
> +            if (cursor != null )
I don;t think query returns any exceptions:
>>    public final @Nullable Cursor query(@NonNull Uri uri, @Nullable String[] projection,
>>            @Nullable String selection, @Nullable String[] selectionArgs,
>>            @Nullable String sortOrder) {
>>        return query(uri, projection, selection, selectionArgs, sortOrder, null);
>>    }
Created attachment 8722207 [details]
MozReview Request: Bug 1247636 - Close Cursors in GeckoSmsManager.getThreadFromCursorAndNotify. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/35901/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35901/
Attachment #8722207 - Flags: review?(s.kaspari)
Assignee: bogdan.postelnicu → michael.l.comella
Attachment #8718375 - Attachment is obsolete: true
Attachment #8718375 - Flags: review?(michael.l.comella)
Comment on attachment 8722207 [details]
MozReview Request: Bug 1247636 - Close Cursors in GeckoSmsManager.getThreadFromCursorAndNotify. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35901/diff/1-2/
Attachment #8722207 - Attachment description: MozReview Request: Bug 1247636 - Close Cursors in GeckoSmsManager. r=sebastian → MozReview Request: Bug 1247636 - Close Cursors in GeckoSmsManager.getThreadFromCursorAndNotify. r=sebastian
Created attachment 8722208 [details]
MozReview Request: Bug 1247636 - Close Cursor in GeckoSmsManager.createThreadCursor. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/35903/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35903/
Attachment #8722208 - Flags: review?(s.kaspari)
I had some suspicions about the other Cursors in this bug so I filed bug 1250325.
See Also: → bug 1250325
And it looks like we're poised to remove the GeckoSmsManager code anyway. Dep on bug 1250325 to remove code, likely dupe.
Depends on: 1250325
Depends on: 1153560
No longer depends on: 1250325
Unassigning because it looks like we'll remove this eventually.
Assignee: michael.l.comella → nobody
GeckoSmsManager is gone.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.