Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

View.backPressHandler memory leak fix. #894

Merged
merged 2 commits into from
Oct 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
<activity
android:name="com.squareup.workflow1.ui.container.fixtures.BackStackContainerLifecycleActivity"
android:theme="@style/Theme.AppCompat.NoActionBar"/>
<activity android:name="androidx.activity.ComponentActivity"/>
<activity android:name="androidx.activity.ComponentActivity"/>
</application>
</manifest>
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package com.squareup.workflow1.ui

import android.view.View
import android.view.ViewGroup
import androidx.activity.ComponentActivity
import androidx.activity.OnBackPressedCallback
import androidx.activity.OnBackPressedDispatcherSpy
import androidx.lifecycle.Lifecycle.State.DESTROYED
import androidx.lifecycle.Lifecycle.State.RESUMED
import androidx.lifecycle.LifecycleRegistry
import androidx.lifecycle.ViewTreeLifecycleOwner
import androidx.test.ext.junit.rules.ActivityScenarioRule
import com.google.common.truth.Truth.assertThat
import com.squareup.workflow1.ui.internal.test.DetectLeaksAfterTestSuccess
import com.squareup.workflow1.ui.internal.test.IdlingDispatcherRule
import org.junit.Rule
import org.junit.Test
import org.junit.rules.RuleChain

@OptIn(WorkflowUiExperimentalApi::class)
internal class BackPressedHandlerTest {
private val scenarioRule = ActivityScenarioRule(ComponentActivity::class.java)
private val scenario get() = scenarioRule.scenario

@get:Rule val rules: RuleChain = RuleChain.outerRule(DetectLeaksAfterTestSuccess())
.around(scenarioRule)
.around(IdlingDispatcherRule)

private var viewHandlerCount = 0
private val viewBackHandler: BackPressHandler = {
viewHandlerCount++
}

@Test fun itWorksWhenHandlerIsAddedBeforeAttach() {
scenario.onActivity { activity ->
val view = View(activity)
view.backPressedHandler = viewBackHandler

activity.setContentView(view)
assertThat(viewHandlerCount).isEqualTo(0)

activity.onBackPressed()
assertThat(viewHandlerCount).isEqualTo(1)
}
}

@Test fun itWorksWhenHandlerIsAddedAfterAttach() {
rjrjr marked this conversation as resolved.
Show resolved Hide resolved
scenario.onActivity { activity ->
val view = View(activity)
activity.setContentView(view)

view.backPressedHandler = viewBackHandler
assertThat(viewHandlerCount).isEqualTo(0)

activity.onBackPressed()
assertThat(viewHandlerCount).isEqualTo(1)
}
}

@Test fun onlyActiveWhileViewIsAttached() {
var fallbackCallCount = 0
val defaultBackHandler = object : OnBackPressedCallback(true) {
override fun handleOnBackPressed() {
fallbackCallCount++
}
}

scenario.onActivity { activity ->
activity.onBackPressedDispatcher.addCallback(defaultBackHandler)

val view = View(activity)
view.backPressedHandler = viewBackHandler

activity.onBackPressed()
assertThat(fallbackCallCount).isEqualTo(1)
assertThat(viewHandlerCount).isEqualTo(0)

activity.setContentView(view)
activity.onBackPressed()
assertThat(fallbackCallCount).isEqualTo(1)
assertThat(viewHandlerCount).isEqualTo(1)

(view.parent as ViewGroup).removeView(view)
activity.onBackPressed()
assertThat(fallbackCallCount).isEqualTo(2)
assertThat(viewHandlerCount).isEqualTo(1)

activity.setContentView(view)
activity.onBackPressed()
assertThat(fallbackCallCount).isEqualTo(2)
assertThat(viewHandlerCount).isEqualTo(2)
}
}

@Test fun callbackIsRemoved() {
scenario.onActivity { activity ->
val spy = OnBackPressedDispatcherSpy(activity.onBackPressedDispatcher)
assertThat(spy.callbacks()).isEmpty()

val lifecycle = LifecycleRegistry(activity)
lifecycle.currentState = RESUMED

val view = View(activity)
view.backPressedHandler = viewBackHandler
assertThat(spy.callbacks()).hasSize(1)

ViewTreeLifecycleOwner.set(view) { lifecycle }
activity.setContentView(view)

(view.parent as ViewGroup).removeView(view)
assertThat(spy.callbacks()).hasSize(1)

lifecycle.currentState = DESTROYED
assertThat(spy.callbacks()).isEmpty()
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package androidx.activity;

import java.util.ArrayDeque;

public class OnBackPressedDispatcherSpy {
private final OnBackPressedDispatcher dispatcher;

public OnBackPressedDispatcherSpy(OnBackPressedDispatcher dispatcher) {
this.dispatcher = dispatcher;
}

public ArrayDeque<OnBackPressedCallback> callbacks() {
return dispatcher.mOnBackPressedCallbacks;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Collection or List maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exposing the implementation of an Android class for testing purposes, I don't think there's a real need to abstract it.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import android.view.View.OnAttachStateChangeListener
import androidx.activity.OnBackPressedCallback
import androidx.activity.OnBackPressedDispatcherOwner
import androidx.lifecycle.DefaultLifecycleObserver
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.ViewTreeLifecycleOwner

Expand All @@ -21,79 +22,123 @@ public typealias BackPressHandler = () -> Unit
* A function to be called if the device back button is pressed while this
* view is attached to a window.
*
* Implemented via [OnBackPressedDispatcher][androidx.activity.OnBackPressedDispatcher],
* making this a last-registered-first-served mechanism.
* Implemented via [OnBackPressedDispatcher][androidx.activity.OnBackPressedDispatcher].
* That means that this is a last-registered-first-served mechanism, and that it is
* compatible with Compose back button handling.
*/
@WorkflowUiExperimentalApi
public var View.backPressedHandler: BackPressHandler?
get() = handlerWrapperOrNull?.handler
get() = observerOrNull?.handler
set(value) {
handlerWrapperOrNull?.stop()
observerOrNull?.stop()

val wrapper = value?.let {
HandleBackPressWhenAttached(this, it).apply { start() }
observerOrNull = value?.let {
AttachStateAndLifecycleObserver(this, it).apply { start() }
}
setTag(R.id.view_back_handler, wrapper)
}

@WorkflowUiExperimentalApi
private val View.handlerWrapperOrNull
get() = getTag(R.id.view_back_handler) as HandleBackPressWhenAttached?
private var View.observerOrNull: AttachStateAndLifecycleObserver?
get() = getTag(R.id.view_back_handler) as AttachStateAndLifecycleObserver?
set(value) {
setTag(R.id.view_back_handler, value)
}

/**
* Uses the [androidx.activity.OnBackPressedDispatcher] to associate a [BackPressHandler]
* with a [View].
* This is more complicated than one would hope because [Lifecycle] and memory leaks.
*
* - We need to claim our spot in the
* [OnBackPressedDispatcher][androidx.activity.OnBackPressedDispatcher] immediately,
* to ensure our [onBackPressedCallback] shadows upstream ones, and can be shadowed
* appropriately itself
* - The whole point of this mechanism is to be active only while the view is active
* - That's what [ViewTreeLifecycleOwner] is for, but we can't really find that until
* we're attached -- which often does not happen in the order required for registering
* with the dispatcher
*
* So, our [start] is called immediately, to get [onBackPressedCallback] listed at the right
* spot in the dispatcher's stack. But the [onBackPressedCallback]'s enabled / disabled state
* is tied to whether the [view] is attached or not.
*
* - Registers [handler] on [start]
* - Enables and disables it as [view] is attached to or detached from its window
* - De-registers it on [stop], or when its [lifecycle][ViewTreeLifecycleOwner] is destroyed
* Also note that we expect to find a [ViewTreeLifecycleOwner] at attach time,
* so that we can know when it's time to remove the [onBackPressedCallback] from
* the dispatch stack
* ([no memory leaks please](https://github.com/square/workflow-kotlin/issues/889)).
*
* Why is it okay to wait for the [ViewTreeLifecycleOwner] to be destroyed before we
* remove [onBackPressedCallback] from the dispatcher? In normal apps that's
* the `Activity` or a `Fragment`, which will live a very long time, but Workflow UI
* is more controlling than that. `WorkflowViewStub` and the rest of the stock container
* classes use `WorkflowLifecycleOwner` to provide a short lived [ViewTreeLifecycleOwner]
* for each [View] they create, and tear it down before moving to the next one.
*
* None the less, as a belt-and-suspenders guard against leaking,
* we also take care to null out the pointer from the [onBackPressedCallback] to the
* actual [handler] while the [view] is detached. We can't be confident that the
* [ViewTreeLifecycleOwner] we find will be a well behaved one that was put in place
* by `WorkflowLifecycleOwner`. Who knows what adventures our clients will get up to.
*/
@WorkflowUiExperimentalApi
private class HandleBackPressWhenAttached(
private class AttachStateAndLifecycleObserver(
private val view: View,
val handler: BackPressHandler
) : OnAttachStateChangeListener, DefaultLifecycleObserver {
private val onBackPressedCallback = object : OnBackPressedCallback(false) {
override fun handleOnBackPressed() {
handler.invoke()
}
}
private val onBackPressedCallback = NullableOnBackPressedCallback()
private var lifecycleOrNull: Lifecycle? = null

fun start() {
view.context.onBackPressedDispatcherOwnerOrNull()
?.let { owner ->
owner.onBackPressedDispatcher.addCallback(owner, onBackPressedCallback)
view.addOnAttachStateChangeListener(this)
if (view.isAttachedToWindow) onViewAttachedToWindow(view)

// We enable the handler only while its view is attached to a window.
// This ensures that a temporarily removed view (e.g. for caching)
// does not participate in back button handling.
ViewTreeLifecycleOwner.get(view)?.lifecycle?.addObserver(this)
}
}

fun stop() {
onBackPressedCallback.remove()
view.removeOnAttachStateChangeListener(this)
ViewTreeLifecycleOwner.get(view)?.lifecycle?.removeObserver(this)
lifecycleOrNull?.removeObserver(this)
}

override fun onViewAttachedToWindow(attachedView: View) {
require(view === attachedView)
onBackPressedCallback.isEnabled = true
lifecycleOrNull?.let { lifecycle ->
lifecycle.removeObserver(this)
lifecycleOrNull = null
}
Comment on lines +106 to +109

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

lifecycleOrNull?.removeObserver(this)
lifecycleOrNull = null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact you can do

lifecycleOrNull?.removeObserver(this)
lifecycleOrNull = ViewTreeLifecycleOwner.get(view)?.lifecycle

ViewTreeLifecycleOwner.get(view)?.lifecycle?.let { lifecycle ->
lifecycleOrNull = lifecycle
onBackPressedCallback.handlerOrNull = handler
onBackPressedCallback.isEnabled = true
lifecycle.addObserver(this)
}
?: error(
"Expected to find a ViewTreeLifecycleOwner to manage the " +
"backPressedHandler ($handler) for $view"
)
}

override fun onViewDetachedFromWindow(detachedView: View) {
require(view === detachedView)
onBackPressedCallback.isEnabled = false
onBackPressedCallback.handlerOrNull = null
}

override fun onDestroy(owner: LifecycleOwner) {
stop()
}
}

@WorkflowUiExperimentalApi
internal class NullableOnBackPressedCallback : OnBackPressedCallback(false) {
var handlerOrNull: BackPressHandler? = null

override fun handleOnBackPressed() {
handlerOrNull?.invoke()
}
}

@WorkflowUiExperimentalApi
public tailrec fun Context.onBackPressedDispatcherOwnerOrNull(): OnBackPressedDispatcherOwner? =
when (this) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,6 @@ internal class RealWorkflowLifecycleOwner(
OnAttachStateChangeListener,
LifecycleEventObserver {

/**
* Weak reference ensures that we don't leak the view.
*/
Comment on lines -115 to -117

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL

private var view: View? = null

private val localLifecycle =
Expand Down