Improve detection of CloseGuard protected resource leakage

* Add CloseGuardMonitor to intercept and collate CloseGuard reports and if
  necessary throw an exception listing the resource leaks.
* Add ResourceLeakageDetector to abstract away the CloseGuardMonitor which will
  not work on RI.
* Add AbstractResourceLeakageDetectorTestCase as a base class for tests
  that need to detect resource leaks, in future this could be handled by
  modifications to Cts and Vogar test runners.
* Remove CloseGuardTester and its sole usage in ProcessBuilderTest.
* Remove CloseGuardGuard from within URLConnectionTest
* Change ZipFileTest, ProcessBuilderTest, URLConnectionTest to use new
  mechanism, fix issues that are identified and do some cleanup/remove
  duplicated code.

Bug: https://code.google.com/p/android/issues/detail?id=66383
Change-Id: Id026dbb6bc66091a15f07329e6371cd0d1f32cf5

8 files changed, 365 insertions, 127 deletions

diff --git a/dalvik/src/test/java/dalvik/system/CloseGuardMonitor.java b/dalvik/src/test/java/dalvik/system/CloseGuardMonitor.java
new file mode 100644
index 0000000..b5bf380
--- /dev/null
+++ b/dalvik/src/test/java/dalvik/system/CloseGuardMonitor.java
@@ -0,0 +1,119 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package dalvik.system;
+
+import dalvik.system.CloseGuard.Reporter;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.lang.ref.WeakReference;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+/**
+ * Provides support for detecting issues found by {@link CloseGuard} from within tests.
+ *
+ * <p>This is a best effort as it relies on both {@link CloseGuard} being enabled and being able to
+ * force a GC and finalization, none of which are directly controllable by this.
+ *
+ * <p>This is loaded using reflection by the AbstractResourceLeakageDetectorTestCase class as that
+ * class needs to run on the reference implementation which does not have this class. It implements
+ * {@link Runnable} because that is simpler than trying to manage a specialized interface.
+ *
+ * @hide
+ */
+public class CloseGuardMonitor implements Runnable {
+  /**
+   * The {@link Reporter} instance used to receive warnings from {@link CloseGuard}.
+   */
+  private final Reporter closeGuardReporter;
+
+  /**
+   * The list of allocation sites that {@link CloseGuard} has reported as not being released.
+   *
+   * <p>Is thread safe as this will be called during finalization and so there are no guarantees
+   * as to whether it will be called concurrently or not.
+   */
+  private final List<Throwable> closeGuardAllocationSites = new CopyOnWriteArrayList<>();
+
+  /**
+   * Default constructor required for reflection.
+   */
+  public CloseGuardMonitor() {
+    System.logI("Creating CloseGuard monitor");
+
+    // Save current reporter.
+    closeGuardReporter = CloseGuard.getReporter();
+
+    // Override the reporter with our own which collates the allocation sites.
+    CloseGuard.setReporter(new Reporter() {
+      @Override
+      public void report(String message, Throwable allocationSite) {
+        // Ignore message as it's always the same.
+        closeGuardAllocationSites.add(allocationSite);
+      }
+    });
+  }
+
+  /**
+   * Check to see whether any resources monitored by {@link CloseGuard} were not released before
+   * they were garbage collected.
+   */
+  @Override
+  public void run() {
+    // Create a weak reference to an object so that we can detect when it is garbage collected.
+    WeakReference<Object> reference = new WeakReference<>(new Object());
+
+    try {
+      // 'Force' a GC and finalize to cause CloseGuards to report warnings. Doesn't loop
+      // forever as there are no guarantees that the following code does anything at all so
+      // don't want a potential infinite loop.
+      Runtime runtime = Runtime.getRuntime();
+      for (int i = 0; i < 20; ++i) {
+        runtime.gc();
+        System.runFinalization();
+        try {
+          Thread.sleep(1);
+        } catch (InterruptedException e) {
+          throw new AssertionError(e);
+        }
+
+        // Check to see if the weak reference has been garbage collected.
+        if (reference.get() == null) {
+          System.logI("Sentry object has been freed so assuming CloseGuards have reported"
+              + " any resource leakages");
+          break;
+        }
+      }
+    } finally {
+      // Restore the reporter.
+      CloseGuard.setReporter(closeGuardReporter);
+    }
+
+    if (!closeGuardAllocationSites.isEmpty()) {
+      StringWriter writer = new StringWriter();
+      PrintWriter printWriter = new PrintWriter(writer);
+      int i = 0;
+      for (Throwable allocationSite : closeGuardAllocationSites) {
+        printWriter.print(++i);
+        printWriter.print(") ");
+        allocationSite.printStackTrace(printWriter);
+        printWriter.println("    --------------------------------");
+      }
+      throw new AssertionError("Potential resource leakage detected:\n" + writer);
+    }
+  }
+}
diff --git a/luni/src/main/java/java/util/zip/ZipFile.java b/luni/src/main/java/java/util/zip/ZipFile.java
index 4b3e431..43e8567 100644
--- a/luni/src/main/java/java/util/zip/ZipFile.java
+++ b/luni/src/main/java/java/util/zip/ZipFile.java
@@ -33,6 +33,7 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import libcore.io.BufferIterator;
 import libcore.io.HeapBufferIterator;
