709330 - AboutHomeContent.getRecommendedAddonsStream() resource leak because ZipFile was not closed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Chris Peterson [:cpeterson]]

Using a Nightly or local build with Android StrictMode enabled, logcat reports that AboutHomeContent.getRecommendedAddonsStream() does not call ZipFile.close().

I can repro these warnings on ICS, but not Gingerbread.

E/StrictMode( 2332): A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
E/StrictMode( 2332): java.lang.Throwable: Explicit termination method 'close' not called
E/StrictMode( 2332): 	at dalvik.system.CloseGuard.open(CloseGuard.java:184)
E/StrictMode( 2332): 	at java.util.zip.ZipFile.<init>(ZipFile.java:133)
E/StrictMode( 2332): 	at java.util.zip.ZipFile.<init>(ZipFile.java:103)
E/StrictMode( 2332): 	at org.mozilla.gecko.AboutHomeContent.getRecommendedAddonsStream(AboutHomeContent.java:174)
E/StrictMode( 2332): 	at org.mozilla.gecko.AboutHomeContent$4.run(AboutHomeContent.java:184)
E/StrictMode( 2332): 	at android.os.Handler.handleCallback(Handler.java:605)
E/StrictMode( 2332): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/StrictMode( 2332): 	at android.os.Looper.loop(Looper.java:137)
E/StrictMode( 2332): 	at org.mozilla.gecko.GeckoAppShell$LooperThread.run(GeckoAppShell.java:153)

E/StrictMode( 2332): A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
E/StrictMode( 2332): java.lang.Throwable: Explicit termination method 'close' not called
E/StrictMode( 2332): 	at dalvik.system.CloseGuard.open(CloseGuard.java:184)
E/StrictMode( 2332): 	at java.io.RandomAccessFile.<init>(RandomAccessFile.java:128)
E/StrictMode( 2332): 	at java.io.RandomAccessFile.<init>(RandomAccessFile.java:150)
E/StrictMode( 2332): 	at java.util.zip.ZipFile.<init>(ZipFile.java:130)
E/StrictMode( 2332): 	at java.util.zip.ZipFile.<init>(ZipFile.java:103)
E/StrictMode( 2332): 	at org.mozilla.gecko.AboutHomeContent.getRecommendedAddonsStream(AboutHomeContent.java:174)
E/StrictMode( 2332): 	at org.mozilla.gecko.AboutHomeContent$4.run(AboutHomeContent.java:184)
E/StrictMode( 2332): 	at android.os.Handler.handleCallback(Handler.java:605)
E/StrictMode( 2332): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/StrictMode( 2332): 	at android.os.Looper.loop(Looper.java:137)
E/StrictMode( 2332): 	at org.mozilla.gecko.GeckoAppShell$LooperThread.run(GeckoAppShell.java:153)

E/StrictMode( 2332): A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
E/StrictMode( 2332): java.lang.Throwable: Explicit termination method 'end' not called
E/StrictMode( 2332): 	at dalvik.system.CloseGuard.open(CloseGuard.java:184)
E/StrictMode( 2332): 	at java.util.zip.Inflater.<init>(Inflater.java:82)
E/StrictMode( 2332): 	at java.util.zip.ZipFile.getInputStream(ZipFile.java:270)
E/StrictMode( 2332): 	at org.mozilla.gecko.AboutHomeContent.getRecommendedAddonsStream(AboutHomeContent.java:176)
E/StrictMode( 2332): 	at org.mozilla.gecko.AboutHomeContent$4.run(AboutHomeContent.java:184)
E/StrictMode( 2332): 	at android.os.Handler.handleCallback(Handler.java:605)
E/StrictMode( 2332): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/StrictMode( 2332): 	at android.os.Looper.loop(Looper.java:137)
E/StrictMode( 2332): 	at org.mozilla.gecko.GeckoAppShell$LooperThread.run(GeckoAppShell.java:153)

Comment 1

[Chris Peterson [:cpeterson]]

You may need to apply the StrictMode patches from bug 708114 to repro the warnings.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 582439 [details] [diff] [review]
patch

drive by:


+            if (zip == null)
+                return null;

and

+            if (fileEntry == null)
+                return null;

and

+            if (zip != null)

Don't need those if.  Its java, after all.  Just add a catch that returns null.


+        try {
+            zip = new ZipFile(applicationPackage);
+            ZipEntry fileEntry = zip.getEntry("recommended-addons.json");
+            return zip.getInputStream(fileEntry);
+        } finally {
+            if (zip != null)
+                zip.close();
+        } catch (Exception) {
+            return null;
+        }

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/43fd5d2ede79

> Don't need those if.  Its java, after all.  Just add a catch that returns null.
I don't like throwing unnecessary exceptions

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43fd5d2ede79

Comment 6

[Chris Peterson [:cpeterson]]

@blassey:

1. I don't think your code change will work. When your finally block calls zip.close(), the zip's InputStream will also be closed. This method will always return a closed (and useless) InputStream.

http://docs.oracle.com/javase/1.5.0/docs/api/java/util/zip/ZipFile.html#close%28%29

2. Java |new| does not return null. You do not need to check if (zip == null).