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

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Andi [:andi]]

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()

Comment 1

[Andi [:andi]]

Attachment: 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/

Comment 2

[Andi [:andi]]

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/

Comment 3

[Andi [:andi]]

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/

Comment 4

[Andi [:andi]]

I've updated the patch since same issue, cursor was out of context before closing it.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 7

[Andi [:andi]]

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/

Comment 8

[Andi [:andi]]

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);
>>    }

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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/

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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/

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I had some suspicions about the other Cursors in this bug so I filed bug 1250325.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And it looks like we're poised to remove the GeckoSmsManager code anyway. Dep on bug 1250325 to remove code, likely dupe.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning because it looks like we'll remove this eventually.

Comment 15

[Richard Newman [:rnewman]]

GeckoSmsManager is gone.