Merge pull request #2597 from square/py/better_invalid_id_error

Provide reference path on invalid object id error

config/hooks/pre-push

@@ -3,7 +3,7 @@
 echo "Running static analysis..."
 
 # Run static analysis tools
-./gradlew detekt
+./gradlew detekt --no-configuration-cache
 
 status=$?
 

shark-cli.sh

@@ -1,2 +1,2 @@
-./gradlew --quiet :shark:shark-cli:installDist
+./gradlew --quiet --no-configuration-cache :shark:shark-cli:installDist
 ./shark/shark-cli/build/install/shark-cli/bin/shark-cli "$@"

shark/shark/api/shark.api

@@ -752,3 +752,7 @@ public final class shark/internal/ReferencePathNode$RootNode$NormalRootNode : sh
 	public fun getGcRoot ()Lshark/GcRoot;
 }
 
+public final class shark/internal/ReferencePathNodeKt {
+	public static final fun invalidObjectIdErrorMessage (Lshark/HeapGraph;Lshark/internal/ReferencePathNode;)Ljava/lang/String;
+}
+

shark/shark/src/main/java/shark/PrioritizingShortestPathFinder.kt

@@ -14,6 +14,7 @@ import shark.internal.ReferencePathNode.RootNode
 import shark.internal.ReferencePathNode.RootNode.LibraryLeakRootNode
 import shark.internal.ReferencePathNode.RootNode.NormalRootNode
 import shark.internal.hppc.LongScatterSet
+import shark.internal.invalidObjectIdErrorMessage
 
 /**
  * Not thread safe.
@@ -177,7 +178,13 @@ class PrioritizingShortestPathFinder private constructor(
         }
       }
 
-      val heapObject = graph.findObjectById(node.objectId)
+      val heapObject = try {
+        graph.findObjectById(node.objectId)
+      } catch (objectIdNotFound: IllegalArgumentException) {
+        // This should never happen (a heap should only have references to objects that exist)
+        // but when it does happen, let's at least display how we got there.
+        throw RuntimeException(graph.invalidObjectIdErrorMessage(node), objectIdNotFound)
+      }
       objectReferenceReader.read(heapObject).forEach { reference ->
         val newNode = ChildNode(
           objectId = reference.valueObjectId,

shark/shark/src/main/java/shark/internal/ReferencePathNode.kt

@@ -1,8 +1,28 @@
 package shark.internal
 
 import shark.GcRoot
+import shark.HeapGraph
+import shark.HeapObject.HeapClass
+import shark.HeapObject.HeapInstance
+import shark.HeapObject.HeapObjectArray
+import shark.HeapObject.HeapPrimitiveArray
+import shark.LeakTrace
+import shark.LeakTrace.GcRootType
+import shark.LeakTraceObject
+import shark.LeakTraceObject.LeakingStatus.UNKNOWN
+import shark.LeakTraceObject.ObjectType.ARRAY
+import shark.LeakTraceObject.ObjectType.CLASS
+import shark.LeakTraceObject.ObjectType.INSTANCE
+import shark.LeakTraceReference
+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.LibraryLeakReferenceMatcher
 import shark.Reference.LazyDetails
+import shark.ReferenceLocationType
+import shark.internal.ReferencePathNode.ChildNode
+import shark.internal.ReferencePathNode.RootNode
 
 sealed class ReferencePathNode {
   abstract val objectId: Long
@@ -31,3 +51,75 @@ sealed class ReferencePathNode {
     val lazyDetailsResolver: LazyDetails.Resolver,
   ) : ReferencePathNode()
 }
+
+fun HeapGraph.invalidObjectIdErrorMessage(node: ReferencePathNode): String {
+  // This should never happen (a heap should only have references to objects that exist)
+  // but when it does happen, let's at least display how we got there.
+  return when (node) {
+    is ChildNode -> {
+      val childPath = mutableListOf<ChildNode>()
+      var iteratingNode = node
+      while (iteratingNode is ChildNode) {
+        childPath.add(0, iteratingNode)
+        iteratingNode = iteratingNode.parent
+      }
+      val rootNode = iteratingNode as RootNode
+      val childPathWithDetails = childPath.map { it to it.lazyDetailsResolver.resolve() }
+      val leakTraceObjects = (listOf(rootNode) + childPath.dropLast(1)).map {
+        val heapObject = findObjectById(it.objectId)
+        val className = when (heapObject) {
+          is HeapClass -> heapObject.name
+          is HeapInstance -> heapObject.instanceClassName
+          is HeapObjectArray -> heapObject.arrayClassName
+          is HeapPrimitiveArray -> heapObject.arrayClassName
+        }
+        val objectType = when (heapObject) {
+          is HeapClass -> CLASS
+          is HeapObjectArray, is HeapPrimitiveArray -> ARRAY
+          else -> INSTANCE
+        }
+        LeakTraceObject(
+          type = objectType,
+          className = className,
+          labels = emptySet(),
+          leakingStatus = UNKNOWN,
+          leakingStatusReason = "",
+          retainedHeapByteSize = null,
+          retainedObjectCount = null
+        )
+      } + LeakTraceObject(
+        type = INSTANCE,
+        className = "UnknownObject${node.objectId}",
+        labels = emptySet(),
+        leakingStatus = UNKNOWN,
+        leakingStatusReason = "",
+        retainedHeapByteSize = null,
+        retainedObjectCount = null
+      )
+      val referencePath = childPathWithDetails.mapIndexed { index, (_, details) ->
+        LeakTraceReference(
+          originObject = leakTraceObjects[index],
+          referenceType = when (details.locationType) {
+            ReferenceLocationType.INSTANCE_FIELD -> INSTANCE_FIELD
+            ReferenceLocationType.STATIC_FIELD -> STATIC_FIELD
+            ReferenceLocationType.LOCAL -> LOCAL
+            ReferenceLocationType.ARRAY_ENTRY -> ARRAY_ENTRY
+          },
+          owningClassName = findObjectById(details.locationClassObjectId).asClass!!.name,
+          referenceName = details.name
+        )
+      }
+      val leakTrace = LeakTrace(
+        gcRootType = GcRootType.fromGcRoot(rootNode.gcRoot),
+        referencePath = referencePath,
+        leakingObject = leakTraceObjects.last()
+      )
+      return "Invalid object id reached through path:\n${leakTrace.toSimplePathString()}"
+    }
+
+    is RootNode -> {
+      val rootType = GcRootType.fromGcRoot(node.gcRoot)
+      "Invalid object id for root ${rootType.name}"
+    }
+  }
+}