Repository: square/leakcanary
Issue: Some GC roots can be leak causes

Specifically thinking about gc roots which are natively held refs, those are currently marked as "not leaking" which is incorrect.

Changed files:
leakcanary-analyzer/src/main/java/leakcanary/HeapAnalyzer.kt, leakcanary-analyzer/src/test/java/leakcanary/internal/LeakTraceRendererTest.kt, leakcanary-analyzer/src/main/java/leakcanary/internal/ShortestPathFinder.kt, leakcanary-analyzer/src/main/java/leakcanary/LeakTrace.kt, leakcanary-analyzer/src/main/java/leakcanary/LeakNode.kt, leakcanary-analyzer/src/test/java/leakcanary/internal/LeakStatusTest.kt

diff --git a/leakcanary-analyzer/src/main/java/leakcanary/HeapAnalyzer.kt b/leakcanary-analyzer/src/main/java/leakcanary/HeapAnalyzer.kt
index b1a14d8da..db18b64c8 100644
--- a/leakcanary-analyzer/src/main/java/leakcanary/HeapAnalyzer.kt
+++ b/leakcanary-analyzer/src/main/java/leakcanary/HeapAnalyzer.kt
@@ -37,6 +37,7 @@ import leakcanary.GraphObjectRecord.GraphObjectArrayRecord
 import leakcanary.GraphObjectRecord.GraphPrimitiveArrayRecord
 import leakcanary.HprofPushRecordsParser.OnRecordListener
 import leakcanary.LeakNode.ChildNode
+import leakcanary.LeakNode.RootNode
 import leakcanary.LeakNodeStatus.LEAKING
 import leakcanary.LeakNodeStatus.NOT_LEAKING
 import leakcanary.LeakNodeStatus.UNKNOWN
@@ -436,10 +437,11 @@ class HeapAnalyzer constructor(
       )
       node = node.parent
     }
+    val rootNode = node as RootNode
 
     leakTraceInspectors.forEach { it.inspect(graph, leakReporters) }
 
