diff --git a/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/ShortestPathFinder.java b/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/ShortestPathFinder.java index 134c5e41ae..82028a181b 100644 --- a/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/ShortestPathFinder.java +++ b/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, "", LOCAL); + enqueue(visitRootNow, parent, child, "", 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) { diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java index 8c5b668d6e..3545c1eed5 100644 --- a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java +++ b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java @@ -17,7 +17,6 @@ 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; @@ -25,6 +24,8 @@ 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; - } - - } diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/ServiceBinderLeakTest.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/ServiceBinderLeakTest.java new file mode 100644 index 0000000000..d270647afd --- /dev/null +++ b/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); + } + +} diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TestUtil.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TestUtil.java new file mode 100644 index 0000000000..da3d5cd79a --- /dev/null +++ b/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(); + } +} diff --git a/leakcanary-analyzer/src/test/resources/leak_service_binder.hprof b/leakcanary-analyzer/src/test/resources/leak_service_binder.hprof new file mode 100644 index 0000000000..af1dab1820 Binary files /dev/null and b/leakcanary-analyzer/src/test/resources/leak_service_binder.hprof differ diff --git a/leakcanary-analyzer/src/test/resources/leak_service_binder_ignored.hprof b/leakcanary-analyzer/src/test/resources/leak_service_binder_ignored.hprof new file mode 100644 index 0000000000..cdb867c441 Binary files /dev/null and b/leakcanary-analyzer/src/test/resources/leak_service_binder_ignored.hprof differ diff --git a/leakcanary-android/src/main/java/com/squareup/leakcanary/AndroidExcludedRefs.java b/leakcanary-android/src/main/java/com/squareup/leakcanary/AndroidExcludedRefs.java index c97ec97c43..d912b020d1 100644 --- a/leakcanary-android/src/main/java/com/squareup/leakcanary/AndroidExcludedRefs.java +++ b/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)); } /** diff --git a/leakcanary-watcher/src/main/java/com/squareup/leakcanary/ExcludedRefs.java b/leakcanary-watcher/src/main/java/com/squareup/leakcanary/ExcludedRefs.java index adbc4e6225..9660eda278 100644 --- a/leakcanary-watcher/src/main/java/com/squareup/leakcanary/ExcludedRefs.java +++ b/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> staticFieldNameByClassName; public final Map threadNames; public final Map classNames; + public final Map rootSuperClassNames; ExcludedRefs(Map> fieldNameByClassName, - Map> staticFieldNameByClassName, - Map threadNames, Map classNames) { + Map> staticFieldNameByClassName, + Map threadNames, Map classNames, + Map 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 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 threadNames = new LinkedHashMap<>(); private final Map classNames = new LinkedHashMap<>(); + private final Map 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); } } }