PR nits for #351

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

@@ -176,22 +176,16 @@ private void visitRootObj(LeakNode node) {
     RootObj rootObj = (RootObj) node.instance;
     Instance child = rootObj.getReferredInstance();
 
-    Boolean alwaysIgnoreClassHierarchy = null;
+    boolean visitRootNow = true;
     if (child != null) {
-      ClassObj superClassObj = child.getClassObj();
-      while (superClassObj != null) {
-        Boolean alwaysIgnoreClass = excludedRefs.rootClassNames.get(superClassObj.getClassName());
-        if (alwaysIgnoreClass != null) {
-          // true overrides null or false.
-          if (alwaysIgnoreClassHierarchy == null || !alwaysIgnoreClassHierarchy) {
-            alwaysIgnoreClassHierarchy = alwaysIgnoreClass;
-          }
+      // true = ignore root, false = visit root later, null = visit root now.
+      Boolean rootSuperClassAlwaysIgnored = rootSuperClassAlwaysIgnored(child);
+      if (rootSuperClassAlwaysIgnored != null) {
+        if (rootSuperClassAlwaysIgnored) {
+          return;
+        } else {
+          visitRootNow = false;
         }
-        superClassObj = superClassObj.getSuperClassObj();
-      }
-
-      if (alwaysIgnoreClassHierarchy != null && alwaysIgnoreClassHierarchy) {
-        return;
       }
     }
 
@@ -200,10 +194,27 @@ private void visitRootObj(LeakNode node) {
       // We switch the parent node with the thread instance that holds
       // the local reference.
       LeakNode parent = new LeakNode(holder, null, null, null);
-      enqueue(alwaysIgnoreClassHierarchy == null, parent, child, "<Java Local>", LOCAL);
+      enqueue(visitRootNow, parent, child, "<Java Local>", LOCAL);
     } else {
-      enqueue(alwaysIgnoreClassHierarchy == null, 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/ServiceBinderLeakTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Square, Inc.
+ * 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.
@@ -44,7 +44,7 @@ public class ServiceBinderLeakTest {
   }
 
   @Test public void realBinderLeak() {
-    excludedRefs.rootClass("android.os.Binder", true);
+    excludedRefs.rootSuperClass("android.os.Binder", true);
 
     AnalysisResult result = analyze(
         TestUtil.fileFromName("leak_service_binder.hprof"),
@@ -61,7 +61,7 @@ public class ServiceBinderLeakTest {
   }
 
   @Test public void ignorableBinderLeak() {
-    excludedRefs.rootClass("android.os.Binder", false);
+    excludedRefs.rootSuperClass("android.os.Binder", false);
 
     AnalysisResult result = analyze(
         TestUtil.fileFromName("leak_service_binder_ignored.hprof"),
@@ -78,7 +78,7 @@ public class ServiceBinderLeakTest {
   }
 
   @Test public void alwaysIgnorableBinderLeak() {
-    excludedRefs.rootClass("android.os.Binder", true);
+    excludedRefs.rootSuperClass("android.os.Binder", true);
 
     AnalysisResult result = analyze(
         TestUtil.fileFromName("leak_service_binder_ignored.hprof"),

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

@@ -1,9 +1,24 @@
+/*
+ * 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;
 
-class TestUtil {
+final class TestUtil {
   static File fileFromName(String filename) {
     ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
     URL url = classLoader.getResource(filename);

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

@@ -355,7 +355,7 @@ public enum AndroidExcludedRefs {
       // 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.rootClass("android.os.Binder", true);
+      excluded.rootSuperClass("android.os.Binder", true);
     }
   },
 

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,19 +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> rootClassNames;
+  public final Map<String, Boolean> rootSuperClassNames;
 
   private ExcludedRefs(Map<String, Map<String, Boolean>> fieldNameByClassName,
       Map<String, Map<String, Boolean>> staticFieldNameByClassName,
       Map<String, Boolean> threadNames, Map<String, Boolean> classNames,
-      Map<String, Boolean> rootClassNames) {
+      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.rootClassNames = unmodifiableMap(new LinkedHashMap<>(rootClassNames));
+    this.rootSuperClassNames = unmodifiableMap(new LinkedHashMap<>(rootSuperClassNames));
   }
 
   @Override public String toString() {
@@ -75,7 +75,7 @@ private ExcludedRefs(Map<String, Map<String, Boolean>> fieldNameByClassName,
       String always = clazz.getValue() ? " (always)" : "";
       string += "| Class:" + clazz.getKey() + always + "\n";
     }
-    for (Map.Entry<String, Boolean> clazz : rootClassNames.entrySet()) {
+    for (Map.Entry<String, Boolean> clazz : rootSuperClassNames.entrySet()) {
       String always = clazz.getValue() ? " (always)" : "";
       string += "| Root Class:" + clazz.getKey() + always + "\n";
     }
@@ -88,7 +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> rootClassNames = new LinkedHashMap<>();
+    private final Map<String, Boolean> rootSuperClassNames = new LinkedHashMap<>();
 
     public Builder instanceField(String className, String fieldName) {
       return instanceField(className, fieldName, false);
@@ -142,19 +142,20 @@ public Builder clazz(String className, boolean always) {
       return this;
     }
 
-    public Builder rootClass(String rootClassName) {
-      return rootClass(rootClassName, false);
+    public Builder rootSuperClass(String rootSuperClassName) {
+      return rootSuperClass(rootSuperClassName, false);
     }
 
-    public Builder rootClass(String rootClassName, boolean always) {
-      checkNotNull(rootClassName, "rootClassName");
-      rootClassNames.put(rootClassName, always);
+    /** 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, rootClassNames);
+          classNames, rootSuperClassNames);
     }
   }
 }