From 8fb4380be0e4308623aac464f9ce0a0fadfc6041 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Ricau Date: Wed, 28 Jun 2023 22:37:38 -0700 Subject: [PATCH] Better support for ActivityThread.mNewActivities --- .../java/shark/AndroidReferenceMatchers.kt | 9 +- .../shark/internal/AndroidReferenceReaders.kt | 138 ++++++++++++++++++ 2 files changed, 145 insertions(+), 2 deletions(-) diff --git a/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt b/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt index e47fbcda25..2cde2aa06a 100644 --- a/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt +++ b/shark-android/src/main/java/shark/AndroidReferenceMatchers.kt @@ -72,6 +72,9 @@ enum class AndroidReferenceMatchers { } }, + /** + * See AndroidReferenceReaders.ACTIVITY_THREAD__NEW_ACTIVITIES for more context + */ ACTIVITY_THREAD__M_NEW_ACTIVITIES { override fun add( references: MutableList @@ -79,11 +82,11 @@ enum class AndroidReferenceMatchers { references += instanceFieldLeak( "android.app.ActivityThread", "mNewActivities", description = """ - New activities are leaks by ActivityThread until the main thread becomes idle. + New activities are leaked by ActivityThread until the main thread becomes idle. Tracked here: https://issuetracker.google.com/issues/258390457 """.trimIndent() ) { - sdkInt in 19..33 + sdkInt >= 19 } } }, @@ -559,6 +562,8 @@ enum class AndroidReferenceMatchers { } }, + + BIOMETRIC_PROMPT { override fun add(references: MutableList) { references += instanceFieldLeak( diff --git a/shark/src/main/java/shark/internal/AndroidReferenceReaders.kt b/shark/src/main/java/shark/internal/AndroidReferenceReaders.kt index 9c07b384fc..15ab5bddf5 100644 --- a/shark/src/main/java/shark/internal/AndroidReferenceReaders.kt +++ b/shark/src/main/java/shark/internal/AndroidReferenceReaders.kt @@ -2,6 +2,9 @@ package shark.internal import shark.HeapGraph import shark.HeapObject.HeapInstance +import shark.LibraryLeakReferenceMatcher +import shark.ReferencePattern.InstanceFieldPattern +import shark.ValueHolder import shark.ValueHolder.ReferenceHolder import shark.internal.ChainingInstanceReferenceReader.VirtualInstanceReferenceReader import shark.internal.ChainingInstanceReferenceReader.VirtualInstanceReferenceReader.OptionalFactory @@ -11,6 +14,141 @@ import shark.internal.ReferenceLocationType.INSTANCE_FIELD internal enum class AndroidReferenceReaders : OptionalFactory { + /** + * ActivityThread.mNewActivity is a linked list of ActivityClientRecord that keeps track of + * activities after they were resumed, until the main thread is idle. This is used to report + * analytics to system_server about how long it took for the main thread to settle after + * resuming an activity. Unfortunately, if the main thread never becomes idle, all these + * new activities leak in memory. + * + * We'd normally catch these with a pattern in AndroidReferenceMatchers, and we do have + * AndroidReferenceMatchers.ACTIVITY_THREAD__M_NEW_ACTIVITIES to do that, however this matching + * only works if none of the activities alive are waiting for idle. If any activity alive is + * still waiting for idle (which all the alive activities would be if they main thread is never + * idle) then ActivityThread.mActivities will reference an ActivityClientRecord through an + * ArrayMap and because ActivityClientRecord are reused that instance will also have its nextIdle + * fields set, so we're effectively traversing the ActivityThread.mNewActivity from a completely + * different and unexpected entry point. + * + * To fix that problem of broken pattern matching, we emit the mNewActivities field when + * finding an ActivityThread instance, and because custom ref readers have priority over the + * default instance field reader, we're guaranteed that mNewActivities is enqueued before + * mActivities. Unfortunately, that also means we can't rely on AndroidReferenceMatchers as + * those aren't used here, so we recreate our own LibraryLeakReferenceMatcher. + * + * We want to traverse mNewActivities before mActivities so we can't set isLowPriority to true + * like we would for normal path tagged as source of leak. So we will prioritize going through all + * activities in mNewActivities, some of which aren't destroyed yet (and therefore not leaking). + * Going through those paths of non leaking activities, we might find other leaks though. + * This would result in us tagging unrelated leaks as part of the mNewActivities leak. To prevent + * this, we traverse ActivityThread.mNewActivities as a linked list through + * ActivityClientRecord.nextIdle as a linked list, but we emit only ActivityClientRecord.activity + * fields if such activities are destroyed, which means any live activity in + * ActivityThread.mNewActivities will be discovered through the normal field navigation process + * and should go through ActivityThread.mActivities. + */ + ACTIVITY_THREAD__NEW_ACTIVITIES { + override fun create(graph: HeapGraph): VirtualInstanceReferenceReader? { + val activityThreadClass = graph.findClassByName("android.app.ActivityThread") ?: return null + + if (activityThreadClass.readRecordFields().none { + activityThreadClass.instanceFieldName(it) == "mNewActivities" + } + ) { + return null + } + + val activityClientRecordClass = + graph.findClassByName("android.app.ActivityThread\$ActivityClientRecord") ?: return null + + val activityClientRecordFieldNames = activityClientRecordClass.readRecordFields() + .map { activityThreadClass.instanceFieldName(it) } + .toList() + + if ("nextIdle" !in activityClientRecordFieldNames || + "activity" !in activityClientRecordFieldNames) { + return null + } + + val activityThreadClassId = activityThreadClass.objectId + val activityClientRecordClassId = activityClientRecordClass.objectId + + return object : VirtualInstanceReferenceReader { + override fun matches(instance: HeapInstance) = + instance.instanceClassId == activityThreadClassId || + instance.instanceClassId == activityClientRecordClassId + + override fun read(source: HeapInstance): Sequence { + return if (source.instanceClassId == activityThreadClassId) { + val mNewActivities = + source["android.app.ActivityThread", "mNewActivities"]!!.value.asObjectId!! + if (mNewActivities == ValueHolder.NULL_REFERENCE) { + emptySequence() + } else { + source.graph.context[ACTIVITY_THREAD__NEW_ACTIVITIES.name] = mNewActivities + sequenceOf( + Reference( + valueObjectId = mNewActivities, + isLowPriority = false, + lazyDetailsResolver = { + LazyDetails( + name = "mNewActivities", + locationClassObjectId = activityThreadClassId, + locationType = INSTANCE_FIELD, + isVirtual = false, + matchedLibraryLeak = LibraryLeakReferenceMatcher( + pattern = InstanceFieldPattern( + "android.app.ActivityThread", "mNewActivities" + ), + description = """ + New activities are leaked by ActivityThread until the main thread becomes idle. + Tracked here: https://issuetracker.google.com/issues/258390457 + """.trimIndent() + ) + ) + }) + ) + } + } else { + val mNewActivities = + source.graph.context.get(ACTIVITY_THREAD__NEW_ACTIVITIES.name) + if (mNewActivities == null || source.objectId != mNewActivities) { + emptySequence() + } else { + generateSequence(source) { node -> + node["android.app.ActivityThread\$ActivityClientRecord", "nextIdle"]!!.valueAsInstance + }.withIndex().mapNotNull { (index, node) -> + + val activity = node["android.app.ActivityThread\$ActivityClientRecord", "activity"]!!.valueAsInstance + if (activity == null || + // Skip non destroyed activities. + // (!= true because we also skip if mDestroyed is missing) + activity["android.app.Activity", "mDestroyed"]?.value?.asBoolean != true + ) { + null + } else { + Reference( + valueObjectId = activity.objectId, + isLowPriority = false, + lazyDetailsResolver = { + LazyDetails( + name = "$index", + locationClassObjectId = activityClientRecordClassId, + locationType = ARRAY_ENTRY, + isVirtual = true, + matchedLibraryLeak = null + ) + }) + } + } + } + } + } + } + } + }, + + MESSAGE_QUEUE { override fun create(graph: HeapGraph): VirtualInstanceReferenceReader? { val messageQueueClass =