738421 - java.lang.Throwable: Explicit termination method 'close' not called @ GeckoJarReader.java:33

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

E/StrictMode(10704): A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
E/StrictMode(10704): java.lang.Throwable: Explicit termination method 'close' not called
E/StrictMode(10704): 	at dalvik.system.CloseGuard.open(CloseGuard.java:184)
E/StrictMode(10704): 	at java.util.zip.ZipFile.<init>(ZipFile.java:133)
E/StrictMode(10704): 	at java.util.zip.ZipFile.<init>(ZipFile.java:103)
E/StrictMode(10704): 	at org.mozilla.gecko.GeckoJarReader.getStream(GeckoJarReader.java:33)
E/StrictMode(10704): 	at org.mozilla.gecko.Favicons$LoadFaviconTask.downloadFavicon(Favicons.java:300)
E/StrictMode(10704): 	at org.mozilla.gecko.Favicons$LoadFaviconTask.doInBackground(Favicons.java:383)
E/StrictMode(10704): 	at org.mozilla.gecko.Favicons$LoadFaviconTask.doInBackground(Favicons.java:226)
E/StrictMode(10704): 	at android.os.AsyncTask$2.call(AsyncTask.java:264)
E/StrictMode(10704): 	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:305)
E/StrictMode(10704): 	at java.util.concurrent.FutureTask.run(FutureTask.java:137)
E/StrictMode(10704): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
E/StrictMode(10704): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
E/StrictMode(10704): 	at java.lang.Thread.run(Thread.java:856)

--
Tested via
Galaxy Nexus (Android 4.0.3)
20120322114020
http://hg.mozilla.org/integration/mozilla-inbound/rev/7e6750f282cc

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Lets close this stuff! I'm not really sure what to do if this fails...

Comment 2

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 608492 [details] [diff] [review]
Patch

Review of attachment 608492 [details] [diff] [review]:
-----------------------------------------------------------------

I am fine with the first part, except for indentation in 'catch' block.

The zip file part can better be like..

Zipfile zip;
try {
  ... 
} catch {
} finally {
  if (zip != null)
    zip.close();
}

r+ with these changes.

Comment 3

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bad9f42cdd0f

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/bad9f42cdd0f

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

So turned out that was a stupid patch I think. We can't close the stream or zip file until we've got the data we need. Basically that means we can't return a stream unless we copy the data into something like a ByteArrayStream or something. I figured at that point, its easier to just have the method return the BitmapDrawable we actually want.

If we ever have callers who want something more advanced, we can either expand or expose openZip, getStream, etc. and callers can be responsible for closing streams when they're done.

cpeterson found this, so I thought I'd let him tell me what else I'm doing wrong here.

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 608766 [details] [diff] [review]
Patch v2

Review of attachment 608766 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Favicons.java
@@ +299,5 @@
> +                if (url.startsWith("jar:jar:")) {
> +                    image = GeckoJarReader.getBitmapDrawable(url);
> +                } else {
> +                    Log.e(LOGTAG, "Error downloading favicon", e);
> +                }

Why not check for "jar:jar:" URLs in doInBackground() before calling downloadFavicon()? Reading from the jar is not downloading.

You would need to move saveFaviconToDb() from downloadFavicon() to doInBackground() (so favicon from jars can be saved to the DB too). Saving a favicon to the DB does not seem an appropriate side effect of a method called `downloadFavicon()`.

::: mobile/android/base/GeckoJarReader.java
@@ +23,5 @@
>   */
>  public class GeckoJarReader {
>      static private String LOGTAG = "GeckoJarReader";
>  
> +    static public BitmapDrawable getBitmapDrawable(String url) {

s/static public/public static/

Java coding style recommends that access modifiers (public) should proceed static modifier:
http://checkstyle.sourceforge.net/config_modifier.html

@@ +42,5 @@
> +                    bitmap = new BitmapDrawable(inputStream);
> +                    inputStream.close();
> +                }
> +                if (zip != null) {
> +                    zip.close();

If `new BitmapDrawable()` or `inputStream.close()` throw, then zip.close() won't be called.

@@ +52,5 @@
>  
> +        return bitmap;
> +    }
> +
> +    static private ZipFile getZipFile(String url) throws IOException {

s/static private/private static/

@@ +58,5 @@
> +        File file = new File(fileUrl.getPath());
> +        return new ZipFile(file);
> +    }
> +
> +    static private InputStream getStream(ZipFile zip, Stack<String> jarUrls) throws IOException {

s/static private/private static/

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Thanks!

Comment 8

[Wesley Johnston (:wesj)]

I saw another favicon error come by here for a local file:

java.io.FileNotFoundException

I guess I should catch and smother that one too, but I'm leery to just catch and ignore all errors without logging their signature somehow.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 608803 [details] [diff] [review]
Patch v3

r+

Should this bug be nom'd for aurora?

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71f36268b7de

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71f36268b7de