Keep a weak reference to LifecycleOwner in observers

WeakListeners in data binding are designed to hold on a strong
reference to the observed object but weak reference to the binding
it self so that we don't leak the binding.

Unfortunately, live data observers were keeping a strong reference
to the lifecycle owner, which meant that if binding is GC'ed and
we don't get a chance to process weak references, we might leak the
lifecycleowner.

Ideally, we wouldn't want to use this ReferenceQueue that cleans up
weak listeners but aside from that queue, we don't have a good
notification (on the main thread) that the binding can never be used
anymore so I decided to keep it for now.

Bug: 176886060
Test: existing tests, also adding tests that clear lifecycle
Change-Id: Ia72639571cf6aef2769de20383ee400ea20f67ba

2 files changed, 49 insertions, 16 deletions

diff --git a/extensions/databindingKtx/src/main/java/androidx/databinding/ViewDataBindingKtx.kt b/extensions/databindingKtx/src/main/java/androidx/databinding/ViewDataBindingKtx.kt
index 509b44ab..53e7b494 100644
--- a/extensions/databindingKtx/src/main/java/androidx/databinding/ViewDataBindingKtx.kt
+++ b/extensions/databindingKtx/src/main/java/androidx/databinding/ViewDataBindingKtx.kt
@@ -23,6 +23,7 @@ import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.collect
 import java.lang.ref.ReferenceQueue