+import libcore.io.IoUtils;
 import libcore.io.Streams;
 
 /**
@@ -199,7 +200,19 @@ public class ZipFile implements Closeable, ZipConstants {
 
         raf = new RandomAccessFile(filename, "r");
 
-        readCentralDir();
+        // Make sure to close the RandomAccessFile if reading the central directory fails.
+        boolean mustCloseFile = true;
+        try {
+            readCentralDir();
+
+            // Read succeeded so do not close the underlying RandomAccessFile.
+            mustCloseFile = false;
+        } finally {
+            if (mustCloseFile) {
+                IoUtils.closeQuietly(raf);
+            }
+        }
+
         guard.open("close");
     }
 
diff --git a/luni/src/test/java/libcore/java/lang/ProcessBuilderTest.java b/luni/src/test/java/libcore/java/lang/ProcessBuilderTest.java
index d03ae65..9766cef 100644
--- a/luni/src/test/java/libcore/java/lang/ProcessBuilderTest.java
+++ b/luni/src/test/java/libcore/java/lang/ProcessBuilderTest.java
@@ -22,10 +22,10 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.HashMap;
 import java.util.Map;
-import libcore.dalvik.system.CloseGuardTester;
+import libcore.java.util.AbstractResourceLeakageDetectorTestCase;
 import static tests.support.Support_Exec.execAndCheckOutput;
 
-public class ProcessBuilderTest extends junit.framework.TestCase {
+public class ProcessBuilderTest extends AbstractResourceLeakageDetectorTestCase {
 
     private static String shell() {
         String deviceSh = "/system/bin/sh";
@@ -84,14 +84,8 @@ public class ProcessBuilderTest extends junit.framework.TestCase {
     }
 
     public void testDestroyDoesNotLeak() throws IOException {
-        CloseGuardTester closeGuardTester = new CloseGuardTester();
-        try {
-            Process process = new ProcessBuilder(shell(), "-c", "echo out; echo err 1>&2").start();
-            process.destroy();
-            closeGuardTester.assertEverythingWasClosed();
-        } finally {
-            closeGuardTester.close();
-        }
+        Process process = new ProcessBuilder(shell(), "-c", "echo out; echo err 1>&2").start();
+        process.destroy();
     }
 
     public void testEnvironmentMapForbidsNulls() throws Exception {
diff --git a/luni/src/test/java/libcore/java/net/URLConnectionTest.java b/luni/src/test/java/libcore/java/net/URLConnectionTest.java
index b40976e..e945e60 100644
--- a/luni/src/test/java/libcore/java/net/URLConnectionTest.java
+++ b/luni/src/test/java/libcore/java/net/URLConnectionTest.java
@@ -21,9 +21,7 @@ import com.google.mockwebserver.MockResponse;
 import com.google.mockwebserver.MockWebServer;
 import com.google.mockwebserver.RecordedRequest;
 import com.google.mockwebserver.SocketPolicy;
-import dalvik.system.CloseGuard;
 import java.io.ByteArrayOutputStream;
-import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -31,7 +29,6 @@ import java.io.OutputStream;
 import java.net.Authenticator;
 import java.net.CacheRequest;
 import java.net.CacheResponse;
-import java.net.ConnectException;
 import java.net.HttpRetryException;
 import java.net.HttpURLConnection;
 import java.net.InetAddress;
@@ -68,9 +65,8 @@ import javax.net.ssl.SSLSession;
 import javax.net.ssl.SSLSocketFactory;
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.X509TrustManager;
-import junit.framework.TestCase;
-import libcore.java.lang.ref.FinalizationTester;
 import libcore.java.security.TestKeyStore;
+import libcore.java.util.AbstractResourceLeakageDetectorTestCase;
 import libcore.javax.net.ssl.TestSSLContext;
 import tests.net.StuckServer;
 
@@ -80,7 +76,8 @@ import static com.google.mockwebserver.SocketPolicy.FAIL_HANDSHAKE;
 import static com.google.mockwebserver.SocketPolicy.SHUTDOWN_INPUT_AT_END;
 import static com.google.mockwebserver.SocketPolicy.SHUTDOWN_OUTPUT_AT_END;
 
-public final class URLConnectionTest extends TestCase {
+public final class URLConnectionTest extends AbstractResourceLeakageDetectorTestCase {
+
     private MockWebServer server;
     private HttpResponseCache cache;
     private String hostName;
@@ -101,8 +98,10 @@ public final class URLConnectionTest extends TestCase {
         System.clearProperty("https.proxyHost");
         System.clearProperty("https.proxyPort");
         server.shutdown();
+        server = null;
         if (cache != null) {
             cache.delete();
+            cache = null;
         }
         super.tearDown();
     }
@@ -797,47 +796,17 @@ public final class URLConnectionTest extends TestCase {
     }
 
     public void testDisconnectAfterOnlyResponseCodeCausesNoCloseGuardWarning() throws IOException {
-        CloseGuardGuard guard = new CloseGuardGuard();
-        try {
-            server.enqueue(new MockResponse()
-                           .setBody(gzip("ABCABCABC".getBytes("UTF-8")))
-                           .addHeader("Content-Encoding: gzip"));
-            server.play();
+        server.enqueue(new MockResponse()
+                .setBody(gzip("ABCABCABC".getBytes("UTF-8")))
+                .addHeader("Content-Encoding: gzip"));
+        server.play();
 
-            HttpURLConnection connection = (HttpURLConnection) server.getUrl("/").openConnection();
+        HttpURLConnection connection = (HttpURLConnection) server.getUrl("/").openConnection();
+        try {
             assertEquals(200, connection.getResponseCode());
-            connection.disconnect();
-            connection = null;
-            assertFalse(guard.wasCloseGuardCalled());
         } finally {
-            guard.close();
-        }
-    }
-
-    public static class CloseGuardGuard implements Closeable, CloseGuard.Reporter  {
-        private final CloseGuard.Reporter oldReporter = CloseGuard.getReporter();
-
-        private AtomicBoolean closeGuardCalled = new AtomicBoolean();
-
-        public CloseGuardGuard() {
-            CloseGuard.setReporter(this);
-        }
-
-        @Override public void report(String message, Throwable allocationSite) {
-            oldReporter.report(message, allocationSite);
-            closeGuardCalled.set(true);
-        }
-
-        public boolean wasCloseGuardCalled() {
-            FinalizationTester.induceFinalization();
-            close();
-            return closeGuardCalled.get();
-        }
-
-        @Override public void close() {
-            CloseGuard.setReporter(oldReporter);
+            connection.disconnect();
         }
-
     }
 
     public void testDefaultRequestProperty() throws Exception {
@@ -2272,11 +2241,15 @@ public final class URLConnectionTest extends TestCase {
         HttpsURLConnection connection = (HttpsURLConnection) server.getUrl("/").openConnection();
         connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory());
         connection.connect();
-        assertNotNull(connection.getHostnameVerifier());
-        assertNull(connection.getLocalCertificates());
-        assertNotNull(connection.getServerCertificates());
-        assertNotNull(connection.getCipherSuite());
-        assertNotNull(connection.getPeerPrincipal());
+        try {
+            assertNotNull(connection.getHostnameVerifier());
+            assertNull(connection.getLocalCertificates());
+            assertNotNull(connection.getServerCertificates());
+            assertNotNull(connection.getCipherSuite());
+            assertNotNull(connection.getPeerPrincipal());
+        } finally {
+            connection.disconnect();
+        }
     }
 
     /**
diff --git a/luni/src/test/java/libcore/java/util/AbstractResourceLeakageDetectorTestCase.java b/luni/src/test/java/libcore/java/util/AbstractResourceLeakageDetectorTestCase.java
new file mode 100644
index 0000000..5ea67d3
--- /dev/null
+++ b/luni/src/test/java/libcore/java/util/AbstractResourceLeakageDetectorTestCase.java
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package libcore.java.util;
+
+import junit.framework.TestCase;
+
+/**
+ * Ensures that resources used within a test are cleaned up; will detect problems with tests and
+ * also with runtime.
+ */
+public abstract class AbstractResourceLeakageDetectorTestCase extends TestCase {
+  /**
+   * The leakage detector.
+   */
+  private ResourceLeakageDetector detector;
+
+  @Override
+  protected void setUp() throws Exception {
+    detector = ResourceLeakageDetector.newDetector();
+  }
+
+  @Override
+  protected void tearDown() throws Exception {
+    // If available check for resource leakage. At this point it is impossible to determine
+    // whether the test has thrown an exception. If it has then the exception thrown by this
+    // could hide that test failure; it largely depends on the test runner.
+    if (detector != null) {
+      detector.checkForLeaks();
+    }
+  }
+}
diff --git a/luni/src/test/java/libcore/java/util/ResourceLeakageDetector.java b/luni/src/test/java/libcore/java/util/ResourceLeakageDetector.java
new file mode 100644
index 0000000..954665a
--- /dev/null
+++ b/luni/src/test/java/libcore/java/util/ResourceLeakageDetector.java
@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package libcore.java.util;
+
+/**
+ * Detects resource leakages for resources that are protected by <code>CloseGuard</code> mechanism.
+ *
+ * <p>If multiple instances of this are active at the same time, i.e. have been created but not yet
+ * had their {@link #checkForLeaks()} method called then while they will report all the leakages
+ * detected they may report the leakages caused by the code being tested by another detector.
+ *
+ * <p>The underlying CloseGuardMonitor is loaded using reflection to ensure that this will run,
+ * albeit doing nothing, on the reference implementation.
+ */
+public class ResourceLeakageDetector {
+  /** The class for the CloseGuardMonitor, null if not supported. */
+  private static final Class<?> CLOSE_GUARD_MONITOR_CLASS;
+
+  static {
+    ClassLoader classLoader = ResourceLeakageDetector.class.getClassLoader();
+    Class<?> clazz;
+    try {
+      // Make sure that the CloseGuard class exists; this ensures that this is not running
+      // on a RI JVM.
+      classLoader.loadClass("dalvik.system.CloseGuard");
+
+      // Load the monitor class for later instantiation.
+      clazz = classLoader.loadClass("dalvik.system.CloseGuardMonitor");
+
+    } catch (ClassNotFoundException e) {
+      System.err.println("Resource leakage will not be detected; "
+          + "this is expected in the reference implementation");
+      e.printStackTrace(System.err);
+
+      // Ignore, probably running in reference implementation.
+      clazz = null;
+    }
+
+    CLOSE_GUARD_MONITOR_CLASS = clazz;
+  }
+
+  /**
+   * The underlying CloseGuardMonitor that will perform the post test checks for resource
+   * leakage.
+   */
+  private Runnable postTestChecker;
+
+  /**
+   * Create a new detector.
+   *
+   * @return The new {@link ResourceLeakageDetector}, its {@link #checkForLeaks()} method must be
+   * called otherwise it will not clean up properly after itself.
+   */
+  public static ResourceLeakageDetector newDetector()
+      throws Exception {
+    return new ResourceLeakageDetector();
+  }
+
+  private ResourceLeakageDetector()
+      throws Exception {
+    if (CLOSE_GUARD_MONITOR_CLASS != null) {
+      postTestChecker = (Runnable) CLOSE_GUARD_MONITOR_CLASS.newInstance();
+    }
+  }
+
+  /**
+   * Detect any leaks that have arisen since this was created.
+   *
+   * @throws Exception If any leaks were detected.
+   */
+  public void checkForLeaks() throws Exception {
+    // If available check for resource leakage.
+    if (postTestChecker != null) {
+      postTestChecker.run();
+    }
+  }
+}
diff --git a/luni/src/test/java/libcore/java/util/ResourceLeakageDetectorTest.java b/luni/src/test/java/libcore/java/util/ResourceLeakageDetectorTest.java
new file mode 100644
index 0000000..d86c9f2
--- /dev/null
+++ b/luni/src/test/java/libcore/java/util/ResourceLeakageDetectorTest.java
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package libcore.java.util;
+
+import dalvik.system.CloseGuard;
+import junit.framework.TestCase;
+
+/**
+ * Test for {@link ResourceLeakageDetector}
+ */
+public class ResourceLeakageDetectorTest extends TestCase {
+  /**
+   * This test will not work on RI as it does not support the <code>CloseGuard</code> or similar
+   * mechanism.
+   */
+  public void testDetectsUnclosedCloseGuard() throws Exception {
+    ResourceLeakageDetector detector = ResourceLeakageDetector.newDetector();
+    try {
+      CloseGuard closeGuard = createCloseGuard();
+      closeGuard.open("open");
+    } finally {
+      try {
+        System.logI("Checking for leaks");
+        detector.checkForLeaks();
+        fail();
+      } catch (AssertionError expected) {
+      }
+    }
+  }
+
+  public void testIgnoresClosedCloseGuard() throws Exception {
+    ResourceLeakageDetector detector = ResourceLeakageDetector.newDetector();
+    try {
+      CloseGuard closeGuard = createCloseGuard();
+      closeGuard.open("open");
+      closeGuard.close();
+    } finally {
+      detector.checkForLeaks();
+    }
+  }
+
+  /**
+   * Private method to ensure that the CloseGuard object is garbage collected.
+   */
+  private CloseGuard createCloseGuard() {
+    final CloseGuard closeGuard = CloseGuard.get();
+    new Object() {
+      @Override
+      protected void finalize() throws Throwable {
+        try {
+          closeGuard.warnIfOpen();
+        } finally {
+          super.finalize();
+        }
+      }
+    };
+
+    return closeGuard;
+  }
+}
diff --git a/support/src/test/java/libcore/dalvik/system/CloseGuardTester.java b/support/src/test/java/libcore/dalvik/system/CloseGuardTester.java
deleted file mode 100644
index e82d33d..0000000
--- a/support/src/test/java/libcore/dalvik/system/CloseGuardTester.java
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * Copyright (C) 2010 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package libcore.dalvik.system;
-
-import dalvik.system.CloseGuard;
-import java.io.Closeable;
-import java.io.IOException;
-import java.util.List;
-import java.util.concurrent.CopyOnWriteArrayList;
-import java.util.logging.ConsoleHandler;
-import java.util.logging.Handler;
-import java.util.logging.LogRecord;
-import java.util.logging.Logger;
-import junit.framework.Assert;
-import libcore.java.lang.ref.FinalizationTester;
-
-public final class CloseGuardTester implements Closeable {
-
-    private final List<LogRecord> logRecords = new CopyOnWriteArrayList<LogRecord>();
-    private final Logger logger = Logger.getLogger(CloseGuard.class.getName());
-
-    private final Handler logWatcher = new Handler() {
-        @Override public void close() {}
-        @Override public void flush() {}
-        @Override public void publish(LogRecord record) {
-            logRecords.add(record);
-        }
-    };
-
-    public CloseGuardTester() {
-        /*
-         * Collect immediately before we start monitoring the CloseGuard logs.
-         * This lowers the chance that we'll report an unrelated leak.
-         */
-        FinalizationTester.induceFinalization();
-        logger.addHandler(logWatcher);
-    }
-
-    public void assertEverythingWasClosed() {
-        FinalizationTester.induceFinalization();
-
-        if (!logRecords.isEmpty()) {
-            // print the log records with the output of this test
-            for (LogRecord leak : logRecords) {
-                new ConsoleHandler().publish(leak);
-            }
-            Assert.fail("CloseGuard detected unclosed resources!");
-        }
-    }
-
-    @Override public void close() throws IOException {
-        Logger.getLogger(CloseGuard.class.getName()).removeHandler(logWatcher);
-    }
-}