Merge branch 'binder_leak'

* binder_leak:
  PR nits for #351
  Add special case to ignore Binder leaks

leakcanary-analyzer/src/main/java/com/squareup/leakcanary/ShortestPathFinder.java

@@ -176,15 +176,45 @@ private void visitRootObj(LeakNode node) {
     RootObj rootObj = (RootObj) node.instance;
     Instance child = rootObj.getReferredInstance();
 
+    boolean visitRootNow = true;
+    if (child != null) {
+      // true = ignore root, false = visit root later, null = visit root now.
+      Boolean rootSuperClassAlwaysIgnored = rootSuperClassAlwaysIgnored(child);
+      if (rootSuperClassAlwaysIgnored != null) {
+        if (rootSuperClassAlwaysIgnored) {
+          return;
+        } else {
+          visitRootNow = false;
+        }
+      }
+    }
+
     if (rootObj.getRootType() == RootType.JAVA_LOCAL) {
       Instance holder = HahaSpy.allocatingThread(rootObj);
       // We switch the parent node with the thread instance that holds
       // the local reference.
       LeakNode parent = new LeakNode(holder, null, null, null);
-      enqueue(true, parent, child, "<Java Local>", LOCAL);
+      enqueue(visitRootNow, parent, child, "<Java Local>", LOCAL);
     } else {
-      enqueue(true, node, child, null, null);
+      enqueue(visitRootNow, node, child, null, null);
+    }
+  }
+
+  private Boolean rootSuperClassAlwaysIgnored(Instance child) {
+    Boolean alwaysIgnoreClassHierarchy = null;
+    ClassObj superClassObj = child.getClassObj();
+    while (superClassObj != null) {
+      Boolean alwaysIgnoreClass =
+          excludedRefs.rootSuperClassNames.get(superClassObj.getClassName());
+      if (alwaysIgnoreClass != null) {
+        // true overrides null or false.
+        if (alwaysIgnoreClassHierarchy == null || !alwaysIgnoreClassHierarchy) {
+          alwaysIgnoreClassHierarchy = alwaysIgnoreClass;
+        }
+      }
+      superClassObj = superClassObj.getSuperClassObj();
     }
+    return alwaysIgnoreClassHierarchy;
   }
 
   private void visitClassObj(LeakNode node) {

leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java

@@ -17,14 +17,15 @@
 
 import java.io.File;
 import java.lang.ref.WeakReference;
-import java.net.URL;
 import java.util.Arrays;
 import java.util.Collection;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
+import static com.squareup.leakcanary.TestUtil.fileFromName;
+import static com.squareup.leakcanary.TestUtil.analyze;
 import static com.squareup.leakcanary.LeakTraceElement.Holder.THREAD;
 import static com.squareup.leakcanary.LeakTraceElement.Type.STATIC_FIELD;
 import static org.hamcrest.core.StringContains.containsString;
@@ -49,12 +50,6 @@ public class AsyncTaskLeakTest {
     });
   }
 
-  private static File fileFromName(String filename) {
-    ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
-    URL url = classLoader.getResource(filename);
-    return new File(url.getPath());
-  }
-
   final File heapDumpFile;
   final String referenceKey;
 
@@ -71,7 +66,7 @@ public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
   }
 
   @Test public void leakFound() {
-    AnalysisResult result = analyze();
+    AnalysisResult result = analyze(heapDumpFile, referenceKey, excludedRefs);
     assertTrue(result.leakFound);
     assertFalse(result.excludedLeak);
     LeakTraceElement gcRoot = result.leakTrace.elements.get(0);
@@ -82,7 +77,7 @@ public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
 
   @Test public void excludeThread() {
     excludedRefs.thread(ASYNC_TASK_THREAD);
-    AnalysisResult result = analyze();
+    AnalysisResult result = analyze(heapDumpFile, referenceKey, excludedRefs);
     assertTrue(result.leakFound);
     assertFalse(result.excludedLeak);
     LeakTraceElement gcRoot = result.leakTrace.elements.get(0);
@@ -96,22 +91,9 @@ public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
     excludedRefs.thread(ASYNC_TASK_THREAD);
     excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_1);
     excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_2);
-    AnalysisResult result = analyze();
+    AnalysisResult result = analyze(heapDumpFile, referenceKey, excludedRefs);
     assertTrue(result.leakFound);
     assertTrue(result.excludedLeak);
   }
 
-  private AnalysisResult analyze() {
-    HeapAnalyzer heapAnalyzer = new HeapAnalyzer(excludedRefs.build());
-    AnalysisResult result = heapAnalyzer.checkForLeak(heapDumpFile, referenceKey);
-    if (result.failure != null) {
-      result.failure.printStackTrace();
-    }
-    if (result.leakTrace != null) {
-      System.out.println(result.leakTrace);
-    }
-    return result;
-  }
-
-
 }