+import java.lang.ref.WeakReference
 
 /**
  * Helper methods for data binding Ktx features.
@@ -62,8 +63,9 @@ object ViewDataBindingKtx {
             localFieldId: Int,
             referenceQueue: ReferenceQueue<ViewDataBinding>
     ) : ObservableReference<Flow<Any?>> {
-
-        private var _lifecycleOwner : LifecycleOwner? = null
+        // keep this weak so that we don't end up leaking the lifecycle owner if the
+        // binding is GC'ed. (see: b/176886060)
+        private var _lifecycleOwnerRef : WeakReference<LifecycleOwner>? = null
         private var observerJob : Job? = null
         private val listener = WeakListener<Flow<Any?>>(
                 binder, localFieldId, this, referenceQueue
@@ -73,7 +75,7 @@ object ViewDataBindingKtx {
         }
 
         override fun addListener(target: Flow<Any?>?) {
-            val owner = _lifecycleOwner ?: return
+            val owner = _lifecycleOwnerRef?.get() ?: return
             if (target != null) {
                 startCollection(owner, target)
             }
@@ -94,13 +96,17 @@ object ViewDataBindingKtx {
         }
 
         override fun setLifecycleOwner(lifecycleOwner: LifecycleOwner?) {
-            if (_lifecycleOwner === lifecycleOwner) {
+            if (_lifecycleOwnerRef?.get() === lifecycleOwner) {
                 return
             }
             observerJob?.cancel()
-            _lifecycleOwner = lifecycleOwner
+            if (lifecycleOwner == null) {
+                _lifecycleOwnerRef = null
+                return
+            }
+            _lifecycleOwnerRef = WeakReference(lifecycleOwner)
             val target = listener.target
-            if (lifecycleOwner != null && target != null) {
+            if (target != null) {
                 startCollection(lifecycleOwner, target)
             }
         }
diff --git a/extensions/library/src/main/java/androidx/databinding/ViewDataBinding.java b/extensions/library/src/main/java/androidx/databinding/ViewDataBinding.java
index 9612bd65..f9c4e2fe 100644
--- a/extensions/library/src/main/java/androidx/databinding/ViewDataBinding.java
+++ b/extensions/library/src/main/java/androidx/databinding/ViewDataBinding.java
@@ -18,7 +18,9 @@ package androidx.databinding;
 
 import android.annotation.TargetApi;
 
+import android.util.Log;
 import androidx.annotation.RestrictTo;
+import androidx.fragment.app.Fragment;
 import androidx.lifecycle.Lifecycle;
 import androidx.lifecycle.LifecycleObserver;
 import androidx.lifecycle.LifecycleOwner;
@@ -203,7 +205,6 @@ public abstract class ViewDataBinding extends BaseObservable implements ViewBind
                 mPendingRebind = false;
             }
             processReferenceQueue();
-
             if (VERSION.SDK_INT >= VERSION_CODES.KITKAT) {
                 // Nested so that we don't get a lint warning in IntelliJ
                 if (!mRoot.isAttachedToWindow()) {
@@ -404,11 +405,21 @@ public abstract class ViewDataBinding extends BaseObservable implements ViewBind
      * and no LifecycleOwner is set, the LiveData will not be observed and updates to it
      * will not be propagated to the UI.
      *
+     * When using Data Binding with Fragments, make sure to use Fragment.getViewLifecycleOwner().
+     * Using the Fragment as the LifecycleOwner might cause memory leaks since the Fragment's
+     * Lifecycle outlives the view Lifecycle.
+     * When using Data Binding with Activities, you can use the Activity as the LifecycleOwner.
+     *
      * @param lifecycleOwner The LifecycleOwner that should be used for observing changes of
      *                       LiveData in this binding.
      */
     @MainThread
     public void setLifecycleOwner(@Nullable LifecycleOwner lifecycleOwner) {
+        if (lifecycleOwner instanceof Fragment) {
+            Log.w("DataBinding", "Setting the fragment as the LifecycleOwner might cause"
+                    + " memory leaks because views lives shorter than the Fragment. Consider"
+                    + " using Fragment's view lifecycle");
+        }
         if (mLifecycleOwner == lifecycleOwner) {
             return;
         }
@@ -1563,7 +1574,10 @@ public abstract class ViewDataBinding extends BaseObservable implements ViewBind
     private static class LiveDataListener implements Observer,
             ObservableReference<LiveData<?>> {
         final WeakListener<LiveData<?>> mListener;
-        LifecycleOwner mLifecycleOwner;
+        // keep this weak because listeners might leak, we don't want to leak the owner
+        // see: b/176886060
+        @Nullable
+        WeakReference<LifecycleOwner> mLifecycleOwnerRef = null;
 
         public LiveDataListener(
                 ViewDataBinding binder,
@@ -1573,19 +1587,31 @@ public abstract class ViewDataBinding extends BaseObservable implements ViewBind
             mListener = new WeakListener(binder, localFieldId, this, referenceQueue);
         }
 
+        @Nullable
+        private LifecycleOwner getLifecycleOwner() {
+            WeakReference<LifecycleOwner> ownerRef = this.mLifecycleOwnerRef;
+            if (ownerRef == null) {
+                return null;
+            }
+            return ownerRef.get();
+        }
+
         @Override
-        public void setLifecycleOwner(LifecycleOwner lifecycleOwner) {
-            LifecycleOwner owner = (LifecycleOwner) lifecycleOwner;
+        public void setLifecycleOwner(@Nullable LifecycleOwner lifecycleOwner) {
+            LifecycleOwner previousOwner = getLifecycleOwner();
+            LifecycleOwner newOwner = lifecycleOwner;
             LiveData<?> liveData = mListener.getTarget();
             if (liveData != null) {
-                if (mLifecycleOwner != null) {
+                if (previousOwner != null) {
                     liveData.removeObserver(this);
                 }
-                if (lifecycleOwner != null) {
-                    liveData.observe(owner, this);
+                if (newOwner != null) {
+                    liveData.observe(newOwner, this);
                 }
             }
-            mLifecycleOwner = owner;
+            if (newOwner != null) {
+                mLifecycleOwnerRef = new WeakReference<LifecycleOwner>(newOwner);
+            }
         }
 
         @Override
@@ -1595,8 +1621,9 @@ public abstract class ViewDataBinding extends BaseObservable implements ViewBind
 
         @Override
         public void addListener(LiveData<?> target) {
-            if (mLifecycleOwner != null) {
-                target.observe(mLifecycleOwner, this);
+            LifecycleOwner lifecycleOwner = getLifecycleOwner();
+            if (lifecycleOwner != null) {
+                target.observe(lifecycleOwner, this);
             }
         }