1247557 - [Static Analysis][Resource leak] In function query (Uri mainUri, Uri fallbackUri, String condition)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that a memory leak can occur in the following code:

>>        Cursor cursor = mCr.query(mainUri, null, condition, null, null);
>>
>>        if (Build.MANUFACTURER.equals(SAMSUNG_MANUFACTURER) && (cursor == null || cursor.getCount() == 0)) {
>>            cursor = mCr.query(fallbackUri, null, null, null, null);
>>        }

I think this is possible only if cursor is a valid object. We should do a check inside the if branch for cursor validity and if so close it.

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

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

Comment 2

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

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Passing to Mike. He's a better reviewer for this.

Comment 3

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

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

https://reviewboard.mozilla.org/r/34487/#review31791

Looks good – just a small style nit.

Thanks for the patch!

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:158
(Diff revision 1)
> +            if ( cursor != null )
> +                cursor.close();

nit: Our style is `if (expression) {`. Also, we always brace if statements.

Comment 4

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

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

https://reviewboard.mozilla.org/r/34487/#review31795

Actually, this should really be in a `try {} finally { Cursor.close(); }` since it's safer. Can you figure out a way to shoe-horn the code into that? Perhaps with some temp variables.

Comment 5

[Andi [:andi]]

Hello Michael, i'm not a java expert but as far as i can see query doesn't throw any exception so i don't see the point of using try {} finally {}. The exceptions that can be triggered inside query are handled locally by that function.
If there is something that i didn't get let me know.

Comment 6

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

(In reply to Andi-Bogdan Postelnicu from comment #5)
> Hello Michael, i'm not a java expert but as far as i can see query doesn't
> throw any exception so i don't see the point of using try {} finally {}. The
> exceptions that can be triggered inside query are handled locally by that
> function.
> If there is something that i didn't get let me know.

It's a defensive coding technique – there are no Exceptions thrown at the moment but will the code throw Exceptions after a future developer adds code? Additionally, the code can still throw RuntimeExceptions that don't need to be explicitly rethrown  by the method (e.g. NullPointerException), which the calling code can (questionably) catch and recover from, in which case the resource would be left open.

Comment 7

[Andi [:andi]]

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

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

Comment 8

[Andi [:andi]]

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

https://reviewboard.mozilla.org/r/34487/#review32091

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:161
(Diff revision 3)
> -            cursor = mCr.query(fallbackUri, null, null, null, null);
> +                cursorSecond = mCr.query(fallbackUri, null, null, null, null);

Isn't this the only situation where you need to close the previous cursor if it is not null? The code might be simpler if you close it here.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:164
(Diff revision 3)
> +        catch (Exception e) {
> +            Log.e(LOGTAG, "Exception while querying db: ", e);
> +        }

You can have try { } finaly { } blocks without catching an exception.

Comment 10

[Andi [:andi]]

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34487/diff/3-4/

Comment 11

[Andi [:andi]]

Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34487/diff/4-5/

Comment 12

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

Hey Andi-Bogdan.

bug 1247636 and this one are some pretty complex error cases and, as you mentioned you were not a Java expert, I'd like to avoid the back-and-forth review cycle if it bothers you. Would you like me to take over finishing these bugs?

In any case, I really appreciate your help and your watch over our Coverity defects! :)

Comment 13

[Andi [:andi]]

Hey Michael,

No problem for me, do as you consider it's best.

Comment 14

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

Attachment: MozReview Request: Bug 1247557 - Close Cursors in AndroidImport. r=sebastian

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

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8722200 [details]
MozReview Request: Bug 1247557 - Close Cursors in AndroidImport. r=sebastian

https://reviewboard.mozilla.org/r/35897/#review32663

I don't think the catch is needed, see below. Without that r+. :)

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:165
(Diff revision 1)
> +        } catch (final Exception e) {
> +            if (cursor != null) {
> +                cursor.close();
> +            }
> +            throw e;
> +        }

Isn't this already (and shouldn't this) be handled by the callers? They all wrap their calls to query() in try..finally and always close the cursor.

Comment 16

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

https://reviewboard.mozilla.org/r/35897/#review32663

> Isn't this already (and shouldn't this) be handled by the callers? They all wrap their calls to query() in try..finally and always close the cursor.

But if `query` throws, the `Cursor` will not be returned so the callers can't close it.

Comment 17

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

https://reviewboard.mozilla.org/r/35897/#review32663

> But if `query` throws, the `Cursor` will not be returned so the callers can't close it.

Spoke with rnewman – the catch complicates the code path so much more than the probability that someone will come along and change the code, miss the leaked Cursor in review, and we won't catch it again in static analysis that it's not worth adding in the extra lines.

Comment 18

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

https://hg.mozilla.org/integration/fx-team/rev/7a2db11cc81934507f730ae24429a41be6202438
Bug 1247557 - Close Cursors in AndroidImport. r=sebastian

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a2db11cc819