leakcanary-analyzer/src/test/java/com/squareup/leakcanary/ServiceBinderLeakTest.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2016 Square, Inc.
+ *
+ * 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 com.squareup.leakcanary;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.lang.ref.WeakReference;
+
+import static com.squareup.leakcanary.LeakTraceElement.Holder.CLASS;
+import static com.squareup.leakcanary.LeakTraceElement.Holder.OBJECT;
+import static com.squareup.leakcanary.LeakTraceElement.Type.INSTANCE_FIELD;
+import static com.squareup.leakcanary.LeakTraceElement.Type.STATIC_FIELD;
+import static com.squareup.leakcanary.TestUtil.analyze;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * leak_service_binder_ignored.hprof contains a "normal" leak when binding to a service, where
+ * leak_service_binder.hprof contains a leak where a binder is leaked by a static field.
+ */
+public class ServiceBinderLeakTest {
+
+  ExcludedRefs.Builder excludedRefs;
+
+  @Before public void setUp() {
+    excludedRefs = new ExcludedRefs.Builder()
+        .clazz(WeakReference.class.getName(), true)
+        .clazz("java.lang.ref.FinalizerReference", true);
+  }
+
+  @Test public void realBinderLeak() {
+    excludedRefs.rootSuperClass("android.os.Binder", true);
+
+    AnalysisResult result = analyze(
+        TestUtil.fileFromName("leak_service_binder.hprof"),
+        "b3abfae6-2c53-42e1-b8c1-96b0558dbeae",
+        excludedRefs
+    );
+
+    assertTrue(result.leakFound);
+    assertFalse(result.excludedLeak);
+    LeakTraceElement gcRoot = result.leakTrace.elements.get(0);
+    assertEquals(STATIC_FIELD, gcRoot.type);
+    assertEquals("com.example.leakcanary.LeakyService", gcRoot.className);
+    assertEquals(CLASS, gcRoot.holder);
+  }
+
+  @Test public void ignorableBinderLeak() {
+    excludedRefs.rootSuperClass("android.os.Binder", false);
+
+    AnalysisResult result = analyze(
+        TestUtil.fileFromName("leak_service_binder_ignored.hprof"),
+        "6e524414-9581-4ce7-8690-e8ddf8b82454",
+        excludedRefs
+    );
+
+    assertTrue(result.leakFound);
+    assertTrue(result.excludedLeak);
+    LeakTraceElement gcRoot = result.leakTrace.elements.get(0);
+    assertEquals(INSTANCE_FIELD, gcRoot.type);
+    assertEquals("com.example.leakcanary.LeakyService$MyBinder", gcRoot.className);
+    assertEquals(OBJECT, gcRoot.holder);
+  }
+
+  @Test public void alwaysIgnorableBinderLeak() {
+    excludedRefs.rootSuperClass("android.os.Binder", true);
+
+    AnalysisResult result = analyze(
+        TestUtil.fileFromName("leak_service_binder_ignored.hprof"),
+        "6e524414-9581-4ce7-8690-e8ddf8b82454",
+        excludedRefs
+    );
+
+    assertFalse(result.leakFound);
+  }
+
+}

leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TestUtil.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2016 Square, Inc.
+ *
+ * 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 com.squareup.leakcanary;
+
+import java.io.File;
+import java.net.URL;
+
+final class TestUtil {
+  static File fileFromName(String filename) {
+    ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
+    URL url = classLoader.getResource(filename);
+    return new File(url.getPath());
+  }
+
+  static AnalysisResult analyze(File heapDumpFile, String referenceKey, ExcludedRefs.Builder excludedRefs) {
+    HeapAnalyzer heapAnalyzer = new HeapAnalyzer(excludedRefs.build());
+    AnalysisResult result = heapAnalyzer.checkForLeak(heapDumpFile, referenceKey);
+    if (result.failure != null) {
+      result.failure.printStackTrace();
+    }
+    if (result.leakTrace != null) {
+      System.out.println(result.leakTrace);
+    }
+    return result;
+  }
+
+  private TestUtil() {
+    throw new AssertionError();
+  }
+}

leakcanary-android/src/main/java/com/squareup/leakcanary/AndroidExcludedRefs.java

@@ -349,6 +349,16 @@ public enum AndroidExcludedRefs {
     }
   },
 
