Report super class holding field in leaktrace

Fixes #1972
square · Oct 15, 2020 · a6c589c · a6c589c
1 parent ff85924
commit a6c589c
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 18 deletions.
diff --git a/leakcanary-android-core/src/main/java/leakcanary/internal/DisplayLeakAdapter.kt b/leakcanary-android-core/src/main/java/leakcanary/internal/DisplayLeakAdapter.kt
@@ -44,6 +44,7 @@ import shark.LeakTraceObject
 import shark.LeakTraceObject.LeakingStatus.LEAKING
 import shark.LeakTraceObject.LeakingStatus.NOT_LEAKING
 import shark.LeakTraceObject.LeakingStatus.UNKNOWN
+import shark.LeakTraceReference
 import shark.LeakTraceReference.ReferenceType.STATIC_FIELD
 
 @Suppress("DEPRECATION")
@@ -144,7 +145,7 @@ internal class DisplayLeakAdapter constructor(
         val staticPrefix = if (referencePath.referenceType == STATIC_FIELD) "static " else ""
 
         val htmlString = leakTraceObject.asHtmlString(typeName) +
-            "$INDENTATION$staticPrefix${leakTraceObject.styledClassSimpleName()}.$referenceName"
+            "$INDENTATION$staticPrefix${referencePath.styledOwningClassSimpleName()}.$referenceName"
 
         val builder = Html.fromHtml(htmlString) as SpannableStringBuilder
         if (isSuspect) {
@@ -196,6 +197,11 @@ internal class DisplayLeakAdapter constructor(
     return "<font color='$highlightColorHexString'>$simpleName</font>"
   }
 
+  private fun LeakTraceReference.styledOwningClassSimpleName(): String {
+    val simpleName = owningClassSimpleName.replace("[]", "[ ]")
+    return "<font color='$highlightColorHexString'>$simpleName</font>"
+  }
+
   @Suppress("ReturnCount")
   private fun getConnectorType(position: Int): Type {
     if (position == 1) {

diff --git a/shark/src/main/java/shark/HeapAnalyzer.kt b/shark/src/main/java/shark/HeapAnalyzer.kt
@@ -295,7 +295,7 @@ class HeapAnalyzer constructor(
     fun asList() = listOf(root) + childPath
   }
 
-  private fun buildLeakTraces(
+  private fun FindLeakInput.buildLeakTraces(
     shortestPaths: List<ShortestPath>,
     inspectedObjectsByPath: List<List<InspectedObject>>,
     retainedSizes: Map<Long, Pair<Int, Int>>?
@@ -424,14 +424,19 @@ class HeapAnalyzer constructor(
     }
   }
 
-  private fun buildReferencePath(
+  private fun FindLeakInput.buildReferencePath(
     shortestChildPath: List<ChildNode>,
     leakTraceObjects: List<LeakTraceObject>
   ): List<LeakTraceReference> {
     return shortestChildPath.mapIndexed { index, childNode ->
       LeakTraceReference(
           originObject = leakTraceObjects[index],
           referenceType = childNode.refFromParentType,
+          owningClassName = if (childNode.owningClassId != 0L) {
+            graph.findObjectById(childNode.owningClassId).asClass!!.name
+          } else {
+            leakTraceObjects[index].className
+          },
           referenceName = childNode.refFromParentName
       )
     }

diff --git a/shark/src/main/java/shark/LeakReference.kt b/shark/src/main/java/shark/LeakReference.kt
@@ -22,6 +22,7 @@ internal class LeakReference : Serializable {
         LOCAL -> LeakTraceReference.ReferenceType.LOCAL
         ARRAY_ENTRY -> LeakTraceReference.ReferenceType.ARRAY_ENTRY
       },
+      owningClassName = originObject.className,
       referenceName = name!!
   )
 

diff --git a/shark/src/main/java/shark/LeakTrace.kt b/shark/src/main/java/shark/LeakTrace.kt
@@ -220,7 +220,7 @@ data class LeakTrace(
     ): String {
       val static = if (reference.referenceType == STATIC_FIELD) " static" else ""
       val referenceLine =
-        "    ↓$static ${reference.originObject.classSimpleName}.${reference.referenceDisplayName}"
+        "    ↓$static ${reference.owningClassSimpleName}.${reference.referenceDisplayName}"
 
       return if (showLeakingStatus && leakTrace.referencePathElementIsSuspect(index)) {
         val lengthBeforeReferenceName = referenceLine.lastIndexOf('.') + 1

diff --git a/shark/src/main/java/shark/LeakTraceReference.kt b/shark/src/main/java/shark/LeakTraceReference.kt
@@ -4,6 +4,7 @@ import shark.LeakTraceReference.ReferenceType.ARRAY_ENTRY
 import shark.LeakTraceReference.ReferenceType.INSTANCE_FIELD
 import shark.LeakTraceReference.ReferenceType.LOCAL
 import shark.LeakTraceReference.ReferenceType.STATIC_FIELD
+import shark.internal.lastSegment
 import java.io.Serializable
 
 /**
@@ -17,6 +18,8 @@ data class LeakTraceReference(
 
   val referenceType: ReferenceType,
 
+  val owningClassName: String,
+
   val referenceName: String
 
 ) : Serializable {
@@ -28,6 +31,13 @@ data class LeakTraceReference(
     ARRAY_ENTRY
   }
 
+  /**
+   * Returns {@link #className} without the package, ie stripped of any string content before the
+   * last period (included).
+   */
+  val owningClassSimpleName: String get() = owningClassName.lastSegment('.')
+
+
   val referenceDisplayName: String
     get() {
       return when (referenceType) {

diff --git a/shark/src/main/java/shark/internal/PathFinder.kt b/shark/src/main/java/shark/internal/PathFinder.kt
@@ -492,24 +492,25 @@ internal class PathFinder(
     val fieldNamesAndValues =
       instance.readAllNonNullFieldsOfReferenceType(classHierarchy)
 
-    fieldNamesAndValues.sortBy { it.second }
+    fieldNamesAndValues.sortBy { it.fieldName }
 
-    fieldNamesAndValues.forEach { pair ->
-      val (objectId, name) = pair
-      val node = when (val referenceMatcher = fieldReferenceMatchers[name]) {
+    fieldNamesAndValues.forEach { instanceRefField ->
+      val node = when (val referenceMatcher = fieldReferenceMatchers[instanceRefField.fieldName]) {
         null -> NormalNode(
-            objectId = objectId,
+            objectId = instanceRefField.refObjectId,
             parent = parent,
             refFromParentType = INSTANCE_FIELD,
-            refFromParentName = name
+            refFromParentName = instanceRefField.fieldName,
+            owningClassId = instanceRefField.declaringClassId
         )
         is LibraryLeakReferenceMatcher ->
           LibraryLeakChildNode(
-              objectId = objectId,
+              objectId = instanceRefField.refObjectId,
               parent = parent,
               refFromParentType = INSTANCE_FIELD,
-              refFromParentName = name,
-              matcher = referenceMatcher
+              refFromParentName = instanceRefField.fieldName,
+              matcher = referenceMatcher,
+              owningClassId = instanceRefField.declaringClassId
           )
         is IgnoredReferenceMatcher -> null
       }
@@ -519,14 +520,20 @@ internal class PathFinder(
     }
   }
 
+  private class InstanceRefField(
+    val declaringClassId: Long,
+    val refObjectId: Long,
+    val fieldName: String
+  )
+
   private fun HeapInstance.readAllNonNullFieldsOfReferenceType(
     classHierarchy: List<HeapClass>
-  ): MutableList<LongObjectPair<String>> {
+  ): MutableList<InstanceRefField> {
     // Assigning to local variable to avoid repeated lookup and cast:
     // HeapInstance.graph casts HeapInstance.hprofGraph to HeapGraph in its getter
     val hprofGraph = graph
     var fieldReader: FieldIdReader? = null
-    val result = mutableListOf<LongObjectPair<String>>()
+    val result = mutableListOf<InstanceRefField>()
     var skipBytesCount = 0
 
     for (heapClass in classHierarchy) {
@@ -546,7 +553,11 @@ internal class PathFinder(
 
           val objectId = fieldReader.readId()
           if (objectId != 0L) {
-            result.add(objectId to heapClass.instanceFieldName(fieldRecord))
+            result.add(
+                InstanceRefField(
+                    heapClass.objectId, objectId, heapClass.instanceFieldName(fieldRecord)
+                )
+            )
           }
         }
       }

diff --git a/shark/src/main/java/shark/internal/ReferencePathNode.kt b/shark/src/main/java/shark/internal/ReferencePathNode.kt
@@ -37,19 +37,27 @@ internal sealed class ReferencePathNode {
     abstract val refFromParentType: LeakTraceReference.ReferenceType
     abstract val refFromParentName: String
 
+    /**
+     * If this node is an instance, then this is the id of the class that actually
+     * declares the node.
+     */
+    abstract val owningClassId: Long
+
     class LibraryLeakChildNode(
       override val objectId: Long,
       override val parent: ReferencePathNode,
       override val refFromParentType: LeakTraceReference.ReferenceType,
       override val refFromParentName: String,
-      override val matcher: LibraryLeakReferenceMatcher
+      override val matcher: LibraryLeakReferenceMatcher,
+      override val owningClassId: Long = 0
     ) : ChildNode(), LibraryLeakNode
 
     class NormalNode(
       override val objectId: Long,
       override val parent: ReferencePathNode,
       override val refFromParentType: LeakTraceReference.ReferenceType,
-      override val refFromParentName: String
+      override val refFromParentName: String,
+      override val owningClassId: Long = 0
     ) : ChildNode()
   }
 

diff --git a/shark/src/test/java/shark/LeakTraceStringRenderingTest.kt b/shark/src/test/java/shark/LeakTraceStringRenderingTest.kt
@@ -9,6 +9,7 @@ import shark.FilteringLeakingObjectFinder.LeakingObjectFilter
 import shark.HeapObject.HeapInstance
 import shark.ReferencePattern.InstanceFieldPattern
 import shark.ReferencePattern.StaticFieldPattern
+import shark.ValueHolder.ReferenceHolder
 import java.io.File
 
 class LeakTraceStringRenderingTest {
@@ -282,6 +283,39 @@ class LeakTraceStringRenderingTest {
     """
   }
 
+  @Test fun renderFieldFromSuperClass() {
+    hprofFile.dump {
+      val leakingInstance = "Leaking" watchedInstance {}
+      val parentClassId = clazz("com.ParentClass", fields = listOf("leak" to ReferenceHolder::class))
+      val childClassId = clazz("com.ChildClass", superclassId = parentClassId)
+      "GcRoot" clazz {
+        staticField["child"] = instance(childClassId, listOf(leakingInstance))
+      }
+    }
+
+    val analysis =
+      hprofFile.checkForLeaks<HeapAnalysisSuccess>()
+
+    analysis renders """
+    ┬───
+    │ GC Root: System class
+    │
+    ├─ GcRoot class
+    │    Leaking: UNKNOWN
+    │    ↓ static GcRoot.child
+    │                    ~~~~~
+    ├─ com.ChildClass instance
+    │    Leaking: UNKNOWN
+    │    ↓ ParentClass.leak
+    │                  ~~~~
+    ╰→ Leaking instance
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
+         key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
+         watchDurationMillis = 25000
+         retainedDurationMillis = 10000
+        """
+  }
+
   private infix fun HeapAnalysisSuccess.renders(expectedString: String) {
     assertThat(applicationLeaks[0].leakTraces.first().toString()).isEqualTo(
         expectedString.trimIndent()