-    val leakStatuses = computeLeakStatuses(leakReporters)
+    val leakStatuses = computeLeakStatuses(rootNode, leakReporters)
 
     node = leafNode
     while (node is ChildNode) {
@@ -453,16 +455,42 @@ class HeapAnalyzer constructor(
   }
 
   private fun computeLeakStatuses(
+    rootNode: RootNode,
     leakReporters: List<LeakTraceElementReporter>
   ): List<LeakNodeStatusAndReason> {
-    var lastNotLeakingElementIndex = 0
     val lastElementIndex = leakReporters.size - 1
+
+    val rootNodeReporter = leakReporters[0]
+
+    rootNodeReporter.addLabel(
+        "GC Root: " + when (rootNode.gcRoot) {
+          is ThreadObject -> "Thread object"
+          is JniGlobal -> "Global variable in native code"
+          is JniLocal -> "Local variable in native code"
+          is JavaFrame -> "Java local variable"
+          is NativeStack -> "Input or output parameters in native code"
+          is StickyClass -> "System class"
+          is ThreadBlock -> "Thread block"
+          is MonitorUsed -> "Monitor (anything that called the wait() or notify() methods, or that is synchronized.)"
+          is ReferenceCleanup -> "Reference cleanup"
+          is JniMonitor -> "Root JNI monitor"
+          else -> throw IllegalStateException("Unexpected gc root ${rootNode.gcRoot}")
+        }
+    )
+
+    when (rootNode.gcRoot) {
+      is StickyClass -> rootNodeReporter.reportNotLeaking("a system class never leaks")
+    }
+
+    leakReporters[lastElementIndex].reportLeaking("RefWatcher was watching this")
+
+    var lastNotLeakingElementIndex = 0
     var firstLeakingElementIndex = lastElementIndex
 
     val leakStatuses = ArrayList<LeakNodeStatusAndReason>()
 
     for ((index, reporter) in leakReporters.withIndex()) {
-      val leakStatus = inspectElementLeakStatus(reporter)
+      val leakStatus = resolveStatus(reporter)
       leakStatuses.add(leakStatus)
       if (leakStatus.status == NOT_LEAKING) {
         lastNotLeakingElementIndex = index
@@ -474,32 +502,12 @@ class HeapAnalyzer constructor(
       }
     }
 
-    leakStatuses[0] = when (leakStatuses[0].status) {
-      UNKNOWN -> LeakNodeStatus.notLeaking("it's a GC root")
-      NOT_LEAKING -> LeakNodeStatus.notLeaking(
-          "it's a GC root and ${leakStatuses[0].reason}"
-      )
-      LEAKING -> LeakNodeStatus.notLeaking(
-          "it's a GC root. Conflicts with ${leakStatuses[0].reason}"
-      )
-    }
-
-    leakStatuses[lastElementIndex] = when (leakStatuses[lastElementIndex].status) {
-      UNKNOWN -> LeakNodeStatus.leaking("RefWatcher was watching this")
-      LEAKING -> LeakNodeStatus.leaking(
-          "RefWatcher was watching this and ${leakStatuses[lastElementIndex].reason}"
-      )
-      NOT_LEAKING -> LeakNodeStatus.leaking(
-          "RefWatcher was watching this. Conflicts with ${leakStatuses[lastElementIndex].reason}"
-      )
-    }
-
     val simpleClassNames = leakReporters.map { reporter ->
       recordClassName(reporter.objectRecord).lastSegment('.')
     }
 
     // First and last are always known.
-    for (i in 1 until lastElementIndex) {
+    for (i in 0 ..lastElementIndex) {
       val leakStatus = leakStatuses[i]
       if (i < lastNotLeakingElementIndex) {
         val nextNotLeakingName = simpleClassNames[i + 1]
@@ -528,9 +536,10 @@ class HeapAnalyzer constructor(
     return leakStatuses
   }
 
-  private fun inspectElementLeakStatus(
+  private fun resolveStatus(
     reporter: LeakTraceElementReporter
   ): LeakNodeStatusAndReason {
+    // NOT_LEAKING always wins over LEAKING
     var current = LeakNodeStatus.unknown()
     for (statusAndReason in reporter.leakNodeStatuses) {
       current = when {
diff --git a/leakcanary-analyzer/src/main/java/leakcanary/LeakNode.kt b/leakcanary-analyzer/src/main/java/leakcanary/LeakNode.kt
index 60ef3694d..9ea3e5d2f 100644
--- a/leakcanary-analyzer/src/main/java/leakcanary/LeakNode.kt
+++ b/leakcanary-analyzer/src/main/java/leakcanary/LeakNode.kt
@@ -6,6 +6,7 @@ sealed class LeakNode {
   abstract val visitOrder: Int
 
   class RootNode(
+    val gcRoot: GcRoot,
     override val instance: Long,
     override val visitOrder: Int
   ) : LeakNode()
diff --git a/leakcanary-analyzer/src/main/java/leakcanary/LeakTrace.kt b/leakcanary-analyzer/src/main/java/leakcanary/LeakTrace.kt
index 1f05e5cb3..c577e6930 100644
--- a/leakcanary-analyzer/src/main/java/leakcanary/LeakTrace.kt
+++ b/leakcanary-analyzer/src/main/java/leakcanary/LeakTrace.kt
@@ -32,7 +32,7 @@ data class LeakTrace(
       NOT_LEAKING -> if (index < elements.lastIndex) {
         elements[index + 1].leakStatusAndReason.status != NOT_LEAKING
       } else {
-        true
+        false
       }
       else -> false
     }
diff --git a/leakcanary-analyzer/src/main/java/leakcanary/internal/ShortestPathFinder.kt b/leakcanary-analyzer/src/main/java/leakcanary/internal/ShortestPathFinder.kt
index 974a26dec..e73d150f8 100644
--- a/leakcanary-analyzer/src/main/java/leakcanary/internal/ShortestPathFinder.kt
+++ b/leakcanary-analyzer/src/main/java/leakcanary/internal/ShortestPathFinder.kt
@@ -261,7 +261,7 @@ internal class ShortestPathFinder {
       when (gcRoot) {
         is ThreadObject -> {
           threadsBySerialNumber[gcRoot.threadSerialNumber] = gcRoot
-          enqueue(graph, RootNode(gcRoot.id, visitOrder++), exclusionPriority = null)
+          enqueue(graph, RootNode(gcRoot, gcRoot.id, visitOrder++), exclusionPriority = null)
         }
         is JavaFrame -> {
           val threadRoot = threadsBySerialNumber.getValue(gcRoot.threadSerialNumber)
@@ -271,7 +271,7 @@ internal class ShortestPathFinder {
 
           if (exclusion == null || exclusion.status != NEVER_REACHABLE) {
             // visitOrder is unused as this root node isn't enqueued.
-            val rootNode = RootNode(threadRoot.id, visitOrder = 0)
+            val rootNode = RootNode(gcRoot, threadRoot.id, visitOrder = 0)
             // TODO #1352 Instead of <Java Local>, it should be <local variable in Foo.bar()>
             // We should also add the full stacktrace as a label of thread objects
             val leakReference = LeakReference(LOCAL, "")
@@ -282,7 +282,7 @@ internal class ShortestPathFinder {
             )
           }
         }
-        else -> enqueue(graph, RootNode(gcRoot.id, visitOrder++), exclusionPriority = null)
+        else -> enqueue(graph, RootNode(gcRoot, gcRoot.id, visitOrder++), exclusionPriority = null)
       }
     }
     gcRoots.clear()
diff --git a/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakStatusTest.kt b/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakStatusTest.kt
index 761db9592..fb2ef613e 100644
--- a/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakStatusTest.kt
+++ b/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakStatusTest.kt
@@ -183,7 +183,7 @@ class LeakStatusTest {
 
     val leak = analysis.retainedInstances[0] as LeakingInstance
     assertThat(leak.leakTrace.elements[0].leakStatusAndReason.status).isEqualTo(NOT_LEAKING)
-    assertThat(leak.leakTrace.elements[0].leakStatusAndReason.reason).isEqualTo("it's a GC root")
+    assertThat(leak.leakTrace.elements[0].leakStatusAndReason.reason).isEqualTo("Class1↓ is not leaking and a system class never leaks")
     assertThat(leak.leakTrace.elements[1].leakStatusAndReason.status).isEqualTo(NOT_LEAKING)
     assertThat(leak.leakTrace.elements[1].leakStatusAndReason.reason).isEqualTo(
         "Class2↓ is not leaking. Conflicts with Class1 is leaking"
@@ -234,7 +234,7 @@ class LeakStatusTest {
 
     assertThat(leak.leakTrace.elements.first().leakStatusAndReason.status).isEqualTo(NOT_LEAKING)
     assertThat(leak.leakTrace.elements.first().leakStatusAndReason.reason).isEqualTo(
-        "it's a GC root. Conflicts with GcRoot is leaking"
+        "a system class never leaks. Conflicts with GcRoot is leaking"
     )
   }
 
@@ -250,7 +250,7 @@ class LeakStatusTest {
 
     assertThat(leak.leakTrace.elements.first().leakStatusAndReason.status).isEqualTo(NOT_LEAKING)
     assertThat(leak.leakTrace.elements.first().leakStatusAndReason.reason).isEqualTo(
-        "it's a GC root and GcRoot is not leaking"
+        "GcRoot is not leaking and a system class never leaks"
     )
   }
 
@@ -262,9 +262,9 @@ class LeakStatusTest {
       )
 
     val leak = analysis.retainedInstances[0] as LeakingInstance
-    assertThat(leak.leakTrace.elements.last().leakStatusAndReason.status).isEqualTo(LEAKING)
+    assertThat(leak.leakTrace.elements.last().leakStatusAndReason.status).isEqualTo(NOT_LEAKING)
     assertThat(leak.leakTrace.elements.last().leakStatusAndReason.reason).isEqualTo(
-        "RefWatcher was watching this. Conflicts with Leaking is not leaking"
+        "Leaking is not leaking. Conflicts with RefWatcher was watching this"
     )
   }
 
@@ -278,7 +278,7 @@ class LeakStatusTest {
     val leak = analysis.retainedInstances[0] as LeakingInstance
     assertThat(leak.leakTrace.elements.last().leakStatusAndReason.status).isEqualTo(LEAKING)
     assertThat(leak.leakTrace.elements.last().leakStatusAndReason.reason).isEqualTo(
-        "RefWatcher was watching this and Leaking is leaking"
+        "Leaking is leaking and RefWatcher was watching this"
     )
   }
 
diff --git a/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakTraceRendererTest.kt b/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakTraceRendererTest.kt
index 0bcdfaeb2..f9f23905f 100644
--- a/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakTraceRendererTest.kt
+++ b/leakcanary-analyzer/src/test/java/leakcanary/internal/LeakTraceRendererTest.kt
@@ -38,7 +38,8 @@ class LeakTraceRendererTest {
     analysis renders """
     ┬
     ├─ GcRoot
-    │    Leaking: NO (it's a GC root)
+    │    Leaking: NO (a system class never leaks)
+    │    GC Root: System class
     │    ↓ static GcRoot.leak
     │                    ~~~~
     ╰→ Leaking
@@ -74,7 +75,8 @@ class LeakTraceRendererTest {
     analysis renders """
     ┬
     ├─ GcRoot
-    │    Leaking: NO (it's a GC root)
+    │    Leaking: NO (a system class never leaks)
+    │    GC Root: System class
     │    ↓ static GcRoot.instanceA
     │                    ~~~~~~~~~
     ├─ ClassA
@@ -85,7 +87,7 @@ class LeakTraceRendererTest {
     │    Leaking: YES (because reasons)
     │    ↓ ClassB.leak
     ╰→ Leaking
-    ​     Leaking: YES (RefWatcher was watching this)
+    ​     Leaking: YES (ClassB↑ is leaking and RefWatcher was watching this)
     """
   }
 
@@ -113,8 +115,9 @@ class LeakTraceRendererTest {
     analysis renders """
     ┬
     ├─ GcRoot
-    │    Leaking: NO (it's a GC root)
+    │    Leaking: NO (a system class never leaks)
     │    ¯\\_(ツ)_/¯
+    │    GC Root: System class
     │    ↓ static GcRoot.leak
     │                    ~~~~
     ╰→ Leaking
@@ -140,7 +143,8 @@ class LeakTraceRendererTest {
     analysis renders """
     ┬
     ├─ GcRoot
-    │    Leaking: NO (it's a GC root)
+    │    Leaking: NO (a system class never leaks)
+    │    GC Root: System class
     │    ↓ static GcRoot.instanceA
     │                    ~~~~~~~~~
     ├─ ClassA
@@ -166,7 +170,8 @@ class LeakTraceRendererTest {
     analysis renders """
     ┬
     ├─ GcRoot
-    │    Leaking: NO (it's a GC root)
+    │    Leaking: NO (a system class never leaks)
+    │    GC Root: System class
     │    ↓ static GcRoot.array
     │                    ~~~~~
     ├─ java.lang.Object[]
@@ -187,7 +192,8 @@ class LeakTraceRendererTest {
     analysis renders """
     ┬
     ├─ MyThread
-    │    Leaking: NO (it's a GC root)
+    │    Leaking: UNKNOWN
+    │    GC Root: Java local variable
     │    ↓ thread MyThread.<Java Local>
     │                      ~~~~~~~~~~~~
     ╰→ Leaking