Commit d3f64670 authored by Robbie McElrath's avatar Robbie McElrath Committed by Commit Bot

[WebLayer] Update Fragment.getActivity() implementation.

This CL updates the bytecode modifications we perform on
Fragment.getActivity() to have it call ContextUtils.activityFromContext(
getContext()), and modifies compile_java.py to detect compilation errors
related to this change and print a message pointing you to documentation
about the bytecode modifications.

A compilation error will now print:
../...../PasswordEntryEditor.java:144: error: cannot find symbol
                getActivity().getSupportFragmentManager(),
                             ^
  symbol:   method getSupportFragmentManager()
  location: class Activity
            Expecting a FragmentActivity? See docs/.......md

Bug: 1123216
Change-Id: I04016e30fba46c7d5c592e9ab2c5e5fa4c4cfde1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2508352
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: default avatarAndrew Grieve <agrieve@chromium.org>
Reviewed-by: default avatarNatalie Chouinard <chouinard@chromium.org>
Reviewed-by: default avatarBoris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828643}
parent 6468cd76
...@@ -20,15 +20,11 @@ import java.io.IOException; ...@@ -20,15 +20,11 @@ import java.io.IOException;
* See crbug.com/1144345 for more context. * See crbug.com/1144345 for more context.
*/ */
public class FragmentActivityReplacer extends ByteCodeRewriter { public class FragmentActivityReplacer extends ByteCodeRewriter {
private static final String FRAGMENT_CLASS_PATH = "androidx/fragment/app/Fragment.class";
private static final String FRAGMENT_ACTIVITY_INTERNAL_CLASS_NAME =
"androidx/fragment/app/FragmentActivity";
private static final String ACTIVITY_INTERNAL_CLASS_NAME = "android/app/Activity";
private static final String GET_ACTIVITY_METHOD_NAME = "getActivity"; private static final String GET_ACTIVITY_METHOD_NAME = "getActivity";
private static final String REQUIRE_ACTIVITY_METHOD_NAME = "requireActivity"; private static final String NEW_METHOD_DESCRIPTOR = "()Landroid/app/Activity;";
private static final String OLD_METHOD_DESCRIPTOR = private static final String OLD_METHOD_DESCRIPTOR =
"()Landroidx/fragment/app/FragmentActivity;"; "()Landroidx/fragment/app/FragmentActivity;";
private static final String NEW_METHOD_DESCRIPTOR = "()Landroid/app/Activity;"; private static final String REQUIRE_ACTIVITY_METHOD_NAME = "requireActivity";
public static void main(String[] args) throws IOException { public static void main(String[] args) throws IOException {
// Invoke this script using //build/android/gyp/bytecode_processor.py // Invoke this script using //build/android/gyp/bytecode_processor.py
...@@ -49,7 +45,7 @@ public class FragmentActivityReplacer extends ByteCodeRewriter { ...@@ -49,7 +45,7 @@ public class FragmentActivityReplacer extends ByteCodeRewriter {
@Override @Override
protected ClassVisitor getClassVisitorForClass(String classPath, ClassVisitor delegate) { protected ClassVisitor getClassVisitorForClass(String classPath, ClassVisitor delegate) {
ClassVisitor getActivityReplacer = new GetActivityReplacer(delegate); ClassVisitor getActivityReplacer = new GetActivityReplacer(delegate);
if (classPath.equals(FRAGMENT_CLASS_PATH)) { if (classPath.equals("androidx/fragment/app/Fragment.class")) {
return new FragmentClassVisitor(getActivityReplacer); return new FragmentClassVisitor(getActivityReplacer);
} }
return getActivityReplacer; return getActivityReplacer;
...@@ -84,8 +80,7 @@ public class FragmentActivityReplacer extends ByteCodeRewriter { ...@@ -84,8 +80,7 @@ public class FragmentActivityReplacer extends ByteCodeRewriter {
} }
/** /**
* Makes Fragment.getActivity() and Fragment.requireActivity() non-final, and changes their * Updates the implementation of Fragment.getActivity() and Fragment.requireActivity().
* return types to Activity.
*/ */
private static class FragmentClassVisitor extends ClassVisitor { private static class FragmentClassVisitor extends ClassVisitor {
private FragmentClassVisitor(ClassVisitor baseVisitor) { private FragmentClassVisitor(ClassVisitor baseVisitor) {
...@@ -95,25 +90,36 @@ public class FragmentActivityReplacer extends ByteCodeRewriter { ...@@ -95,25 +90,36 @@ public class FragmentActivityReplacer extends ByteCodeRewriter {
@Override @Override
public MethodVisitor visitMethod( public MethodVisitor visitMethod(
int access, String name, String descriptor, String signature, String[] exceptions) { int access, String name, String descriptor, String signature, String[] exceptions) {
MethodVisitor base; // Replace FragmentActivity with Fragment in requireActivity().
// Update the descriptor of getActivity/requireActivity, and make them non-final. if (name.equals(REQUIRE_ACTIVITY_METHOD_NAME)) {
if (name.equals(GET_ACTIVITY_METHOD_NAME) return new MethodRemapper(super.visitMethod(access & ~Opcodes.ACC_FINAL, name,
|| name.equals(REQUIRE_ACTIVITY_METHOD_NAME)) { NEW_METHOD_DESCRIPTOR, null, exceptions),
base = super.visitMethod( new Remapper() {
@Override
public String mapType(String internalName) {
if (internalName.equals("androidx/fragment/app/FragmentActivity")) {
return "android/app/Activity";
}
return internalName;
}
});
}
// Replace getActivity() with ContextUtils.activityFromContext(getContext()).
if (name.equals(GET_ACTIVITY_METHOD_NAME)) {
MethodVisitor visitor = super.visitMethod(
access & ~Opcodes.ACC_FINAL, name, NEW_METHOD_DESCRIPTOR, null, exceptions); access & ~Opcodes.ACC_FINAL, name, NEW_METHOD_DESCRIPTOR, null, exceptions);
} else { visitor.visitVarInsn(Opcodes.ALOAD, 0);
base = super.visitMethod(access, name, descriptor, signature, exceptions); visitor.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "androidx/fragment/app/Fragment",
"getContext", "()Landroid/content/Context;", false);
visitor.visitMethodInsn(Opcodes.INVOKESTATIC, "org/chromium/utils/ContextUtils",
"activityFromContext", "(Landroid/content/Context;)Landroid/app/Activity;",
false);
visitor.visitInsn(Opcodes.ARETURN);
return null;
} }
return new MethodRemapper(base, new Remapper() { return super.visitMethod(access, name, descriptor, signature, exceptions);
@Override
public String mapType(String internalName) {
if (internalName.equals(FRAGMENT_ACTIVITY_INTERNAL_CLASS_NAME)) {
return ACTIVITY_INTERNAL_CLASS_NAME;
}
return internalName;
}
});
} }
} }
} }
...@@ -207,6 +207,8 @@ def ProcessJavacOutput(output): ...@@ -207,6 +207,8 @@ def ProcessJavacOutput(output):
r'(Note: .* uses? unchecked or unsafe operations.)$') r'(Note: .* uses? unchecked or unsafe operations.)$')
recompile_re = re.compile(r'(Note: Recompile with -Xlint:.* for details.)$') recompile_re = re.compile(r'(Note: Recompile with -Xlint:.* for details.)$')
activity_re = re.compile(r'^(?P<prefix>\s*location: )class Activity$')
warning_color = ['full_message', colorama.Fore.YELLOW + colorama.Style.DIM] warning_color = ['full_message', colorama.Fore.YELLOW + colorama.Style.DIM]
error_color = ['full_message', colorama.Fore.MAGENTA + colorama.Style.BRIGHT] error_color = ['full_message', colorama.Fore.MAGENTA + colorama.Style.BRIGHT]
marker_color = ['marker', colorama.Fore.BLUE + colorama.Style.BRIGHT] marker_color = ['marker', colorama.Fore.BLUE + colorama.Style.BRIGHT]
...@@ -222,6 +224,13 @@ def ProcessJavacOutput(output): ...@@ -222,6 +224,13 @@ def ProcessJavacOutput(output):
return not (deprecated_re.match(line) or unchecked_re.match(line) return not (deprecated_re.match(line) or unchecked_re.match(line)
or recompile_re.match(line)) or recompile_re.match(line))
def Elaborate(line):
if activity_re.match(line):
prefix = ' ' * activity_re.match(line).end('prefix')
return '{}\n{}Expecting a FragmentActivity? See {}'.format(
line, prefix, 'docs/ui/android/bytecode_rewriting.md')
return line
def ApplyColors(line): def ApplyColors(line):
if warning_re.match(line): if warning_re.match(line):
line = Colorize(line, warning_re, warning_color) line = Colorize(line, warning_re, warning_color)
...@@ -231,7 +240,9 @@ def ProcessJavacOutput(output): ...@@ -231,7 +240,9 @@ def ProcessJavacOutput(output):
line = Colorize(line, marker_re, marker_color) line = Colorize(line, marker_re, marker_color)
return line return line
return '\n'.join(map(ApplyColors, filter(ApplyFilters, output.split('\n')))) lines = (l for l in output.split('\n') if ApplyFilters(l))
lines = (ApplyColors(Elaborate(l)) for l in lines)
return '\n'.join(lines)
def CheckErrorproneStderrWarning(jar_path, expected_warning_regex, def CheckErrorproneStderrWarning(jar_path, expected_warning_regex,
......
# Bytecode Rewriting
## TL;DR
We modify the return type of AndroidX's `Fragment.getActivity()` method from `FragmentActivity`
to `Activity` to more easily add Fragments from multiple ClassLoaders into the same Fragment tree.
## Why?
In Java, two instances of the same class loaded from two different ClassLoaders aren't compatible
with each other at runtime. Because AndroidX libraries are bundled with each APK that use them, and
different APKs are loaded with different ClassLoaders, AndroidX classes from one APK cannot be used
with the same class from another APK. This causes problems for Fragment-based UIs in WebLayer, where
the implementation is in a different ClassLoader than the embedding app, so its Fragments cannot be
added to the embedding app's Fragment tree.
Note that this issue doesn't apply to Framework or standard library classes. Java ClassLoaders form
a tree, and if a ClassLoader can't find a particular class, it delegates to its parent.
The leaf ClassLoader used to load an app is responsible for loading the app's class files, while one
of its parents will load system-level classes. Because AndroidX classes get loaded by the
app-specific ClassLoader, different apps will load mutually incompatible versions, but a class like
Activity, which gets loaded from a parent ClassLoader, *will* be compatible between APKs at runtime,
because it ends up gets loaded from a common ClassLoader.
To get around this incompatibility, we can create a RemoteFragment that lives in the embedding app,
and a RemoteFragmentImpl that lives in another APK. The RemoteFragment can be added to the original
Fragment tree, and will forward all Fragment lifecycle events over an AIDL interface to
RemoteFragmentImpl. The fake Fragment in the secondary APK (RemoteFragmentImpl) can create a
FragmentController, which allows it to become the host of its own Fragment tree, and any UIs from
the secondary ClassLoaded can be added to this new Fragment tree that's been essentially grafted
onto the original.
This mostly works, but runs into issues when Fragments call `Fragment.getActivity()`, which they do
a lot. The getActivity implementation takes the Activity given to the FragmentController constructor
(via FragmentHostCallback), and casts it to a FragmentActivity before returning it. The original
Activity will typically be a FragmentActivity from the embedding app's ClassLoader, which means that
due to the aforementioned issues, this cast will fail when run in the secondary ClassLoader's
Fragment class because even though the Activity is a FragmentActivity, it's from the wrong
ClassLoader.
To fix this second issue, we modify the bytecode of `Fragment.getActivity()` in the AndroidX
prebuilt .aar files to return a plain Activity instead of a FragmentActivity. This allows us to
continue calling getActivity() as normal. Note that this does mean FragmentActivity-specific methods
can no longer be used in Fragments, but there were no uses of them in Chromium that couldn't be
trivially removed as of late 2020.
## How does it work?
The bytecode rewriting happens at build time by
[FragmentActivityReplacer](https://source.chromium.org/chromium/chromium/src/+/master:build/android/bytecode/java/org/chromium/bytecode/FragmentActivityReplacer.java),
which is specified as a bytecode rewriter via the `bytecode_rewriter_target` rule. Compilation errors
related to this should get detected by
[compile_java.py](https://source.chromium.org/chromium/chromium/src/+/master:build/android/gyp/compile_java.py),
and print a message pointing users here, which is likely why you're reading this :)
## How does this affect my code?
The goal is for these changes to be as transparent as possible; most code shouldn't run into issues.
However, if there's no way around calling a FragmentActivity method in your code, **and your
Fragment is in //chrome**, you could cast the Activity to a FragmentActivity as AndroidX used to do.
If your Fragment is in //components, FragmentActivity methods will likely not work directly, and may
need to be forwarded to an implementation in the original ClassLoader somehow.
The more important thing to note is that in a multi-ClassLoader world, `getActivity()` and
`getContext()` will typically return two different objects, so we need to be more careful about which
method we call, particularly for code in //components. `getActivity()` will return the Activity from
the original ClassLoader, and should be used to for calls like `.finish*()`, `.setTitle(), and
`.startActivity()` (which live in Activity anyway). When loading resources, you should default to
calling `getContext()`, as resources usually come from the same ClassLoader as the Fragment, and the
Context should be configured to load them correctly.
As a rule of thumb, prefer `getContext()` to `getActivity()`, unless you need to operate on the
Activity itself, or you know the resource or setting you need belongs to the original Activity.
...@@ -364,7 +364,10 @@ android_aar_prebuilt("androidx_fragment_fragment_java") { ...@@ -364,7 +364,10 @@ android_aar_prebuilt("androidx_fragment_fragment_java") {
":androidx_viewpager_viewpager_java", ":androidx_viewpager_viewpager_java",
] ]
resource_overlay = true resource_overlay = true
deps += [ "//third_party/android_deps/local_modifications/androidx_fragment_fragment:androidx_fragment_fragment_prebuilt_java" ] deps += [
"//third_party/android_deps/local_modifications/androidx_fragment_fragment:androidx_fragment_fragment_prebuilt_java",
"//third_party/android_deps/utils:java",
]
# Omit this file since we use our own copy, included above. # Omit this file since we use our own copy, included above.
# We can remove this once we migrate to AndroidX master for all libraries. # We can remove this once we migrate to AndroidX master for all libraries.
......
...@@ -346,7 +346,10 @@ class BuildConfigGenerator extends DefaultTask { ...@@ -346,7 +346,10 @@ class BuildConfigGenerator extends DefaultTask {
break break
case 'androidx_fragment_fragment': case 'androidx_fragment_fragment':
sb.append("""\ sb.append("""\
| deps += [ "//third_party/android_deps/local_modifications/androidx_fragment_fragment:androidx_fragment_fragment_prebuilt_java" ] | deps += [
| "//third_party/android_deps/utils:java",
| "//third_party/android_deps/local_modifications/androidx_fragment_fragment:androidx_fragment_fragment_prebuilt_java",
| ]
| # Omit this file since we use our own copy, included above. | # Omit this file since we use our own copy, included above.
| # We can remove this once we migrate to AndroidX master for all libraries. | # We can remove this once we migrate to AndroidX master for all libraries.
| jar_excluded_patterns = [ | jar_excluded_patterns = [
......
# Copyright 2020 The Chromium Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
import("//build/config/android/rules.gni")
android_library("java") {
sources = [ "java/org/chromium/utils/ContextUtils.java" ]
deps = [ "//third_party/android_deps:androidx_annotation_annotation_java" ]
}
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.utils;
import android.app.Activity;
import android.content.Context;
import android.content.ContextWrapper;
import androidx.annotation.Nullable;
/**
* Contains a helper method needed by the AndroidX Fragment library due to bytecode modification we
* perform at build time.
*/
public class ContextUtils {
private ContextUtils() {}
/**
* Extract the {@link Activity} if the given {@link Context} either is or wraps one.
*
* Copied from //base/android/java/src/org/chromium/base/ContextUtils.java
*
* @param context The context to check.
* @return Extracted activity if it exists, otherwise null.
*/
public static @Nullable Activity activityFromContext(@Nullable Context context) {
// Only retrieves the base context if the supplied context is a ContextWrapper but not an
// Activity, because Activity is a subclass of ContextWrapper.
while (context instanceof ContextWrapper) {
if (context instanceof Activity) return (Activity) context;
context = ((ContextWrapper) context).getBaseContext();
}
return null;
}
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment