Merge pull request #2915 from square/py/skip-missing-static-field-references

Skip static fields that reference an object missing from the heap dump

Author: pyricau

docs/changelog.md

@@ -29,6 +29,7 @@ Releases before 2.8.1 predate these markers.
 * 💥 `HeapGraph.findObjectByIndex(0)` threw an `IllegalArgumentException` instead of returning the first object of the heap dump.
 * 💥 `LeakTrace.retainedObjectCount` and `Leak.totalRetainedObjectCount` threw a `NoSuchElementException` instead of returning null when the analysis ran without computing retained heap sizes. Both are declared nullable, and their byte size counterparts `LeakTrace.retainedHeapByteSize` and `Leak.totalRetainedHeapByteSize` do return null, so a caller that null checked the count crashed instead. `LeakCanary.Config.computeRetainedHeapSize` defaults to true, so this is about code that turns it off, and about `shark`, where `HeapAnalyzer.analyze()` defaults it to false.
 * 💥 Heap growth detection failed with *"Object id 0 not found in heap dump"* on heap dumps that contain a `java.util.LinkedList` with a null element, because the reference reader for linked lists surfaced null elements as references to object id 0.
+* 💥 [#2567](https://github.com/square/leakcanary/issues/2567) The analysis failed with *"Object id <id> not found in heap dump"* on heap dumps that contain a class which failed to load. Excluding a dependency that provides a superclass is one way to get there: the subclass can no longer be loaded, but ART still creates the array class for it, points that array class's `$class$componentType` at the class object that failed and keeps it in its class loader's class table, while the heap dumper skips it and never writes it out. The heap dump then names an object it doesn't contain, and reading the static fields of the array class surfaced a reference to it. Static fields pointing at an object missing from the heap dump are now skipped, which is what reading GC roots and object array elements already did. Reproduced on Android 14 and Android 16, so this is not limited to the Android 13 the issue was reported against.
 * 💥 [#2835](https://github.com/square/leakcanary/pull/2835) Analyzing a heap dump with hundreds of millions of objects of the same kind failed with a `NegativeArraySizeException`. Each record type was indexed into a single `ByteArray` sized as `recordCount * bytesPerEntry`, which is `Int` arithmetic and wraps negative past 2 GB: a heap dump holding 918610159 instances asked for an array of -346802823 bytes. An index that doesn't fit in one array is now spread over a list of arrays, so what bounds it is the heap the analysis runs with rather than the 2 GB an `Int` can address. Verified on a 27 GB JVM heap dump of 918623867 objects, which indexes in 60 seconds.
 * 💥 [#2777](https://github.com/square/leakcanary/issues/2777) Reading a valid heap dump holding an array whose elements take more than 2 GB — an object array of more than 268435455 element ids with 8 byte identifiers, or a `long` or `double` array of more than 268435455 elements — failed with *"Unknown tag 0x00"*. How many bytes to skip over the elements was computed as `arrayLength * elementByteSize` in `Int` arithmetic, which wraps negative that far out, so the parser skipped *backwards* and read an element id as the next record tag. `HprofPrimitiveArrayStripper` truncated the same count and wrote a corrupt heap dump. Arrays that large need a heap far bigger than a phone's, so this is about heap dumps taken from a JVM and analyzed with Shark or the Shark CLI.
 * 🐛 [#2777](https://github.com/square/leakcanary/issues/2777) An object whose record takes more than 2 GB in the heap dump reported a negative size. `HeapObject.recordSize` and the `byteSize` of an array were `Int`, narrowed from the `Long` the index holds, so a 2147483672 byte array record came back as -2147483624 and every size derived from it was wrong, including the shallow and retained sizes an analysis reports. Sizes are now `Long` from the index all the way out to `ObjectSizeCalculator`.

shark/shark/src/main/java/shark/ClassReferenceReader.kt

@@ -31,6 +31,7 @@ class ClassReferenceReader(
 
   override fun read(source: HeapClass): Sequence<Reference> {
     val ignoredStaticFields = staticFieldNameByClassName[source.name] ?: emptyMap()
+    val graph = source.graph
 
     return source.readStaticFields().mapNotNull { staticField ->
       // not non null: no null + no primitives.
@@ -52,6 +53,15 @@ class ClassReferenceReader(
       // Note: instead of calling staticField.value.asObjectId!! we cast holder to ReferenceHolder
       // and access value directly. This allows us to avoid unnecessary boxing of Long.
       val valueObjectId = (staticField.value.holder as ReferenceHolder).value
+
+      // A class can reference an object that the heap dump doesn't contain: when a class fails to
+      // load, ART creates the matching array class anyway and points its $class$componentType at
+      // the class object that failed, yet never dumps that class object. Reading GC roots and
+      // object array elements skips missing objects for the same reason. See #2567.
+      if (!graph.objectExists(valueObjectId)) {
+        return@mapNotNull null
+      }
+
       val referenceMatcher = ignoredStaticFields[fieldName]
 
       if (referenceMatcher is IgnoredReferenceMatcher) {

shark/shark/src/test/java/shark/DanglingReferenceTest.kt

@@ -0,0 +1,53 @@
+package shark
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Test
+import shark.ValueHolder.ReferenceHolder
+
+/**
+ * A heap dump can reference an object it doesn't contain: when a class fails to load, ART keeps
+ * creating the matching array class and points its `$class$componentType` at the class object that
+ * failed, yet never dumps that class object. See
+ * [#2567](https://github.com/square/leakcanary/issues/2567).
+ */
+class DanglingReferenceTest {
+
+  /**
+   * An object id that no record in the heap dump defines, well above the ids the DSL hands out.
+   */
+  private val danglingId = 123456789L
+
+  @Test fun `dangling static field reference is skipped`() {
+    val heapDump = dump {
+      "GcRoot" clazz {
+        staticField["leak"] = "Leaking" watchedInstance {}
+      }
+      "ArrayClass" clazz {
+        staticField["\$class\$componentType"] = ReferenceHolder(danglingId)
+      }
+    }
+
+    // Computing retained sizes makes the traversal exhaustive, so it reaches the array class.
+    // Without it the traversal stops as soon as it has found every leaking object.
+    val analysis = heapDump.checkForLeaks<HeapAnalysis>(computeRetainedHeapSize = true)
+
+    assertThat(analysis).isInstanceOf(HeapAnalysisSuccess::class.java)
+    assertThat((analysis as HeapAnalysisSuccess).applicationLeaks.flatMap { it.leakTraces })
+      .hasSize(1)
+  }
+
+  @Test fun `dangling object array entry is skipped`() {
+    val heapDump = dump {
+      "GcRoot" clazz {
+        staticField["array"] = objectArray(
+          ReferenceHolder(danglingId),
+          "Leaking" watchedInstance {}
+        )
+      }
+    }
+
+    val analysis = heapDump.checkForLeaks<HeapAnalysisSuccess>()
+
+    assertThat(analysis.applicationLeaks.flatMap { it.leakTraces }).hasSize(1)
+  }
+}