1246839 - Fix resource leaks from Coverity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

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

Attachment: MozReview Request: Bug 1246839 - Close FileOutputStream in BrowserApp. r=margaret

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

Comment 2

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

Attachment: MozReview Request: Bug 1246839 - Close Reader in ApkResources. r=margaret

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

Comment 3

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

Attachment: MozReview Request: Bug 1246839 - Close Reader in ApkResources. r=margaret

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

Comment 4

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

Attachment: MozReview Request: Bug 1246839 - Close DB in SharedBrowserDatabaseProvider. r=margaret

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

Comment 5

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/34101/#review30797

https://groups.google.com/forum/#!msg/android-developers/NwDRpHUXt0U/jIam4Q8-cqQJ

"A content provider is created when its hosting process is created, and remains around for as long as the process does, so there is no need to close the database -- it will get closed as part of the kernel cleaning up the process's resources when the process is killed."

-- Dianne Hackborn

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8717267 [details]
MozReview Request: Bug 1246839 - Close Reader in ApkResources. r=margaret

https://reviewboard.mozilla.org/r/34103/#review30799

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3279
(Diff revision 1)
> +                                    } catch (final IOException e) { /* Nothing to do here. */ }

Just push this into the enclosing `catch`.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8717268 [details]
MozReview Request: Bug 1246839 - Close Reader in ApkResources. r=margaret

https://reviewboard.mozilla.org/r/34105/#review30801

Comment 8

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/34107/#review30803

::: mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider.java:111
(Diff revision 1)
> -        final Cursor cursor = db.query(tableName, new String[] { CommonColumns._ID }, selection, null, null, null, null, limit);
> +            final Cursor cursor = db.query(tableName, new String[]{CommonColumns._ID}, selection, null, null, null, null, limit);

Our style is still to keep whitespace in this situation. Makes things easier to read.

::: mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider.java:121
(Diff revision 1)
> +            db.close();

Again, ContentProviders shouldn't close their database connections.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8717269 [details]
MozReview Request: Bug 1246839 - Close DB in SharedBrowserDatabaseProvider. r=margaret

If you want to keep Coverity happy, you can implement ContentProvider.shutdown(), and shut down any open connections there.

Note that shutdown is an API11+ method, and is ordinarily only called from tests. Do not call super.shutdown(), and try not to open a connection only to close it again!

Comment 10

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

https://reviewboard.mozilla.org/r/34103/#review30799

> Just push this into the enclosing `catch`.

Pushing it into the enclosing catch would result in cleaner code but it's less safe for future changes and less obvious who might be throwing the Exception. I'm going to leave it the way it is.

Comment 11

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

(In reply to Richard Newman [:rnewman] from comment #9)
> If you want to keep Coverity happy, you can implement
> ContentProvider.shutdown(), and shut down any open connections there.

The javadoc indicates this is really a test method. Changing our code with hacks to keep the static analyzer happy seems like a waste so I'll just ignore the issues in Coverity.

Comment 12

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

https://reviewboard.mozilla.org/r/34107/#review30803

> Our style is still to keep whitespace in this situation. Makes things easier to read.

/me shakes fist at IntelliJ!

Comment 13

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

https://reviewboard.mozilla.org/r/34101/#review30797

After discussing this with Newman and reading some source code, this makes sense. Assuming from the above comment that the db gets cleaned up when the process is killed and the resources used by an open DB handle are negligble, the `SQLiteOpenHelper` that backs the CP will cache an open database connection and each time `getWriteableDatabase` or `getReadableDatabase` is called, the cached instance is returned. Thus, when `close` is called, the backing DB is called and all open handles are invalidated. Since there's no reference counting, there's no need to close each DB instance.

This is why it makes sense for an application to access the DB through a CP – it doesn't have to manage opening/closing the DB resources itself, a process which typically can cause leaks. Instead, it relies on the lifecycle of the CP to manage the DB state.

Since these handles are accessed in CPs, I agree that the closing calls are unnecessary.

It's also worth mentioning that it's less efficient to close the DB each time because there is some work done to re-open it – might as well make it faster (and simplify the code!) by not closing the connections.

Comment 14

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

Comment on attachment 8717266 [details]
MozReview Request: Bug 1246839 - Close FileOutputStream in BrowserApp. r=margaret

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

Comment 15

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

Comment on attachment 8717267 [details]
MozReview Request: Bug 1246839 - Close Reader in ApkResources. r=margaret

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

Comment 16

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

Comment on attachment 8717266 [details]
MozReview Request: Bug 1246839 - Close FileOutputStream in BrowserApp. r=margaret

Newman already r+'d this one.

Comment 17

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

https://hg.mozilla.org/integration/fx-team/rev/49eb5cc4bcbfef4a65cff9820bae21bee0a8c26e
Bug 1246839 - Close FileOutputStream in BrowserApp. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/2fd796bc695a87e4eee45e4ad2388e063ac5de3a
Bug 1246839 - Close Reader in ApkResources. r=rnewman

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49eb5cc4bcbf
https://hg.mozilla.org/mozilla-central/rev/2fd796bc695a

Comment 19

[:Margaret Leibovic]

You should make a post to mobile-firefox-dev about Coverity. Maybe I missed a memo, but I don't have context around these changes.

Comment 20

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

(In reply to :Margaret Leibovic from comment #19)
> You should make a post to mobile-firefox-dev about Coverity. Maybe I missed
> a memo, but I don't have context around these changes.

afaict, you have to be a member of our Coverity project in order to see the defects which may or may not be available to the community. I posted internally, but if someone wants to do more research about how open it is, we could forward that to the larger group.