From 879ab7ce807c3d6a3c3a7674cd559df772276d8b Mon Sep 17 00:00:00 2001 From: PY Date: Tue, 10 Sep 2019 16:27:12 -0700 Subject: [PATCH] Ref patterns for native global variables (#1570) Leaks are often caused by native global variables holding references to live objects. On Android this is super common due to the IPC mechanism holding on to instances until the stub in the other process is garbage collected. Fixes #1562 --- .../java/shark/AndroidReferenceMatchers.kt | 66 +++++++++++-------- shark/src/main/java/shark/HeapAnalyzer.kt | 4 +- shark/src/main/java/shark/ReferencePattern.kt | 6 ++ .../main/java/shark/internal/PathFinder.kt | 48 ++++++++++---- .../java/shark/internal/ReferencePathNode.kt | 29 ++++++-- .../test/java/shark/ReferenceMatcherTest.kt | 38 +++++++++++ 6 files changed, 145 insertions(+), 46 deletions(-) diff --git a/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt b/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt index 46bb84f5dc..9ea0b94192 100644 --- a/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt +++ b/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt @@ -19,6 +19,7 @@ import shark.AndroidReferenceMatchers.Companion.appDefaults import shark.AndroidReferenceMatchers.Companion.buildKnownReferences import shark.ReferencePattern.InstanceFieldPattern import shark.ReferencePattern.JavaLocalPattern +import shark.ReferencePattern.NativeGlobalVariablePattern import shark.ReferencePattern.StaticFieldPattern import java.lang.ref.PhantomReference import java.lang.ref.SoftReference @@ -667,6 +668,28 @@ enum class AndroidReferenceMatchers { } }, + CONTROLLED_INPUT_CONNECTION_WRAPPER { + override fun add(references: MutableList) { + references += nativeGlobalVariableLeak( + "android.view.inputmethod.InputMethodManager\$ControlledInputConnectionWrapper", + description = """ + ControlledInputConnectionWrapper is held by a global variable in native code. + """.trimIndent() + ) + } + }, + + TOAST_TN { + override fun add(references: MutableList) { + references += nativeGlobalVariableLeak( + "android.widget.Toast\$TN", + description = """ + Toast.TN is held by a global variable in native code due to an IPC call to show the toast. + """.trimIndent() + ) + } + }, + // ######## Manufacturer specific known leaks ######## // SAMSUNG @@ -709,35 +732,18 @@ enum class AndroidReferenceMatchers { override fun add( references: MutableList ) { - references += instanceFieldLeak( - "com.samsung.android.content.clipboard.SemClipboardManager", - "mContext" - , - description = - "SemClipboardManager is held in memory by an anonymous inner class" + - " implementation of android.os.Binder, thereby leaking an activity context." + val description = """ + SemClipboardManager inner classes are held by native references due to IPC calls + """.trimIndent() + references += nativeGlobalVariableLeak( + "com.samsung.android.content.clipboard.SemClipboardManager$1", description ) { - manufacturer == SAMSUNG && sdkInt in 19..24 - } - references += instanceFieldLeak( - "com.samsung.android.content.clipboard.SemClipboardManager$3", - "this$0" - , - description = - "SemClipboardManager is held in memory by an anonymous inner class" + - " implementation of android.os.Binder, thereby leaking an activity context." - ) { - manufacturer == SAMSUNG && sdkInt in 22..28 + manufacturer == SAMSUNG && sdkInt in 19..28 } - references += instanceFieldLeak( - "com.samsung.android.content.clipboard.SemClipboardManager$1", - "this$0" - , - description = - "SemClipboardManager is held in memory by an anonymous inner class" + - " implementation of android.os.Binder, thereby leaking an activity context." + references += nativeGlobalVariableLeak( + "com.samsung.android.content.clipboard.SemClipboardManager$3", description ) { - manufacturer == SAMSUNG && sdkInt == 24 + manufacturer == SAMSUNG && sdkInt in 19..28 } } }, @@ -1239,6 +1245,14 @@ enum class AndroidReferenceMatchers { return libraryLeak(InstanceFieldPattern(className, fieldName), description, patternApplies) } + fun nativeGlobalVariableLeak( + className: String, + description: String = "", + patternApplies: AndroidBuildMirror.() -> Boolean = ALWAYS + ): LibraryLeakReferenceMatcher { + return libraryLeak(NativeGlobalVariablePattern(className), description, patternApplies) + } + private fun libraryLeak( referencePattern: ReferencePattern, description: String, diff --git a/shark/src/main/java/shark/HeapAnalyzer.kt b/shark/src/main/java/shark/HeapAnalyzer.kt index 791af7e0e8..1ad4ca5bb9 100644 --- a/shark/src/main/java/shark/HeapAnalyzer.kt +++ b/shark/src/main/java/shark/HeapAnalyzer.kt @@ -48,7 +48,7 @@ import shark.internal.PathFinder import shark.internal.PathFinder.PathFindingResults import shark.internal.ReferencePathNode import shark.internal.ReferencePathNode.ChildNode -import shark.internal.ReferencePathNode.ChildNode.LibraryLeakNode +import shark.internal.ReferencePathNode.LibraryLeakNode import shark.internal.ReferencePathNode.RootNode import shark.internal.lastSegment import java.io.File @@ -353,7 +353,7 @@ class HeapAnalyzer constructor( val className = recordClassName(graph.findObjectById(pathNode.objectId)) - val firstLibraryLeakNode = + val firstLibraryLeakNode = if (rootNode is LibraryLeakNode) rootNode else shortestChildPath.firstOrNull { it is LibraryLeakNode } as LibraryLeakNode? if (firstLibraryLeakNode != null) { diff --git a/shark/src/main/java/shark/ReferencePattern.kt b/shark/src/main/java/shark/ReferencePattern.kt index 9de31795a1..4482412651 100644 --- a/shark/src/main/java/shark/ReferencePattern.kt +++ b/shark/src/main/java/shark/ReferencePattern.kt @@ -46,4 +46,10 @@ sealed class ReferencePattern : Serializable { return "instance field $className#$fieldName" } } + + data class NativeGlobalVariablePattern(val className: String) : ReferencePattern() { + override fun toString(): String { + return "native global variable referencing $className" + } + } } \ No newline at end of file diff --git a/shark/src/main/java/shark/internal/PathFinder.kt b/shark/src/main/java/shark/internal/PathFinder.kt index 5ea4440420..30bb3bfe78 100644 --- a/shark/src/main/java/shark/internal/PathFinder.kt +++ b/shark/src/main/java/shark/internal/PathFinder.kt @@ -17,6 +17,7 @@ package shark.internal import shark.GcRoot import shark.GcRoot.JavaFrame +import shark.GcRoot.JniGlobal import shark.GcRoot.ThreadObject import shark.HeapGraph import shark.HeapObject @@ -38,12 +39,16 @@ import shark.PrimitiveType.INT import shark.ReferenceMatcher import shark.ReferencePattern import shark.ReferencePattern.InstanceFieldPattern +import shark.ReferencePattern.NativeGlobalVariablePattern import shark.ReferencePattern.StaticFieldPattern import shark.SharkLog import shark.ValueHolder -import shark.internal.ReferencePathNode.ChildNode.LibraryLeakNode +import shark.internal.ReferencePathNode.ChildNode.LibraryLeakChildNode import shark.internal.ReferencePathNode.ChildNode.NormalNode +import shark.internal.ReferencePathNode.LibraryLeakNode import shark.internal.ReferencePathNode.RootNode +import shark.internal.ReferencePathNode.RootNode.LibraryLeakRootNode +import shark.internal.ReferencePathNode.RootNode.NormalRootNode import shark.internal.hppc.LongLongScatterMap import shark.internal.hppc.LongScatterSet import java.util.ArrayDeque @@ -106,11 +111,13 @@ internal class PathFinder( private val fieldNameByClassName: Map> private val staticFieldNameByClassName: Map> private val threadNameReferenceMatchers: Map + private val jniGlobalReferenceMatchers: Map init { val fieldNameByClassName = mutableMapOf>() val staticFieldNameByClassName = mutableMapOf>() val threadNames = mutableMapOf() + val jniGlobals = mutableMapOf() referenceMatchers.filter { (it is IgnoredReferenceMatcher || (it is LibraryLeakReferenceMatcher && it.patternApplies( @@ -140,11 +147,15 @@ internal class PathFinder( } map[pattern.fieldName] = referenceMatcher } + is NativeGlobalVariablePattern -> { + jniGlobals[pattern.className] = referenceMatcher + } } } this.fieldNameByClassName = fieldNameByClassName this.staticFieldNameByClassName = staticFieldNameByClassName this.threadNameReferenceMatchers = threadNames + this.jniGlobalReferenceMatchers = jniGlobals } fun findPathsFromGcRoots( @@ -243,35 +254,49 @@ internal class PathFinder( when (gcRoot) { is ThreadObject -> { threadsBySerialNumber[gcRoot.threadSerialNumber] = objectRecord.asInstance!! to gcRoot - enqueue(RootNode(gcRoot, gcRoot.id)) + enqueue(NormalRootNode(gcRoot.id, gcRoot)) } is JavaFrame -> { val (threadInstance, threadRoot) = threadsBySerialNumber.getValue( gcRoot.threadSerialNumber ) val threadName = threadNames[threadInstance] ?: { - val name = threadInstance[Thread::class, "name"]?.value?.readAsJavaString()?:"" + val name = threadInstance[Thread::class, "name"]?.value?.readAsJavaString() ?: "" threadNames[threadInstance] = name name }() val referenceMatcher = threadNameReferenceMatchers[threadName] if (referenceMatcher !is IgnoredReferenceMatcher) { - val rootNode = RootNode(gcRoot, threadRoot.id) + val rootNode = NormalRootNode(threadRoot.id, gcRoot) // Unfortunately Android heap dumps do not include stack trace data, so // JavaFrame.frameNumber is always -1 and we cannot know which method is causing the // reference to be held. val leakReference = LeakReference(LOCAL, "") val childNode = if (referenceMatcher is LibraryLeakReferenceMatcher) { - LibraryLeakNode(gcRoot.id, rootNode, leakReference, referenceMatcher) + LibraryLeakChildNode(gcRoot.id, rootNode, leakReference, referenceMatcher) } else { NormalNode(gcRoot.id, rootNode, leakReference) } enqueue(childNode) } } - else -> enqueue(RootNode(gcRoot, gcRoot.id)) + is JniGlobal -> { + val referenceMatcher = when (objectRecord) { + is HeapClass -> jniGlobalReferenceMatchers[objectRecord.name] + is HeapInstance -> jniGlobalReferenceMatchers[objectRecord.instanceClassName] + is HeapObjectArray -> jniGlobalReferenceMatchers[objectRecord.arrayClassName] + is HeapPrimitiveArray -> jniGlobalReferenceMatchers[objectRecord.arrayClassName] + } + if (referenceMatcher !is IgnoredReferenceMatcher) { + if (referenceMatcher is LibraryLeakReferenceMatcher) + enqueue(LibraryLeakRootNode(gcRoot.id, gcRoot, referenceMatcher)) + } else { + enqueue(NormalRootNode(gcRoot.id, gcRoot)) + } + } + else -> enqueue(NormalRootNode(gcRoot.id, gcRoot)) } } } @@ -307,7 +332,7 @@ internal class PathFinder( val objectExists = graph.objectExists(gcRoot.id) if (!objectExists) { SharkLog.d { - "${gcRoot::class.java.simpleName} gc root ignored because it's pointing to unknown object @${gcRoot.id}" + "${gcRoot::class.java.simpleName} gc root ignored because it's pointing to unknown object @${gcRoot.id}" } } objectExists @@ -348,7 +373,7 @@ internal class PathFinder( val node = when (val referenceMatcher = ignoredStaticFields[fieldName]) { null -> NormalNode(objectId, parent, LeakReference(STATIC_FIELD, fieldName)) - is LibraryLeakReferenceMatcher -> LibraryLeakNode( + is LibraryLeakReferenceMatcher -> LibraryLeakChildNode( objectId, parent, LeakReference(STATIC_FIELD, fieldName), referenceMatcher ) is IgnoredReferenceMatcher -> null @@ -397,7 +422,7 @@ internal class PathFinder( if (threadLocalValuesMatcher != null && field.declaringClass.name == "java.lang.Thread" && field.name == "localValues") { // Earlier Android versions store local references in a Thread.localValues field. if (threadLocalValuesMatcher is LibraryLeakReferenceMatcher) { - LibraryLeakNode( + LibraryLeakChildNode( objectId, parent, LeakReference(INSTANCE_FIELD, field.name), threadLocalValuesMatcher ) @@ -407,7 +432,7 @@ internal class PathFinder( } else when (val referenceMatcher = fieldReferenceMatchers[field.name]) { null -> NormalNode(objectId, parent, LeakReference(INSTANCE_FIELD, field.name)) is LibraryLeakReferenceMatcher -> - LibraryLeakNode( + LibraryLeakChildNode( objectId, parent, LeakReference(INSTANCE_FIELD, field.name), referenceMatcher ) is IgnoredReferenceMatcher -> null @@ -572,7 +597,8 @@ internal class PathFinder( } return } - val nextDominator = if (parentIsRetainedObject) parent else dominatedObjectIds.getSlotValue(parentDominatorSlot) + val nextDominator = + if (parentIsRetainedObject) parent else dominatedObjectIds.getSlotValue(parentDominatorSlot) if (currentDominatorSlot == -1) { dominatedObjectIds[objectId] = nextDominator } else { diff --git a/shark/src/main/java/shark/internal/ReferencePathNode.kt b/shark/src/main/java/shark/internal/ReferencePathNode.kt index 3e2c0e620e..bf83a23b52 100644 --- a/shark/src/main/java/shark/internal/ReferencePathNode.kt +++ b/shark/src/main/java/shark/internal/ReferencePathNode.kt @@ -7,10 +7,25 @@ import shark.LibraryLeakReferenceMatcher internal sealed class ReferencePathNode { abstract val objectId: Long - class RootNode( - val gcRoot: GcRoot, - override val objectId: Long - ) : ReferencePathNode() + interface LibraryLeakNode { + val matcher: LibraryLeakReferenceMatcher + } + + sealed class RootNode : ReferencePathNode() { + abstract val gcRoot: GcRoot + + class LibraryLeakRootNode( + override val objectId: Long, + override val gcRoot: GcRoot, + override val matcher: LibraryLeakReferenceMatcher + ) : RootNode(), LibraryLeakNode + + class NormalRootNode( + override val objectId: Long, + override val gcRoot: GcRoot + ) : RootNode() + + } sealed class ChildNode : ReferencePathNode() { @@ -21,12 +36,12 @@ internal sealed class ReferencePathNode { */ abstract val referenceFromParent: LeakReference - class LibraryLeakNode( + class LibraryLeakChildNode( override val objectId: Long, override val parent: ReferencePathNode, override val referenceFromParent: LeakReference, - val matcher: LibraryLeakReferenceMatcher - ) : ChildNode() + override val matcher: LibraryLeakReferenceMatcher + ) : ChildNode(), LibraryLeakNode class NormalNode( override val objectId: Long, diff --git a/shark/src/test/java/shark/ReferenceMatcherTest.kt b/shark/src/test/java/shark/ReferenceMatcherTest.kt index bf1c10076a..f9831986de 100644 --- a/shark/src/test/java/shark/ReferenceMatcherTest.kt +++ b/shark/src/test/java/shark/ReferenceMatcherTest.kt @@ -5,9 +5,12 @@ import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder +import shark.GcRoot.JniGlobal import shark.ReferencePattern.InstanceFieldPattern import shark.ReferencePattern.JavaLocalPattern +import shark.ReferencePattern.NativeGlobalVariablePattern import shark.ReferencePattern.StaticFieldPattern +import shark.ValueHolder.ReferenceHolder import java.io.File import java.lang.ref.WeakReference @@ -116,4 +119,39 @@ class ReferenceMatcherTest { assertThat(analysis.libraryLeaks).isEmpty() } + @Test fun nativeGlobalVariableLibraryLeak() { + hprofFile.dump { + gcRoot(JniGlobal(id = "Leaking".watchedInstance {}.value, jniGlobalRefId = 42)) + } + + val matcher = LibraryLeakReferenceMatcher(NativeGlobalVariablePattern("Leaking")) + val analysis = hprofFile.checkForLeaks( + referenceMatchers = listOf(matcher) + ) + val leak = analysis.libraryLeaks[0] + assertThat(leak.pattern).isEqualTo(matcher.pattern) + } + + @Test fun nativeGlobalVariableShortestPathExcluded() { + hprofFile.dump { + val leaking = instance(clazz("Leaking")) + keyedWeakReference(leaking) + val hasLeaking = instance( + clazz("HasLeaking", fields = listOf("leaking" to ReferenceHolder::class)), + fields = listOf(leaking) + ) + clazz("GcRoot", staticFields = listOf("longestPath" to hasLeaking)) + gcRoot(JniGlobal(id = leaking.value, jniGlobalRefId = 42)) + } + + val matcher = LibraryLeakReferenceMatcher(NativeGlobalVariablePattern("Leaking")) + val analysis = hprofFile.checkForLeaks( + referenceMatchers = listOf(matcher) + ) + val leak = analysis.applicationLeaks[0] + assertThat(leak.leakTrace.elements).hasSize(3) + assertThat(leak.leakTrace.elements[0].className).isEqualTo("GcRoot") + assertThat(leak.leakTrace.elements[0].reference!!.name).isEqualTo("longestPath") + } + } \ No newline at end of file