Skip to content

Commit

Permalink
Ref patterns for native global variables (#1570)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pyricau
pyricau committed Sep 10, 2019
1 parent 973c8ef commit 879ab7c
Show file tree
Hide file tree
Showing 6 changed files with 145 additions and 46 deletions.
66 changes: 40 additions & 26 deletions shark-android/src/main/java/shark/AndroidReferenceMatchers.kt
View file
Expand Up @@ -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
Expand Down Expand Up @@ -667,6 +668,28 @@ enum class AndroidReferenceMatchers {
}
},

CONTROLLED_INPUT_CONNECTION_WRAPPER {
override fun add(references: MutableList<ReferenceMatcher>) {
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<ReferenceMatcher>) {
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
Expand Down Expand Up @@ -709,35 +732,18 @@ enum class AndroidReferenceMatchers {
override fun add(
references: MutableList<ReferenceMatcher>
) {
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
}
}
},
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions shark/src/main/java/shark/HeapAnalyzer.kt
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 6 additions & 0 deletions shark/src/main/java/shark/ReferencePattern.kt
View file
Expand Up @@ -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"
}
}
}
48 changes: 37 additions & 11 deletions shark/src/main/java/shark/internal/PathFinder.kt
View file
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -106,11 +111,13 @@ internal class PathFinder(
private val fieldNameByClassName: Map<String, Map<String, ReferenceMatcher>>
private val staticFieldNameByClassName: Map<String, Map<String, ReferenceMatcher>>
private val threadNameReferenceMatchers: Map<String, ReferenceMatcher>
private val jniGlobalReferenceMatchers: Map<String, ReferenceMatcher>

init {
val fieldNameByClassName = mutableMapOf<String, MutableMap<String, ReferenceMatcher>>()
val staticFieldNameByClassName = mutableMapOf<String, MutableMap<String, ReferenceMatcher>>()
val threadNames = mutableMapOf<String, ReferenceMatcher>()
val jniGlobals = mutableMapOf<String, ReferenceMatcher>()

referenceMatchers.filter {
(it is IgnoredReferenceMatcher || (it is LibraryLeakReferenceMatcher && it.patternApplies(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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))
}
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
)
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
29 changes: 22 additions & 7 deletions shark/src/main/java/shark/internal/ReferencePathNode.kt
View file
Expand Up @@ -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() {

Expand All @@ -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,
Expand Down
38 changes: 38 additions & 0 deletions shark/src/test/java/shark/ReferenceMatcherTest.kt
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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<HeapAnalysisSuccess>(
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<HeapAnalysisSuccess>(
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")
}

}

0 comments on commit 879ab7c

Please sign in to comment.