Skip to content

Commit

Permalink
Clears Fragment reference in FragmentContextWrapper after Fragment#on…
Browse files Browse the repository at this point in the history
…Destroy()

This CL uses the fragment Lifecycle to clear the fragment instance from the FragmentContextWrapper after onDestroy.

Fixes: #2070

See #2070

RELNOTES=Fixes #2070: Clears the Fragment reference in FragmentContextWrapper after Fragment#onDestroy() to prevent leaks if a non-Hilt View outlives the Hilt Fragment that created it.
PiperOrigin-RevId: 359775904
  • Loading branch information
bcorso authored and Dagger Team committed Feb 26, 2021
1 parent addb1fc commit 7f4c3a2
Show file tree
Hide file tree
Showing 16 changed files with 171 additions and 5 deletions.
1 change: 1 addition & 0 deletions java/dagger/android/support/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ android_library(
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_appcompat_appcompat",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
4 changes: 4 additions & 0 deletions java/dagger/hilt/android/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ android_library(
"@maven//:androidx_activity_activity",
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down Expand Up @@ -82,6 +83,7 @@ android_library(
"@maven//:androidx_activity_activity",
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand All @@ -100,6 +102,7 @@ android_library(
"@google_bazel_common//third_party/java/jsr305_annotations",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down Expand Up @@ -196,6 +199,7 @@ gen_maven_artifact(
"androidx.activity:activity",
"androidx.annotation:annotation",
"androidx.fragment:fragment",
"androidx.lifecycle:lifecycle-common",
"androidx.lifecycle:lifecycle-viewmodel",
"androidx.lifecycle:lifecycle-viewmodel-savedstate",
"androidx.savedstate:savedstate",
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/android/internal/builders/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ android_library(
"//java/dagger/hilt/android/components:view_model_component",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/android/internal/lifecycle/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ android_library(
"@maven//:androidx_activity_activity",
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:androidx_savedstate_savedstate",
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/android/internal/managers/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ android_library(
"@maven//:androidx_activity_activity",
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package dagger.hilt.android.internal.managers;

import androidx.lifecycle.Lifecycle;
import androidx.lifecycle.LifecycleEventObserver;
import androidx.lifecycle.LifecycleOwner;
import android.content.Context;
import android.content.ContextWrapper;
import androidx.fragment.app.Fragment;
Expand Down Expand Up @@ -104,7 +107,7 @@ private GeneratedComponentManager<?> getParentComponentManager(boolean allowMiss
if (context instanceof FragmentContextWrapper) {

FragmentContextWrapper fragmentContextWrapper = (FragmentContextWrapper) context;
return (GeneratedComponentManager<?>) fragmentContextWrapper.fragment;
return (GeneratedComponentManager<?>) fragmentContextWrapper.getFragment();
} else if (allowMissing) {
// We didn't find anything, so return null if we're not supposed to fail.
// The rest of the logic is just about getting a good error message.
Expand Down Expand Up @@ -167,20 +170,38 @@ private static Context unwrap(Context context, Class<?> target) {
*/
// This is only non-final for the account override
public static final class FragmentContextWrapper extends ContextWrapper {
private Fragment fragment;
private LayoutInflater baseInflater;
private LayoutInflater inflater;
public final Fragment fragment;

public FragmentContextWrapper(Context base, Fragment fragment) {
private final LifecycleEventObserver fragmentLifecycleObserver =
new LifecycleEventObserver() {
@Override
public void onStateChanged(LifecycleOwner source, Lifecycle.Event event) {
if (event == Lifecycle.Event.ON_DESTROY) {
// Prevent the fragment from leaking if the view outlives the fragment.
// See https://github.com/google/dagger/issues/2070
FragmentContextWrapper.this.fragment = null;
}
}
};

FragmentContextWrapper(Context base, Fragment fragment) {
super(Preconditions.checkNotNull(base));
this.baseInflater = null;
this.fragment = Preconditions.checkNotNull(fragment);
this.fragment.getLifecycle().addObserver(fragmentLifecycleObserver);
}

public FragmentContextWrapper(LayoutInflater baseInflater, Fragment fragment) {
FragmentContextWrapper(LayoutInflater baseInflater, Fragment fragment) {
super(Preconditions.checkNotNull(Preconditions.checkNotNull(baseInflater).getContext()));
this.baseInflater = baseInflater;
this.fragment = Preconditions.checkNotNull(fragment);
this.fragment.getLifecycle().addObserver(fragmentLifecycleObserver);
}

Fragment getFragment() {
Preconditions.checkNotNull(fragment, "The fragment has already been destroyed.");
return fragment;
}

@Override
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/android/internal/modules/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ android_library(
"@maven//:androidx_activity_activity",
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/android/migration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ android_library(
"@maven//:androidx_activity_activity",
"@maven//:androidx_annotation_annotation",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
1 change: 1 addition & 0 deletions java/dagger/hilt/android/testing/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ gen_maven_artifact(
"androidx.activity:activity",
"androidx.annotation:annotation",
"androidx.fragment:fragment",
"androidx.lifecycle:lifecycle-common",
"androidx.lifecycle:lifecycle-viewmodel",
"androidx.lifecycle:lifecycle-viewmodel-savedstate",
"androidx.multidex:multidex",
Expand Down
1 change: 1 addition & 0 deletions javatests/dagger/android/processor/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ GenJavaTests(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
1 change: 1 addition & 0 deletions javatests/dagger/android/support/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ GenRobolectricTests(
"@maven//:androidx_activity_activity",
"@maven//:androidx_appcompat_appcompat",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
],
Expand Down
2 changes: 2 additions & 0 deletions javatests/dagger/android/support/functional/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ android_library(
deps = [
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:androidx_appcompat_appcompat",
Expand All @@ -57,6 +58,7 @@ GenRobolectricTests(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:androidx_test_core",
Expand Down
6 changes: 6 additions & 0 deletions javatests/dagger/hilt/android/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ android_local_test(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:junit_junit",
Expand All @@ -322,6 +323,7 @@ android_local_test(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:junit_junit",
Expand Down Expand Up @@ -358,6 +360,7 @@ android_local_test(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:junit_junit",
Expand All @@ -383,6 +386,7 @@ android_local_test(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:junit_junit",
Expand Down Expand Up @@ -410,6 +414,7 @@ android_local_test(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:junit_junit",
Expand All @@ -436,6 +441,7 @@ android_local_test(
"@google_bazel_common//third_party/java/truth",
"@maven//:androidx_activity_activity",
"@maven//:androidx_fragment_fragment",
"@maven//:androidx_lifecycle_lifecycle_common",
"@maven//:androidx_lifecycle_lifecycle_viewmodel",
"@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
"@maven//:junit_junit",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="dagger.hilt.android.internal.managers">

<uses-sdk android:minSdkVersion="14" />

<application>
<activity
android:name=".FragmentContextWrapperLeakTest$TestActivity"
android:exported="false"/>
</application>
</manifest>
39 changes: 39 additions & 0 deletions javatests/dagger/hilt/android/internal/managers/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Copyright (C) 2021 The Dagger Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# Description:
# Tests for internal code for implementing Hilt processors.

package(default_visibility = ["//:src"])

android_local_test(
name = "FragmentContextWrapperLeakTest",
size = "small",
srcs = ["FragmentContextWrapperLeakTest.java"],
manifest = "AndroidManifest.xml",
manifest_values = {
"minSdkVersion": "14",
},
deps = [
"//:android_local_test_exports",
"//:dagger_with_compiler",
"@google_bazel_common//third_party/java/jsr330_inject",
"@maven//:junit_junit",
"@google_bazel_common//third_party/java/truth",
"//java/dagger/hilt:entry_point",
"//java/dagger/hilt:install_in",
"//java/dagger/hilt/android:android_entry_point",
"//java/dagger/hilt/android/lifecycle",
"//java/dagger/hilt/android/testing:hilt_android_test",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* Copyright (C) 2021 The Dagger Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package dagger.hilt.android.internal.managers;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import androidx.lifecycle.Lifecycle;
import android.os.Build;
import androidx.fragment.app.Fragment;
import androidx.fragment.app.FragmentActivity;
import androidx.test.core.app.ActivityScenario;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import dagger.hilt.android.AndroidEntryPoint;
import dagger.hilt.android.internal.managers.ViewComponentManager.FragmentContextWrapper;
import dagger.hilt.android.testing.HiltAndroidRule;
import dagger.hilt.android.testing.HiltAndroidTest;
import dagger.hilt.android.testing.HiltTestApplication;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.annotation.Config;

@HiltAndroidTest
@RunWith(AndroidJUnit4.class)
// Robolectric requires Java9 to run API 29 and above, so use API 28 instead
@Config(sdk = Build.VERSION_CODES.P, application = HiltTestApplication.class)
public final class FragmentContextWrapperLeakTest {
/** An activity to test injection. */
@AndroidEntryPoint(FragmentActivity.class)
public static final class TestActivity extends Hilt_FragmentContextWrapperLeakTest_TestActivity {}

/** A fragment to test injection. */
@AndroidEntryPoint(Fragment.class)
public static final class TestFragment extends Hilt_FragmentContextWrapperLeakTest_TestFragment {}

@Rule public HiltAndroidRule hiltRule = new HiltAndroidRule(this);

@Test
public void testFragmentContextWrapperDoesNotLeakFragment() {
try (ActivityScenario<TestActivity> scenario = ActivityScenario.launch(TestActivity.class)) {
TestFragment fragment = new TestFragment();
scenario.onActivity(
activity ->
activity
.getSupportFragmentManager()
.beginTransaction()
.add(fragment, "TestFragment")
.commitNow());

FragmentContextWrapper fragmentContextWrapper =
(FragmentContextWrapper) fragment.getContext();
assertThat(fragmentContextWrapper.getFragment()).isEqualTo(fragment);
scenario.moveToState(Lifecycle.State.DESTROYED);
NullPointerException exception =
assertThrows(NullPointerException.class, fragmentContextWrapper::getFragment);
assertThat(exception).hasMessageThat().contains("The fragment has already been destroyed");
}
}
}

0 comments on commit 7f4c3a2

Please sign in to comment.