From c3f613f4c44c23352dc1485e68ab64d0a65dfa6c Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 20 Sep 2021 12:38:05 -0700 Subject: [PATCH] Add a runtime flag to control the Fragment.getContext() fix. This runtime flag is used only if the compiler flag is also on. RELNOTES=Add a runtime flag for the Fragment.getContext() fix. PiperOrigin-RevId: 397816397 --- java/dagger/hilt/BUILD | 1 + java/dagger/hilt/android/BUILD | 2 + java/dagger/hilt/android/flags/BUILD | 40 +++++++ .../android/flags/FragmentGetContextFix.java | 103 ++++++++++++++++++ .../hilt/android/flags/package-info.java | 27 +++++ .../hilt/android/plugin/HiltExtension.kt | 2 +- .../processor/internal/AndroidClassNames.java | 3 + .../androidentrypoint/FragmentGenerator.java | 40 +++++-- .../AggregatedDepsProcessor.java | 2 +- javatests/dagger/hilt/android/BUILD | 4 + .../android/FragmentContextOnAttachTest.java | 23 +++- 11 files changed, 234 insertions(+), 13 deletions(-) create mode 100644 java/dagger/hilt/android/flags/BUILD create mode 100644 java/dagger/hilt/android/flags/FragmentGetContextFix.java create mode 100644 java/dagger/hilt/android/flags/package-info.java diff --git a/java/dagger/hilt/BUILD b/java/dagger/hilt/BUILD index e7e8ed7dd34..8a54aa2bd11 100644 --- a/java/dagger/hilt/BUILD +++ b/java/dagger/hilt/BUILD @@ -135,6 +135,7 @@ filegroup( srcs = [ "//java/dagger/hilt/android:srcs_filegroup", "//java/dagger/hilt/android/components:srcs_filegroup", + "//java/dagger/hilt/android/flags:srcs_filegroup", "//java/dagger/hilt/android/internal:srcs_filegroup", "//java/dagger/hilt/android/internal/builders:srcs_filegroup", "//java/dagger/hilt/android/internal/lifecycle:srcs_filegroup", diff --git a/java/dagger/hilt/android/BUILD b/java/dagger/hilt/android/BUILD index 4bce7b60d49..0e46c66b07c 100644 --- a/java/dagger/hilt/android/BUILD +++ b/java/dagger/hilt/android/BUILD @@ -31,6 +31,7 @@ android_library( exports = [ "//java/dagger/hilt:install_in", "//java/dagger/hilt/android/components", + "//java/dagger/hilt/android/flags:fragment_get_context_fix", "//java/dagger/hilt/android/internal", "//java/dagger/hilt/android/internal/builders", "//java/dagger/hilt/android/internal/managers", @@ -184,6 +185,7 @@ gen_maven_artifact( "//java/dagger/hilt/android:package_info", "//java/dagger/hilt/android/components", "//java/dagger/hilt/android/components:package_info", + "//java/dagger/hilt/android/flags:fragment_get_context_fix", "//java/dagger/hilt/android/internal", "//java/dagger/hilt/android/internal/builders", "//java/dagger/hilt/android/internal/earlyentrypoint", diff --git a/java/dagger/hilt/android/flags/BUILD b/java/dagger/hilt/android/flags/BUILD new file mode 100644 index 00000000000..7bd9010a326 --- /dev/null +++ b/java/dagger/hilt/android/flags/BUILD @@ -0,0 +1,40 @@ +# 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: +# Runtime flags to control Hilt behavior for rollout of changes. These flags are usually +# meant to be temporary and so defaults may change with releases and then these flags +# may eventually be removed, just like compiler options with similar purposes. + +package(default_visibility = ["//:src"]) + +android_library( + name = "fragment_get_context_fix", + srcs = [ + "FragmentGetContextFix.java", + ], + deps = [ + "//:dagger_with_compiler", + "//java/dagger/hilt:entry_point", + "//java/dagger/hilt:install_in", + "//java/dagger/hilt/android:entry_point_accessors", + "//java/dagger/hilt/android/components", + "//java/dagger/hilt/internal:preconditions", + ], +) + +filegroup( + name = "srcs_filegroup", + srcs = glob(["*"]), +) diff --git a/java/dagger/hilt/android/flags/FragmentGetContextFix.java b/java/dagger/hilt/android/flags/FragmentGetContextFix.java new file mode 100644 index 00000000000..b867ec89d70 --- /dev/null +++ b/java/dagger/hilt/android/flags/FragmentGetContextFix.java @@ -0,0 +1,103 @@ +/* + * 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.flags; + +import android.content.Context; +import dagger.Module; +import dagger.hilt.EntryPoint; +import dagger.hilt.InstallIn; +import dagger.hilt.android.EntryPointAccessors; +import dagger.hilt.components.SingletonComponent; +import dagger.hilt.internal.Preconditions; +import dagger.multibindings.Multibinds; +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; +import java.util.Set; +import javax.inject.Qualifier; + +/** + * Runtime flag for the Fragment.getContext() fix. See https://github.com/google/dagger/pull/2620 + * for this change. Controls if fragment code should use the fixed getContext() behavior where it + * correctly returns null after a fragment is removed. This fixed behavior matches the behavior of a + * regular, non-Hilt fragment and can help catch issues where a removed or leaked fragment is + * incorrectly used. + * + *

This flag is paired with the compiler option flag + * dagger.hilt.android.useFragmentGetContextFix. When that flag is false, this runtime flag has no + * effect on behavior (e.g. the compiler flag being off takes precedence). When the compiler flag is + * on, then the runtime flag may be used to disable the behavior at runtime. + * + *

In order to set the flag, bind a boolean value qualified with + * {@link DisableFragmentGetContextFix} into a set in the {@code SingletonComponent}. A set is used + * instead of an optional binding to avoid a dependency on Guava. Only one value may be bound into + * the set within a given app. Example for binding the value: + * + * 


+ * {@literal @}Module
+ * {@literal @}InstallIn(SingletonComponent.class)
+ * public final class DisableFragmentGetContextFixModule {
+ *   {@literal @}Provides
+ *   {@literal @}IntoSet
+ *   {@literal @}FragmentGetContextFix.DisableFragmentGetContextFix
+ *   static Boolean provideDisableFragmentGetContextFix() {
+ *     return // true or false depending on some rollout logic for your app
+ *   }
+ * }
+ *
+ */ +public final class FragmentGetContextFix { + + /** Qualifier annotation to bind disable the Fragment.getContext() fix at runtime. */ + @Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD}) + @Qualifier + public @interface DisableFragmentGetContextFix {} + + public static boolean isFragmentGetContextFixDisabled(Context context) { + // Use a set here instead of an optional to avoid the Guava dependency + Set flagSet = EntryPointAccessors.fromApplication( + context, FragmentGetContextFixEntryPoint.class).getDisableFragmentGetContextFix(); + + // TODO(b/199927963): Consider adding a plugin to check this at compile time + Preconditions.checkState(flagSet.size() <= 1, + "Cannot bind the flag @DisableFragmentGetContextFix more than once."); + + if (flagSet.isEmpty()) { + return false; + } else { + return flagSet.iterator().next(); + } + } + + /** Entry point for getting the flag. */ + @EntryPoint + @InstallIn(SingletonComponent.class) + public interface FragmentGetContextFixEntryPoint { + @DisableFragmentGetContextFix Set getDisableFragmentGetContextFix(); + } + + /** Declare the empty flag set. */ + @Module + @InstallIn(SingletonComponent.class) + abstract static class FragmentGetContextFixModule { + @Multibinds + @DisableFragmentGetContextFix + abstract Set disableFragmentGetContextFix(); + } + + private FragmentGetContextFix() { + } +} diff --git a/java/dagger/hilt/android/flags/package-info.java b/java/dagger/hilt/android/flags/package-info.java new file mode 100644 index 00000000000..1a39fc29a17 --- /dev/null +++ b/java/dagger/hilt/android/flags/package-info.java @@ -0,0 +1,27 @@ +/* + * 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. + */ + +/** + * Runtime flags to control Hilt behavior for rollout of changes. These flags are usually meant to + * be temporary and so defaults may change with releases and then these flags may eventually be + * removed, just like compiler options with similar purposes. + * + * @see Hilt Developer Docs + */ +@ParametersAreNonnullByDefault +package dagger.hilt.android.flags; + +import javax.annotation.ParametersAreNonnullByDefault; diff --git a/java/dagger/hilt/android/plugin/src/main/kotlin/dagger/hilt/android/plugin/HiltExtension.kt b/java/dagger/hilt/android/plugin/src/main/kotlin/dagger/hilt/android/plugin/HiltExtension.kt index e57cc4a52e2..0afd6dc79b8 100644 --- a/java/dagger/hilt/android/plugin/src/main/kotlin/dagger/hilt/android/plugin/HiltExtension.kt +++ b/java/dagger/hilt/android/plugin/src/main/kotlin/dagger/hilt/android/plugin/HiltExtension.kt @@ -53,7 +53,7 @@ interface HiltExtension { /** * If set to `true`, Hilt will disable cross compilation root validation. * - * See [documentation](https://dagger.dev/hilt/compiler-options#disable-cross-compilation-root-validation) + * See [documentation](https://dagger.dev/hilt/flags#disable-cross-compilation-root-validation) * for more information. */ var disableCrossCompilationRootValidation: Boolean diff --git a/java/dagger/hilt/android/processor/internal/AndroidClassNames.java b/java/dagger/hilt/android/processor/internal/AndroidClassNames.java index d3709200343..9e814a1f867 100644 --- a/java/dagger/hilt/android/processor/internal/AndroidClassNames.java +++ b/java/dagger/hilt/android/processor/internal/AndroidClassNames.java @@ -78,6 +78,9 @@ public final class AndroidClassNames { public static final ClassName VIEW_MODEL_COMPONENT = get("dagger.hilt.android.components", "ViewModelComponent"); + public static final ClassName FRAGMENT_GET_CONTEXT_FIX = + get("dagger.hilt.android.flags", "FragmentGetContextFix"); + public static final ClassName ACTIVITY_COMPONENT_MANAGER = get("dagger.hilt.android.internal.managers", "ActivityComponentManager"); public static final ClassName APPLICATION_COMPONENT_MANAGER = diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java index 642a5cedf6a..3fc33fdf88b 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java +++ b/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java @@ -23,6 +23,7 @@ import com.squareup.javapoet.FieldSpec; import com.squareup.javapoet.JavaFile; import com.squareup.javapoet.MethodSpec; +import com.squareup.javapoet.TypeName; import com.squareup.javapoet.TypeSpec; import com.squareup.javapoet.TypeVariableName; import dagger.hilt.android.processor.internal.AndroidClassNames; @@ -39,6 +40,11 @@ public final class FragmentGenerator { .addModifiers(Modifier.PRIVATE) .build(); + private static final FieldSpec DISABLE_GET_CONTEXT_FIX_FIELD = + FieldSpec.builder(TypeName.BOOLEAN, "disableGetContextFix") + .addModifiers(Modifier.PRIVATE) + .build(); + private final ProcessingEnvironment env; private final AndroidEntryPointMetadata metadata; private final ClassName generatedClassName; @@ -74,6 +80,10 @@ TypeSpec createTypeSpec() { .addMethod(getContextMethod()) .addMethod(inflatorMethod()); + if (useFragmentGetContextFix(env)) { + builder.addField(DISABLE_GET_CONTEXT_FIX_FIELD); + } + Generators.addGeneratedBaseClassJavadoc(builder, AndroidClassNames.ANDROID_ENTRY_POINT); Processors.addGeneratedAnnotation(builder, env, getClass()); Generators.copyLintAnnotations(metadata.element(), builder); @@ -158,10 +168,12 @@ private static MethodSpec onAttachActivityMethod() { // // Fragment's because we are getting it from base context instead of cloning from super // // Fragment's LayoutInflater. // componentContext = FragmentComponentManager.createContextWrapper(super.getContext(), this); + // disableGetContextFix = FragmentGetContextFix.isFragmentGetContextFixDisabled( + // super.getContext()); // } // } private MethodSpec initializeComponentContextMethod() { - return MethodSpec.methodBuilder("initializeComponentContext") + MethodSpec.Builder builder = MethodSpec.methodBuilder("initializeComponentContext") .addModifiers(Modifier.PRIVATE) .beginControlFlow("if ($N == null)", COMPONENT_CONTEXT_FIELD) .addComment( @@ -171,14 +183,22 @@ private MethodSpec initializeComponentContextMethod() { .addStatement( "$N = $T.createContextWrapper(super.getContext(), this)", COMPONENT_CONTEXT_FIELD, - metadata.componentManager()) + metadata.componentManager()); + + if (useFragmentGetContextFix(env)) { + builder.addStatement("$N = $T.isFragmentGetContextFixDisabled(super.getContext())", + DISABLE_GET_CONTEXT_FIX_FIELD, + AndroidClassNames.FRAGMENT_GET_CONTEXT_FIX); + } + + return builder .endControlFlow() .build(); } // @Override // public Context getContext() { - // if (super.getContext() == null) { + // if (super.getContext() == null && !disableGetContextFix) { // return null; // } // initializeComponentContext(); @@ -191,11 +211,17 @@ private MethodSpec getContextMethod() { .addModifiers(Modifier.PUBLIC); if (useFragmentGetContextFix(env)) { - builder.beginControlFlow("if (super.getContext() == null)"); + builder + // Note that disableGetContext can only be true if componentContext is set, so if it is + // true we don't need to check whether componentContext is set or not. + .beginControlFlow( + "if (super.getContext() == null && !$N)", + DISABLE_GET_CONTEXT_FIX_FIELD); } else { - builder.beginControlFlow( - "if (super.getContext() == null && $N == null)", - COMPONENT_CONTEXT_FIELD); + builder + .beginControlFlow( + "if (super.getContext() == null && $N == null)", + COMPONENT_CONTEXT_FIELD); } return builder diff --git a/java/dagger/hilt/processor/internal/aggregateddeps/AggregatedDepsProcessor.java b/java/dagger/hilt/processor/internal/aggregateddeps/AggregatedDepsProcessor.java index 239c4faba47..184894af888 100644 --- a/java/dagger/hilt/processor/internal/aggregateddeps/AggregatedDepsProcessor.java +++ b/java/dagger/hilt/processor/internal/aggregateddeps/AggregatedDepsProcessor.java @@ -134,7 +134,7 @@ private void processModule( || installInCheckDisabled(element), element, "%s is missing an @InstallIn annotation. If this was intentional, see" - + " https://dagger.dev/hilt/compiler-options#disable-install-in-check for how to disable this" + + " https://dagger.dev/hilt/flags#disable-install-in-check for how to disable this" + " check.", element); diff --git a/javatests/dagger/hilt/android/BUILD b/javatests/dagger/hilt/android/BUILD index 73abc007994..abb131b66a1 100644 --- a/javatests/dagger/hilt/android/BUILD +++ b/javatests/dagger/hilt/android/BUILD @@ -188,6 +188,9 @@ android_local_test( name = "FragmentContextOnAttachTest", size = "small", srcs = ["FragmentContextOnAttachTest.java"], + javacopts = [ + "-Adagger.hilt.android.useFragmentGetContextFix=true", + ], manifest_values = { "minSdkVersion": "14", }, @@ -198,6 +201,7 @@ android_local_test( "//java/dagger/hilt:install_in", "//java/dagger/hilt/android:android_entry_point", "//java/dagger/hilt/android:package_info", + "//java/dagger/hilt/android/flags:fragment_get_context_fix", "//java/dagger/hilt/android/testing:bind_value", "//java/dagger/hilt/android/testing:hilt_android_test", "@google_bazel_common//third_party/java/jsr330_inject", diff --git a/javatests/dagger/hilt/android/FragmentContextOnAttachTest.java b/javatests/dagger/hilt/android/FragmentContextOnAttachTest.java index 51d33b8e324..19b3d05aa24 100644 --- a/javatests/dagger/hilt/android/FragmentContextOnAttachTest.java +++ b/javatests/dagger/hilt/android/FragmentContextOnAttachTest.java @@ -24,6 +24,8 @@ import androidx.fragment.app.Fragment; import androidx.fragment.app.FragmentActivity; import androidx.test.ext.junit.runners.AndroidJUnit4; +import dagger.hilt.android.flags.FragmentGetContextFix; +import dagger.hilt.android.testing.BindValueIntoSet; import dagger.hilt.android.testing.HiltAndroidRule; import dagger.hilt.android.testing.HiltAndroidTest; import dagger.hilt.android.testing.HiltTestApplication; @@ -41,6 +43,10 @@ public final class FragmentContextOnAttachTest { @Rule public final HiltAndroidRule rule = new HiltAndroidRule(this); + @BindValueIntoSet + @FragmentGetContextFix.DisableFragmentGetContextFix + boolean disableGetContextFix = false; + /** Hilt Activity */ @AndroidEntryPoint(FragmentActivity.class) public static final class TestActivity extends Hilt_FragmentContextOnAttachTest_TestActivity {} @@ -75,15 +81,24 @@ public void testGetContextAvailableBeforeSuperOnAttach() throws Exception { assertThat(fragment.onAttachActivityContext).isNotNull(); } - // Tests the default behavior of the useFragmentGetContextFix flag. Current default is false - // so this tests the broken behavior. + // Tests the behavior when using the useFragmentGetContextFix flag. @Test - public void testGetContextDoesNotReturnNullAfterRemoval() throws Exception { + public void testGetContextReturnsNullAfterRemoval() throws Exception { FragmentActivity activity = Robolectric.setupActivity(TestActivity.class); TestFragment fragment = new TestFragment(); activity.getSupportFragmentManager().beginTransaction().add(fragment, "").commitNow(); assertThat(fragment.getContext()).isNotNull(); activity.getSupportFragmentManager().beginTransaction().remove(fragment).commitNow(); - assertThat(fragment.getContext()).isNotNull(); + // This should be null since the fix was enabled by the compiler flag and runtime flag + assertThat(fragment.getContext()).isNull(); + + // Flip the flag so that we now disable the fix + disableGetContextFix = true; + TestFragment fragment2 = new TestFragment(); + activity.getSupportFragmentManager().beginTransaction().add(fragment2, "").commitNow(); + assertThat(fragment2.getContext()).isNotNull(); + activity.getSupportFragmentManager().beginTransaction().remove(fragment2).commitNow(); + // This should not be null since the fix was disabled by the runtime flag + assertThat(fragment2.getContext()).isNotNull(); } }