Object watcher description (#1658)

Originally KeyedWeakReference had a "name" field that wasn't used out of the box but could be useful to add identifying information. This PR renames it to "description" and uses it for automatic watching, surfacing more clearly why an object is watched. Related to #1656
square · Dec 9, 2019 · 599011e · 599011e
1 parent de916b3
commit 599011e
Show file tree

Hide file tree

Showing 16 changed files with 62 additions and 43 deletions.
diff --git a/docs/fundamentals.md b/docs/fundamentals.md
@@ -28,7 +28,7 @@ Memory leaks are very common in Android apps and the accumulation of small memor
 The foundation of LeakCanary is a library called *ObjectWatcher Android*. It hooks into the Android lifecycle to automatically detect when activities and fragments are destroyed and should be garbage collected. These destroyed objects are passed to an `ObjectWatcher`, which holds **weak references** to them. You can also watch any objects that is no longer needed, for example a detached view, a destroyed presenter, etc.
 
 ```kotlin
-AppWatcher.objectWatcher.watch(myDetachedView)
+AppWatcher.objectWatcher.watch(myDetachedView, "View was detached")
 ```
 
 If the weak references aren't cleared after **waiting 5 seconds** and running the garbage collector, the watched objects are considered **retained**, and potentially leaking. LeakCanary logs this to Logcat:

diff --git a/docs/recipes.md b/docs/recipes.md
@@ -14,7 +14,7 @@ class MyService : Service {
 
   override fun onDestroy() {
     super.onDestroy()
-    AppWatcher.objectWatcher.watch(this)
+    AppWatcher.objectWatcher.watch(this, "MyService received Service#onDestroy() callback")
   }
 }
 ```

diff --git a/docs/upgrading-to-leakcanary-2.0.md b/docs/upgrading-to-leakcanary-2.0.md
@@ -103,18 +103,18 @@ val retainedObjectCount = AppWatcher.objectWatcher.retainedObjectCount
 ```kotlin
 // In shared code
 interface MaybeObjectWatcher {
-  fun watch(watchedObject: Any)
+  fun watch(watchedObject: Any, description: String)
 
   object None : MaybeObjectWatcher {
-    override fun watch(watchedObject: Any) {
+    override fun watch(watchedObject: Any, description: String) {
     }
   }
 }
 
 // In debug code
 class RealObjectWatcher : MaybeObjectWatcher {
-  override fun watch(watchedObject: Any) {
-    AppWatcher.objectWatcher.watch(watchedObject)
+  override fun watch(watchedObject: Any, description: String) {
+    AppWatcher.objectWatcher.watch(watchedObject, description)
   }
 }
 ```

diff --git a/...ndroid-instrumentation/src/androidTest/java/leakcanary/InstrumentationLeakDetectorTest.kt b/...ndroid-instrumentation/src/androidTest/java/leakcanary/InstrumentationLeakDetectorTest.kt
@@ -25,7 +25,7 @@ class InstrumentationLeakDetectorTest {
   @Test fun detectsLeak() {
     leaking = Date()
     val refWatcher = AppWatcher.objectWatcher
-    refWatcher.watch(leaking)
+    refWatcher.watch(leaking, "This date should not live beyond the test")
     assertLeak(Date::class.java)
   }
 

diff --git a/...cher-android-androidx/src/main/java/leakcanary/internal/AndroidXFragmentDestroyWatcher.kt b/...cher-android-androidx/src/main/java/leakcanary/internal/AndroidXFragmentDestroyWatcher.kt
@@ -35,7 +35,10 @@ internal class AndroidXFragmentDestroyWatcher(
     ) {
       val view = fragment.view
       if (view != null && configProvider().watchFragmentViews) {
-        objectWatcher.watch(view)
+        objectWatcher.watch(
+            view, "Fragment received Fragment#onDestroyView() callback " +
+            "(references to its view should be cleared to prevent leaks)"
+        )
       }
     }
 
@@ -44,7 +47,7 @@ internal class AndroidXFragmentDestroyWatcher(
       fragment: Fragment
     ) {
       if (configProvider().watchFragments) {
-        objectWatcher.watch(fragment)
+        objectWatcher.watch(fragment, "Fragment received Fragment#onDestroy() callback")
       }
     }
   }

diff --git a/...pport-fragments/src/main/java/leakcanary/internal/AndroidSupportFragmentDestroyWatcher.kt b/...pport-fragments/src/main/java/leakcanary/internal/AndroidSupportFragmentDestroyWatcher.kt
@@ -35,7 +35,10 @@ internal class AndroidSupportFragmentDestroyWatcher(
     ) {
       val view = fragment.view
       if (view != null && configProvider().watchFragmentViews) {
-        objectWatcher.watch(view)
+        objectWatcher.watch(
+            view, "Fragment received Fragment#onDestroyView() callback " +
+            "(references to its view should be cleared to prevent leaks)"
+        )
       }
     }
 
@@ -44,7 +47,7 @@ internal class AndroidSupportFragmentDestroyWatcher(
       fragment: Fragment
     ) {
       if (configProvider().watchFragments) {
-        objectWatcher.watch(fragment)
+        objectWatcher.watch(fragment, "Fragment received Fragment#onDestroy() callback")
       }
     }
   }

diff --git a/...canary-object-watcher-android/src/main/java/leakcanary/internal/ActivityDestroyWatcher.kt b/...canary-object-watcher-android/src/main/java/leakcanary/internal/ActivityDestroyWatcher.kt
@@ -30,7 +30,7 @@ internal class ActivityDestroyWatcher private constructor(
     object : Application.ActivityLifecycleCallbacks by noOpDelegate() {
       override fun onActivityDestroyed(activity: Activity) {
         if (configProvider().watchActivities) {
-          objectWatcher.watch(activity)
+          objectWatcher.watch(activity, "Activity received Activity#onDestroy() callback")
         }
       }
     }

diff --git a/...bject-watcher-android/src/main/java/leakcanary/internal/AndroidOFragmentDestroyWatcher.kt b/...bject-watcher-android/src/main/java/leakcanary/internal/AndroidOFragmentDestroyWatcher.kt
@@ -37,7 +37,10 @@ internal class AndroidOFragmentDestroyWatcher(
     ) {
       val view = fragment.view
       if (view != null && configProvider().watchFragmentViews) {
-        objectWatcher.watch(view)
+        objectWatcher.watch(
+            view, "Fragment received Fragment#onDestroyView() callback " +
+            "(references to its view should be cleared to prevent leaks)"
+        )
       }
     }
 
@@ -46,7 +49,7 @@ internal class AndroidOFragmentDestroyWatcher(
       fragment: Fragment
     ) {
       if (configProvider().watchFragments) {
-        objectWatcher.watch(fragment)
+        objectWatcher.watch(fragment, "Fragment received Fragment#onDestroy() callback")
       }
     }
   }

diff --git a/leakcanary-object-watcher/src/main/java/leakcanary/KeyedWeakReference.kt b/leakcanary-object-watcher/src/main/java/leakcanary/KeyedWeakReference.kt
@@ -30,7 +30,7 @@ import java.lang.ref.WeakReference
 class KeyedWeakReference(
   referent: Any,
   val key: String,
-  val name: String,
+  val description: String,
   val watchUptimeMillis: Long,
   referenceQueue: ReferenceQueue<Any>
 ) : WeakReference<Any>(

diff --git a/leakcanary-object-watcher/src/main/java/leakcanary/ObjectWatcher.kt b/leakcanary-object-watcher/src/main/java/leakcanary/ObjectWatcher.kt
@@ -112,18 +112,24 @@ class ObjectWatcher constructor(
   /**
    * Identical to [watch] with an empty string reference name.
    */
+  @Deprecated(
+      "Add description parameter explaining why an object is watched to help understand leak traces.",
+      replaceWith = ReplaceWith(
+          "watch(watchedObject, \"Explain why this object should be garbage collected soon\")"
+      )
+  )
   @Synchronized fun watch(watchedObject: Any) {
     watch(watchedObject, "")
   }
 
   /**
    * Watches the provided [watchedObject].
    *
-   * @param name A logical identifier for the watched object.
+   * @param description Describes why the object is watched.
    */
   @Synchronized fun watch(
     watchedObject: Any,
-    name: String
+    description: String
   ) {
     if (!isEnabled()) {
       return
@@ -133,12 +139,12 @@ class ObjectWatcher constructor(
         .toString()
     val watchUptimeMillis = clock.uptimeMillis()
     val reference =
-      KeyedWeakReference(watchedObject, key, name, watchUptimeMillis, queue)
+      KeyedWeakReference(watchedObject, key, description, watchUptimeMillis, queue)
     SharkLog.d {
-        "Watching " +
-            (if (watchedObject is Class<*>) watchedObject.toString() else "instance of ${watchedObject.javaClass.name}") +
-            (if (name.isNotEmpty()) " named $name" else "") +
-            " with key $key"
+      "Watching " +
+          (if (watchedObject is Class<*>) watchedObject.toString() else "instance of ${watchedObject.javaClass.name}") +
+          (if (description.isNotEmpty()) " ($description)" else "") +
+          " with key $key"
     }
 
     watchedObjects[key] = reference

diff --git a/leakcanary-object-watcher/src/test/java/leakcanary/ObjectWatcherTest.kt b/leakcanary-object-watcher/src/test/java/leakcanary/ObjectWatcherTest.kt
@@ -21,14 +21,14 @@ class ObjectWatcherTest {
   var ref: Any? = Any()
 
   @Test fun `unreachable object not retained`() {
-    objectWatcher.watch(ref!!)
+    objectWatcher.watch(ref!!, "unreachable object not retained")
     ref = null
     runGc()
     assertThat(objectWatcher.hasRetainedObjects).isFalse()
   }
 
   @Test fun `reachable object retained`() {
-    objectWatcher.watch(ref!!)
+    objectWatcher.watch(ref!!, "reachable object retained")
     runGc()
     assertThat(objectWatcher.hasRetainedObjects).isTrue()
   }

diff --git a/shark/src/main/java/shark/ObjectInspectors.kt b/shark/src/main/java/shark/ObjectInspectors.kt
@@ -61,11 +61,12 @@ enum class ObjectInspectors : ObjectInspector {
       val objectId = reporter.heapObject.objectId
       references.forEach { ref ->
         if (ref.referent.value == objectId) {
-          reporter.leakingReasons += "ObjectWatcher was watching this"
-          reporter.labels += "key = ${ref.key}"
-          if (ref.name.isNotEmpty()) {
-            reporter.labels += "name = ${ref.name}"
+          reporter.leakingReasons += if (ref.description.isNotEmpty()) {
+            "ObjectWatcher was watching this because ${ref.description}"
+          } else {
+            "ObjectWatcher was watching this"
           }
+          reporter.labels += "key = ${ref.key}"
           if (ref.watchDurationMillis != null) {
             reporter.labels += "watchDurationMillis = ${ref.watchDurationMillis}"
           }

diff --git a/shark/src/main/java/shark/internal/KeyedWeakReferenceMirror.kt b/shark/src/main/java/shark/internal/KeyedWeakReferenceMirror.kt
@@ -8,8 +8,8 @@ internal class KeyedWeakReferenceMirror(
   val referent: ReferenceHolder,
   val key: String,
     // The name field does not exist in pre 1.0 heap dumps.
-  val name: String,
-  // null in pre 2.0 alpha 3 heap dumps
+  val description: String,
+    // null in pre 2.0 alpha 3 heap dumps
   val watchDurationMillis: Long?,
     // null in pre 2.0 alpha 3 heap dumps, -1 if the instance is not retained.
   val retainedDurationMillis: Long?
@@ -25,7 +25,7 @@ internal class KeyedWeakReferenceMirror(
 
     fun fromInstance(
       weakRef: HeapInstance,
-      // Null for pre 2.0 alpha 3 heap dumps
+        // Null for pre 2.0 alpha 3 heap dumps
       heapDumpUptimeMillis: Long?
     ): KeyedWeakReferenceMirror {
 
@@ -37,21 +37,24 @@ internal class KeyedWeakReferenceMirror(
       }
 
       val retainedDurationMillis = if (heapDumpUptimeMillis != null) {
-        val retainedUptimeMillis = weakRef[keyWeakRefClassName, "retainedUptimeMillis"]!!.value.asLong!!
+        val retainedUptimeMillis =
+          weakRef[keyWeakRefClassName, "retainedUptimeMillis"]!!.value.asLong!!
         if (retainedUptimeMillis == -1L) -1L else heapDumpUptimeMillis - retainedUptimeMillis
       } else {
         null
       }
 
       val keyString = weakRef[keyWeakRefClassName, "key"]!!.value.readAsJavaString()!!
 
-      val name = weakRef[keyWeakRefClassName, "name"]?.value?.readAsJavaString() ?: UNKNOWN_LEGACY
+      // Changed from name to description after 2.0
+      val description = (weakRef[keyWeakRefClassName, "description"]
+          ?: weakRef[keyWeakRefClassName, "name"])?.value?.readAsJavaString() ?: UNKNOWN_LEGACY
       return KeyedWeakReferenceMirror(
           watchDurationMillis = watchDurationMillis,
           retainedDurationMillis = retainedDurationMillis,
           referent = weakRef["java.lang.ref.Reference", "referent"]!!.value.holder as ReferenceHolder,
           key = keyString,
-          name = name
+          description = description
       )
     }
   }

diff --git a/shark/src/test/java/shark/HprofWriterHelper.kt b/shark/src/test/java/shark/HprofWriterHelper.kt
@@ -164,7 +164,7 @@ class HprofWriterHelper constructor(
         classId = keyedWeakReferenceClassId,
         fields = listOf(
             referenceKey,
-            string(""),
+            string("its lifecycle has ended"),
             LongHolder(5000),
             LongHolder(20000),
             ReferenceHolder(referentInstanceId.value)

diff --git a/shark/src/test/java/shark/LeakStatusTest.kt b/shark/src/test/java/shark/LeakStatusTest.kt
@@ -227,7 +227,7 @@ class LeakStatusTest {
     val leak = analysis.applicationLeaks[0]
     assertThat(leak.leakTrace.elements.last().leakStatus).isEqualTo(NOT_LEAKING)
     assertThat(leak.leakTrace.elements.last().leakStatusReason).isEqualTo(
-        "Leaking is not leaking. Conflicts with ObjectWatcher was watching this"
+        "Leaking is not leaking. Conflicts with ObjectWatcher was watching this because its lifecycle has ended"
     )
   }
 
@@ -241,7 +241,7 @@ class LeakStatusTest {
     val leak = analysis.applicationLeaks[0]
     assertThat(leak.leakTrace.elements.last().leakStatus).isEqualTo(LEAKING)
     assertThat(leak.leakTrace.elements.last().leakStatusReason).isEqualTo(
-        "Leaking is leaking and ObjectWatcher was watching this"
+        "Leaking is leaking and ObjectWatcher was watching this because its lifecycle has ended"
     )
   }
 

diff --git a/shark/src/test/java/shark/LeakTraceStringRenderingTest.kt b/shark/src/test/java/shark/LeakTraceStringRenderingTest.kt
@@ -36,7 +36,7 @@ class LeakTraceStringRenderingTest {
     │    ↓ static GcRoot.leak
     │                    ~~~~
     ╰→ Leaking
-         Leaking: YES (ObjectWatcher was watching this)
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
          key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
          watchDurationMillis = 25000
          retainedDurationMillis = 10000
@@ -69,7 +69,7 @@ class LeakTraceStringRenderingTest {
     │    ↓ static GcRoot.leak
     │                    ~~~~
     ╰→ Leaking
-         Leaking: YES (ObjectWatcher was watching this)
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
          key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
          watchDurationMillis = 25000
          retainedDurationMillis = 10000
@@ -143,7 +143,7 @@ class LeakTraceStringRenderingTest {
     │    ↓ static GcRoot.leak
     │                    ~~~~
     ╰→ Leaking
-         Leaking: YES (ObjectWatcher was watching this)
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
          ¯\_(ツ)_/¯
          key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
          watchDurationMillis = 25000
@@ -179,7 +179,7 @@ class LeakTraceStringRenderingTest {
     │    ↓ ClassA.leak
     │             ~~~~
     ╰→ Leaking
-         Leaking: YES (ObjectWatcher was watching this)
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
          key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
          watchDurationMillis = 25000
          retainedDurationMillis = 10000
@@ -208,7 +208,7 @@ class LeakTraceStringRenderingTest {
     │    ↓ array Object[].[0]
     │                     ~~~
     ╰→ Leaking
-         Leaking: YES (ObjectWatcher was watching this)
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
          key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
          watchDurationMillis = 25000
          retainedDurationMillis = 10000
@@ -229,7 +229,7 @@ class LeakTraceStringRenderingTest {
     │    ↓ thread MyThread.<Java Local>
     │                      ~~~~~~~~~~~~
     ╰→ Leaking
-         Leaking: YES (ObjectWatcher was watching this)
+         Leaking: YES (ObjectWatcher was watching this because its lifecycle has ended)
          key = 39efcc1a-67bf-2040-e7ab-3fc9f94731dc
          watchDurationMillis = 25000
          retainedDurationMillis = 10000