From a6c589c94f73daac4b3e379ba0150235db9e067e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Ricau Date: Wed, 14 Oct 2020 22:42:30 -0700 Subject: [PATCH] Report super class holding field in leaktrace Fixes #1972 --- .../leakcanary/internal/DisplayLeakAdapter.kt | 8 ++++- shark/src/main/java/shark/HeapAnalyzer.kt | 9 +++-- shark/src/main/java/shark/LeakReference.kt | 1 + shark/src/main/java/shark/LeakTrace.kt | 2 +- .../src/main/java/shark/LeakTraceReference.kt | 10 ++++++ .../main/java/shark/internal/PathFinder.kt | 35 ++++++++++++------- .../java/shark/internal/ReferencePathNode.kt | 12 +++++-- .../shark/LeakTraceStringRenderingTest.kt | 34 ++++++++++++++++++ 8 files changed, 93 insertions(+), 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 index e14f897054..844fcd76eb 100644 --- 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 "$simpleName" } + private fun LeakTraceReference.styledOwningClassSimpleName(): String { + val simpleName = owningClassSimpleName.replace("[]", "[ ]") + return "$simpleName" + } + @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 index b8c23cb2ef..3993ce4e87 100644 --- 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, inspectedObjectsByPath: List>, retainedSizes: Map>? @@ -424,7 +424,7 @@ class HeapAnalyzer constructor( } } - private fun buildReferencePath( + private fun FindLeakInput.buildReferencePath( shortestChildPath: List, leakTraceObjects: List ): List { @@ -432,6 +432,11 @@ class HeapAnalyzer constructor( 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 index 20e13f7ced..69281101af 100644 --- 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 index 97a67fc172..8075bd72f7 100644 --- 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 index 049dcd42b9..0b305232bd 100644 --- 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 index 641c40c90c..7ca95bca3d 100644 --- 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 - ): MutableList> { + ): MutableList { // 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>() + val result = mutableListOf() 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 index be6b6b7098..188a984770 100644 --- 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 index a627d5d535..e840cb71c8 100644 --- 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() + + 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()