+  SERVICE_BINDER {
+    @Override void add(ExcludedRefs.Builder excluded) {
+      // We should ignore leaks where an android.os.Binder is the root of the leak.
+      // When you bind and unbind from a Service, the OS will keep a reference to the Binder
+      // until the client binder has been GC'ed. This means the Binder can be retained after
+      // Service.onDestroy() is called.
+      excluded.rootSuperClass("android.os.Binder", true);
+    }
+  },
+
   SOFT_REFERENCES {
     @Override void add(ExcludedRefs.Builder excluded) {
       excluded.clazz(WeakReference.class.getName(), true);
@@ -412,7 +422,7 @@ public enum AndroidExcludedRefs {
    */
   public static ExcludedRefs.Builder createAndroidDefaults() {
     return createBuilder(EnumSet.of(SOFT_REFERENCES, FINALIZER_WATCHDOG_DAEMON, MAIN, LEAK_CANARY_THREAD,
-        EVENT_RECEIVER__MMESSAGE_QUEUE));
+        EVENT_RECEIVER__MMESSAGE_QUEUE, SERVICE_BINDER));
   }
 
   /**

leakcanary-watcher/src/main/java/com/squareup/leakcanary/ExcludedRefs.java

@@ -23,10 +23,10 @@
 import static java.util.Collections.unmodifiableMap;
 
 /**
- * Prevents specific references from being taken into account when computing the shortest strong
- * reference path from a suspected leaking instance to the GC roots.
+ * Prevents specific references from being taken into account when computing the shortest reference
+ * path from a suspected leaking instance to the GC roots.
  *
- * This class lets you ignore known memory leaks that you known about. If the shortest path
+ * This class lets you ignore known memory leaks that you know about. If the shortest path
  * matches {@link ExcludedRefs}, than the heap analyzer should look for a longer path with nothing
  * matching in {@link ExcludedRefs}.
  */
@@ -36,16 +36,19 @@ public final class ExcludedRefs implements Serializable {
   public final Map<String, Map<String, Boolean>> staticFieldNameByClassName;
   public final Map<String, Boolean> threadNames;
   public final Map<String, Boolean> classNames;
+  public final Map<String, Boolean> rootSuperClassNames;
 
   ExcludedRefs(Map<String, Map<String, Boolean>> fieldNameByClassName,
-               Map<String, Map<String, Boolean>> staticFieldNameByClassName,
-               Map<String, Boolean> threadNames, Map<String, Boolean> classNames) {
+      Map<String, Map<String, Boolean>> staticFieldNameByClassName,
+      Map<String, Boolean> threadNames, Map<String, Boolean> classNames,
+      Map<String, Boolean> rootSuperClassNames) {
     // Copy + unmodifiable.
     this.fieldNameByClassName = unmodifiableMap(new LinkedHashMap<>(fieldNameByClassName));
     this.staticFieldNameByClassName =
         unmodifiableMap(new LinkedHashMap<>(staticFieldNameByClassName));
     this.threadNames = unmodifiableMap(new LinkedHashMap<>(threadNames));
     this.classNames = unmodifiableMap(new LinkedHashMap<>(classNames));
+    this.rootSuperClassNames = unmodifiableMap(new LinkedHashMap<>(rootSuperClassNames));
   }
 
   @Override public String toString() {
@@ -72,6 +75,10 @@ public final class ExcludedRefs implements Serializable {
       String always = clazz.getValue() ? " (always)" : "";
       string += "| Class:" + clazz.getKey() + always + "\n";
     }
+    for (Map.Entry<String, Boolean> clazz : rootSuperClassNames.entrySet()) {
+      String always = clazz.getValue() ? " (always)" : "";
+      string += "| Root Class:" + clazz.getKey() + always + "\n";
+    }
     return string;
   }
 
@@ -81,6 +88,7 @@ public static final class Builder {
         new LinkedHashMap<>();
     private final Map<String, Boolean> threadNames = new LinkedHashMap<>();
     private final Map<String, Boolean> classNames = new LinkedHashMap<>();
+    private final Map<String, Boolean> rootSuperClassNames = new LinkedHashMap<>();
 
     public Builder instanceField(String className, String fieldName) {
       return instanceField(className, fieldName, false);
@@ -134,9 +142,20 @@ public Builder clazz(String className, boolean always) {
       return this;
     }
 
+    public Builder rootSuperClass(String rootSuperClassName) {
+      return rootSuperClass(rootSuperClassName, false);
+    }
+
+    /** Ignores any GC root that is a subclass of the provided class name. */
+    public Builder rootSuperClass(String rootSuperClassName, boolean always) {
+      checkNotNull(rootSuperClassName, "rootSuperClassName");
+      rootSuperClassNames.put(rootSuperClassName, always);
+      return this;
+    }
+
     public ExcludedRefs build() {
       return new ExcludedRefs(fieldNameByClassName, staticFieldNameByClassName, threadNames,
-          classNames);
+          classNames, rootSuperClassNames);
     }
   }
 }