Fix GC Root bugs that missed leaks (#1168)

In Perflib, Snapshot.getGcRoots() only returns the GC roots from the first heap. Based on empirical evidence, it seems like Android has 4 heaps, and the GC Roots used to only be in the first heap, but more recent versions of Android have GC Roots in other heaps as well.

This means LeakCanary would sometimes show "No Leak Found" when there actually was a leak.

Fixes #1163

Author: pyricau

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Future release
 
+* [#1163](https://github.com/square/leakcanary/issues/1163) Fixed leaks being incorrectly classified as "no leak" due to missed GC Roots.
 * [#1153](https://github.com/square/leakcanary/issues/1153) `LeakCanary.isInAnalyzerProcess` now correctly returns true in the analyzer process prior to any first leak (could be triggered by starting the leak result activity).
 * [#1158](https://github.com/square/leakcanary/issues/1158) Stopped enabling DisplayLeakActivity when not using DisplayLeakService.
 * [#1135](https://github.com/square/leakcanary/issues/1135) Fixed IndexOutOfBoundsException for leak traces of size 1.

leakcanary-analyzer/src/main/java/com/squareup/haha/perflib/HahaSpy.java

@@ -16,6 +16,8 @@
 package com.squareup.haha.perflib;
 
 import android.support.annotation.NonNull;
+import java.util.HashSet;
+import java.util.Set;
 
 public final class HahaSpy {
 
@@ -31,6 +33,18 @@ public final class HahaSpy {
     return snapshot.findInstance(thread.mId);
   }
 
+  /**
+   * Returns the GC Roots for all heaps in the Snapshot. Unfortunately,
+   * {@link Snapshot#getGCRoots()} only returns the GC Roots of the first heap.
+   */
+  public static Set<RootObj> allGcRoots(Snapshot snapshot) {
+    Set<RootObj> allRoots = new HashSet<>();
+    for (Heap heap : snapshot.getHeaps()) {
+      allRoots.addAll(heap.mRoots);
+    }
+    return allRoots;
+  }
+
   private HahaSpy() {
     throw new AssertionError();
   }

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

@@ -129,7 +129,7 @@ private void clearState() {
   }
 
   private void enqueueGcRoots(Snapshot snapshot) {
-    for (RootObj rootObj : snapshot.getGCRoots()) {
+    for (RootObj rootObj : HahaSpy.allGcRoots(snapshot)) {
       switch (rootObj.getRootType()) {
         case JAVA_LOCAL:
           Instance thread = HahaSpy.allocatingThread(rootObj);

leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TrackedReferencesTest.java renamed to leakcanary-analyzer/src/test/java/com/squareup/leakcanary/HeapAnalyzerHeapDumpTest.java

@@ -5,13 +5,15 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_PRE_M;
 import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_M;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_PRE_M;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.GC_ROOT_IN_NON_PRIMARY_HEAP;
+import static com.squareup.leakcanary.TestUtil.analyze;
 import static com.squareup.leakcanary.TestUtil.findTrackedReferences;
 import static org.assertj.core.api.Assertions.assertThat;
 
 @RunWith(JUnit4.class)
-public class TrackedReferencesTest {
+public class HeapAnalyzerHeapDumpTest {
 
   @Test public void findsExpectedRef() {
     List<TrackedReference> trackedReferences = findTrackedReferences(ASYNC_TASK_M);
@@ -25,4 +27,9 @@ public class TrackedReferencesTest {
     List<TrackedReference> trackedReferences = findTrackedReferences(ASYNC_TASK_PRE_M);
     assertThat(trackedReferences).hasSize(2);
   }
+
+  @Test public void leakFoundWithGcRootInNonPrimaryHeap() {
+    AnalysisResult result = analyze(GC_ROOT_IN_NON_PRIMARY_HEAP);
+    assertThat(result.leakFound).isTrue();
+  }
 }

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

@@ -1,9 +1,7 @@
 package com.squareup.leakcanary;
 
-import java.lang.ref.WeakReference;
 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;
@@ -30,22 +28,14 @@ public class RetainedSizeTest {
 
   private final TestUtil.HeapDumpFile heapDumpFile;
   private final long expectedRetainedHeapSize;
-  ExcludedRefs.BuilderWithParams excludedRefs;
 
   public RetainedSizeTest(TestUtil.HeapDumpFile heapDumpFile, long expectedRetainedHeapSize) {
     this.heapDumpFile = heapDumpFile;
     this.expectedRetainedHeapSize = expectedRetainedHeapSize;
   }
 
-  @Before public void setUp() {
-    excludedRefs = new ExcludedRefs.BuilderWithParams().clazz(WeakReference.class.getName())
-        .alwaysExclude()
-        .clazz("java.lang.ref.FinalizerReference")
-        .alwaysExclude();
-  }
-
   @Test public void leakFound() {
-    AnalysisResult result = analyze(heapDumpFile, excludedRefs);
+    AnalysisResult result = analyze(heapDumpFile);
     assertEquals(expectedRetainedHeapSize, result.retainedHeapSize);
   }
 }

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

@@ -16,6 +16,9 @@
 package com.squareup.leakcanary;
 
 import java.io.File;
+import java.lang.ref.PhantomReference;
+import java.lang.ref.SoftReference;
+import java.lang.ref.WeakReference;
 import java.net.URL;
 import java.util.Collections;
 import java.util.List;
@@ -27,7 +30,9 @@ final class TestUtil {
   enum HeapDumpFile {
     ASYNC_TASK_PRE_M("leak_asynctask_pre_m.hprof", "dc983a12-d029-4003-8890-7dd644c664c5"), //
     ASYNC_TASK_M("leak_asynctask_m.hprof", "25ae1778-7c1d-4ec7-ac50-5cce55424069"), //
-    ASYNC_TASK_O("leak_asynctask_o.hprof", "0e8d40d7-8302-4493-93d5-962a4c176089");
+    ASYNC_TASK_O("leak_asynctask_o.hprof", "0e8d40d7-8302-4493-93d5-962a4c176089"),
+    GC_ROOT_IN_NON_PRIMARY_HEAP("gc_root_in_non_primary_heap.hprof",
+        "10a5bc66-e9cb-430c-930a-fc1dc4fc0f85");
 
     public final String filename;
     public final String referenceKey;
@@ -52,6 +57,18 @@ static List<TrackedReference> findTrackedReferences(HeapDumpFile heapDumpFile) {
     return heapAnalyzer.findTrackedReferences(file);
   }
 
+  static AnalysisResult analyze(HeapDumpFile heapDumpFile) {
+    ExcludedRefs.BuilderWithParams excludedRefs = new ExcludedRefs.BuilderWithParams()
+        .clazz(WeakReference.class.getName()).alwaysExclude()
+        .clazz(SoftReference.class.getName()).alwaysExclude()
+        .clazz(PhantomReference.class.getName()).alwaysExclude()
+        .clazz("java.lang.ref.Finalizer").alwaysExclude()
+        .clazz("java.lang.ref.FinalizerReference").alwaysExclude()
+        .thread("FinalizerWatchdogDaemon").alwaysExclude()
+        .thread("main").alwaysExclude();
+    return analyze(heapDumpFile, excludedRefs);
+  }
+
   static AnalysisResult analyze(HeapDumpFile heapDumpFile,
       ExcludedRefs.BuilderWithParams excludedRefs) {
     File file = fileFromName(heapDumpFile.filename);