Closed Bug 738421 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14

People

(Reporter: aaronmt, Assigned: wesj)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch Patch (obsolete) — Splinter Review
Lets close this stuff! I'm not really sure what to do if this fails...
Assignee: nobody → wjohnston
Attachment #608492 - Flags: review?(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.
Attachment #608492 - Flags: review?(sriram) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/bad9f42cdd0f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #608492 - Attachment is obsolete: true
Attachment #608766 - Flags: review?(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/
Attachment #608766 - Flags: review?(cpeterson) → review-
Attached patch Patch v3Splinter Review
Thanks!
Attachment #608766 - Attachment is obsolete: true
Attachment #608803 - Flags: review?(cpeterson)
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 on attachment 608803 [details] [diff] [review]
Patch v3

r+

Should this bug be nom'd for aurora?
Attachment #608803 - Flags: review?(cpeterson) → review+
You need to log in before you can comment on or make changes to this bug.