Android: Make R.java inheritance work with Trichrome proguard.

212f516e changed the way R.java files
work so that all resources are stored in a base R.java file, but
accessed through R.java classes that subclass the base R.java.

Currently for synchronized proguard, we combine all of the jars
produced by the APK's java_library_impl() step. This dedupes class
files, but also requires classes with the same name to be identical.
With R.java inheritance, each APK will have a base module R.java
with different contents, and Chrome will crash at runtime due to
missing resources.

We can't just rename the base module name in the various APKs that
are part of synchronized proguarding because the subclass R.java
files can only be included once, and can only have one direct
superclass.

Instead of creating a more complex R.java class hierarchy, this CL
changes the compile_resources() step of the static library APK to
generate a base R.java that contains the resources of both Chrome
and Webview.

Bug: 901465
Change-Id: Iaa2a05c75cd431b95afe6318da8c7982fffbc242
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1695699
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676552}

build/android/gyp/compile_resources.py

@@ -138,6 +138,13 @@ def _ParseArgs(args):
       '--use-resource-ids-path',
       help='Use resource IDs generated by aapt --emit-ids.')
 
+  input_opts.add_argument(
+      '--extra-main-r-text-files',
+      help='Additional R.txt files that will be added to the root R.java file, '
+      'but not packaged in the generated resources.arsc. If these resources '
+      'entries contain duplicate resources with the generated R.txt file, they '
+      'must be identical.')
+
   input_opts.add_argument(
       '--support-zh-hk',
       action='store_true',
@@ -245,6 +252,8 @@ def _ParseArgs(args):
       options.shared_resources_whitelist_locales)
   options.resource_blacklist_exceptions = build_utils.ParseGnList(
       options.resource_blacklist_exceptions)
+  options.extra_main_r_text_files = build_utils.ParseGnList(
+      options.extra_main_r_text_files)
 
   if options.optimized_proto_path and not options.proto_path:
     # We could write to a temp file, but it's simpler to require it.
@@ -972,7 +981,8 @@ def main(args):
     resource_utils.CreateRJavaFiles(
         build.srcjar_dir, None, build.r_txt_path, options.extra_res_packages,
         options.extra_r_text_files, rjava_build_options, options.srcjar_out,
-        custom_root_package_name, grandparent_custom_package_name)
+        custom_root_package_name, grandparent_custom_package_name,
+        options.extra_main_r_text_files)
 
     build_utils.ZipDir(build.srcjar_path, build.srcjar_dir)
 

build/android/gyp/util/resource_utils.py

@@ -335,7 +335,8 @@ def CreateRJavaFiles(srcjar_dir,
                      rjava_build_options,
                      srcjar_out,
                      custom_root_package_name=None,
-                     grandparent_custom_package_name=None):
+                     grandparent_custom_package_name=None,
+                     extra_main_r_text_files=None):
   """Create all R.java files for a set of packages and R.txt files.
 
   Args:
@@ -358,6 +359,7 @@ def CreateRJavaFiles(srcjar_dir,
       as the grandparent_custom_package_name. The format of this package name
       is identical to custom_root_package_name.
       (eg. for vr grandparent_custom_package_name would be "base")
+    extra_main_r_text_files: R.txt files to be added to the root R.java file.
   Raises:
     Exception if a package name appears several times in |extra_res_packages|
   """
@@ -378,12 +380,23 @@ def CreateRJavaFiles(srcjar_dir,
   all_resources = {}
   all_resources_by_type = collections.defaultdict(list)
 
-  for entry in _ParseTextSymbolsFile(main_r_txt_file, fix_package_ids=True):
-    all_resources[(entry.resource_type, entry.name)] = entry
-    all_resources_by_type[entry.resource_type].append(entry)
-    assert entry.resource_type in _ALL_RESOURCE_TYPES, (
-        'Unknown resource type: %s, add to _ALL_RESOURCE_TYPES!' %
-        entry.resource_type)
+  main_r_text_files = [main_r_txt_file]
+  if extra_main_r_text_files:
+    main_r_text_files.extend(extra_main_r_text_files)
+  for r_txt_file in main_r_text_files:
+    for entry in _ParseTextSymbolsFile(r_txt_file, fix_package_ids=True):
+      entry_key = (entry.resource_type, entry.name)
+      if entry_key in all_resources:
+        assert entry == all_resources[entry_key], (
+            'Input R.txt %s provided a duplicate resource with a different '
+            'entry value. Got %s, expected %s.' % (r_txt_file, entry,
+                                                   all_resources[entry_key]))
+      else:
+        all_resources[entry_key] = entry
+        all_resources_by_type[entry.resource_type].append(entry)
+        assert entry.resource_type in _ALL_RESOURCE_TYPES, (
+            'Unknown resource type: %s, add to _ALL_RESOURCE_TYPES!' %
+            entry.resource_type)
 
   if custom_root_package_name:
     # Custom package name is available, thus use it for root_r_java_package.

build/config/android/internal_rules.gni

@@ -309,7 +309,7 @@ template("write_build_config") {
     }
     if (defined(invoker.r_text)) {
       args += [
-        "--r-text",
+        "--r-text-path",
         rebase_path(invoker.r_text, root_build_dir),
       ]
     }
@@ -328,9 +328,9 @@ template("write_build_config") {
           rebase_path(invoker.proto_resources_path, root_build_dir)
       args += [ "--apk-proto-resources=$_rebased_proto_resources" ]
     }
-    if (defined(invoker.module_rtxt_path)) {
-      _rebased_rtxt_path = rebase_path(invoker.module_rtxt_path, root_build_dir)
-      args += [ "--module-rtxt-path=$_rebased_rtxt_path" ]
+    if (defined(invoker.r_text_path)) {
+      _rebased_rtxt_path = rebase_path(invoker.r_text_path, root_build_dir)
+      args += [ "--r-text-path=$_rebased_rtxt_path" ]
     }
     if (defined(invoker.module_pathmap_path)) {
       _rebased_pathmap_path =
@@ -456,11 +456,6 @@ template("write_build_config") {
       args += [ "--proguard-configs=$_rebased_proguard_configs" ]
     }
     if (defined(invoker.static_library_dependent_targets)) {
-      assert(defined(invoker.static_library_jar_path))
-      args += [
-        "--static-library-jar-path",
-        rebase_path(invoker.static_library_jar_path, root_build_dir),
-      ]
       _dependent_configs = []
       foreach(_target, invoker.static_library_dependent_targets) {
         _target_name = _target.name
@@ -1864,9 +1859,9 @@ if (enable_java_templates) {
         "--include-resources=@FileArg($_rebased_build_config:android:sdk_jars)",
         "--aapt-path",
         rebase_path(_android_aapt_path, root_build_dir),
-        "--dependencies-res-zips=@FileArg($_rebased_build_config:resources:dependency_zips)",
-        "--extra-res-packages=@FileArg($_rebased_build_config:resources:extra_package_names)",
-        "--extra-r-text-files=@FileArg($_rebased_build_config:resources:extra_r_text_files)",
+        "--dependencies-res-zips=@FileArg($_rebased_build_config:deps_info:dependency_zips)",
+        "--extra-res-packages=@FileArg($_rebased_build_config:deps_info:extra_package_names)",
+        "--extra-r-text-files=@FileArg($_rebased_build_config:deps_info:extra_r_text_files)",
       ]
 
       if (defined(invoker.android_manifest)) {
@@ -2112,9 +2107,10 @@ if (enable_java_templates) {
         "--include-resources=@FileArg($_rebased_build_config:android:sdk_jars)",
         "--aapt2-path",
         rebase_path(android_sdk_tools_bundle_aapt2, root_build_dir),
-        "--dependencies-res-zips=@FileArg($_rebased_build_config:resources:dependency_zips)",
-        "--extra-res-packages=@FileArg($_rebased_build_config:resources:extra_package_names)",
-        "--extra-r-text-files=@FileArg($_rebased_build_config:resources:extra_r_text_files)",
+        "--dependencies-res-zips=@FileArg($_rebased_build_config:deps_info:dependency_zips)",
+        "--extra-res-packages=@FileArg($_rebased_build_config:deps_info:extra_package_names)",
+        "--extra-r-text-files=@FileArg($_rebased_build_config:deps_info:extra_r_text_files)",
+        "--extra-main-r-text-files=@FileArg($_rebased_build_config:deps_info:extra_main_r_text_files)",
         "--min-sdk-version=${invoker.min_sdk_version}",
         "--target-sdk-version=${invoker.target_sdk_version}",
       ]
@@ -2341,6 +2337,7 @@ if (enable_java_templates) {
         args += [ "--use-resource-ids-path=$_rebased_ids_path" ]
         deps += [ _compile_res_dep ]
       }
+
       if (defined(invoker.max_sdk_version)) {
         _max_sdk_version = invoker.max_sdk_version
         args += [ "--max-sdk-version=$_max_sdk_version" ]
@@ -2393,7 +2390,7 @@ if (enable_java_templates) {
           args += [
             "--r-text-whitelist",
             rebase_path(invoker.shared_resources_whitelist, root_build_dir),
-            "--r-text",
+            "--r-text-path",
             rebase_path(invoker.r_text_out_path, root_build_dir),
           ]
         }
@@ -2458,7 +2455,7 @@ if (enable_java_templates) {
           args += [
             "--jar-files=@FileArg($_rebased_build_config:deps_info:unprocessed_jar_path)",
             "--jar-files=@FileArg($_rebased_build_config:deps_info:javac_full_classpath)",
-            "--in-res-info-path=@FileArg($_rebased_build_config:resources:size_info)",
+            "--in-res-info-path=@FileArg($_rebased_build_config:deps_info:res_size_info)",
             "--assets=@FileArg($_rebased_build_config:assets)",
             "--uncompressed-assets=@FileArg($_rebased_build_config:uncompressed_assets)",
           ]
@@ -3042,8 +3039,6 @@ if (enable_java_templates) {
   #    java_files is empty. If not
   #  jar_path: Optional path to a prebuilt .jar file for this target.
   #    Mutually exclusive with java_files and srcjar_deps.
-  #  intermediate_jar_path: Optional path to the output .jar file. If used,
-  #    final_jar_path must be created by another target.
   #  final_jar_path: Optional path to the final output .jar file (after
   #    processing). If not provided, the output will go under
   #    $root_build_dir/lib.java/
@@ -3129,8 +3124,8 @@ if (enable_java_templates) {
   #  proto_resources_path: The path of an zip archive containing the APK's
   #    resources compiled to the protocol buffer format (instead of regular
   #    binary xml + resources.arsc).
-  #  module_rtxt_path: The path of the R.txt file generated when compiling the
-  #    resources for the bundle module.
+  #  r_text_path: The path of the R.txt file generated when compiling the
+  #    resources for this target.
   #  module_pathmap_path: The path of the pathmap file generated when compiling
   #    the resources for the bundle module, if path shortening is enabled.
   #  base_whitelist_rtxt_path: The path of the R.txt file containing the
@@ -3217,9 +3212,6 @@ if (enable_java_templates) {
       # for the ijar as well, but this is only used for APK targets where
       # the ijar path isn't actually used.
       _build_config_jar_path = _final_jar_path
-      if (defined(invoker.intermediate_jar_path)) {
-        _final_jar_path = invoker.intermediate_jar_path
-      }
       _final_ijar_path =
           get_path_info(_final_jar_path, "dir") + "/" +
           get_path_info(_final_jar_path, "name") + ".interface.jar"
@@ -3338,6 +3330,7 @@ if (enable_java_templates) {
                                "proguard_configs",
                                "proguard_enabled",
                                "proguard_mapping_path",
+                               "r_text_path",
                                "secondary_abi_loadable_modules",
                                "type",
                              ])
@@ -3374,7 +3367,6 @@ if (enable_java_templates) {
                                  "base_module_target",
                                  "is_base_module",
                                  "module_pathmap_path",
-                                 "module_rtxt_path",
                                  "proto_resources_path",
                                ])
       }
@@ -3389,9 +3381,6 @@ if (enable_java_templates) {
       bypass_platform_checks = defined(invoker.bypass_platform_checks) &&
                                invoker.bypass_platform_checks
 
-      if (defined(invoker.intermediate_jar_path)) {
-        static_library_jar_path = invoker.intermediate_jar_path
-      }
       if (defined(_final_jar_path)) {
         jar_path = _build_config_jar_path
         ijar_path = _final_ijar_path

build/config/android/rules.gni

@@ -1757,8 +1757,8 @@ if (enable_java_templates) {
         rebase_path(depfile, root_build_dir),
         "--output",
         rebase_path(invoker.output, root_build_dir),
-        "--dependencies-res-zips=@FileArg($_rebased_build_config:resources:dependency_zips)",
-        "--r-text-files=@FileArg($_rebased_build_config:resources:extra_r_text_files)",
+        "--dependencies-res-zips=@FileArg($_rebased_build_config:deps_info:dependency_zips)",
+        "--r-text-files=@FileArg($_rebased_build_config:deps_info:extra_r_text_files)",
         "--proguard-configs=@FileArg($_rebased_build_config:deps_info:proguard_all_configs)",
       ]
       if (_direct_deps_only) {
@@ -2308,7 +2308,6 @@ if (enable_java_templates) {
     _is_static_library_provider =
         defined(invoker.static_library_dependent_targets) && _proguard_enabled
     if (_is_static_library_provider) {
-      _static_library_apk_java_target_output = _jar_path
       _static_library_sync_dex_path =
           "$_gen_dir/static_library_synchronized_proguard.classes.dex.zip"
       _resource_ids_provider_deps = []
@@ -2461,13 +2460,21 @@ if (enable_java_templates) {
                _android_sdk_dep,
              ]
 
+      # The static library uses the R.txt files generated by the
+      # static_library_dependent_targets when generating the final R.java file.
+      if (_is_static_library_provider) {
+        foreach(_dep, invoker.static_library_dependent_targets) {
+          deps += [ "${_dep.name}__compile_resources" ]
+        }
+      }
+
       if (defined(invoker.apk_under_test)) {
         # Set the arsc package name to match the apk_under_test package name
         # So that test resources can references under_test resources via
         # @type/name syntax.
         r_java_root_package_name = "test"
         arsc_package_name =
-            "@FileArg($_rebased_build_config:resources:arsc_package_name)"
+            "@FileArg($_rebased_build_config:deps_info:arsc_package_name)"
 
         # Passing in the --emit-ids mapping will cause aapt2 to assign resources
         # IDs that do not conflict with those from apk_under_test.
@@ -2576,7 +2583,20 @@ if (enable_java_templates) {
       }
     }
 
-    _srcjar_deps += [ ":$_compile_resources_target" ]
+    # The static library will provide all R.java files, but we still need to
+    # have a dep on the compile_resources() target from the java_library_impl()
+    # target below. Using a group to rename the target avoids the naming
+    # restrictions described in crbug.com/908819.
+    if (_uses_static_library) {
+      group("${_compile_resources_target}__compile_only_dep") {
+        public_deps = [
+          ":$_compile_resources_target",
+        ]
+      }
+      _deps += [ ":${_compile_resources_target}__compile_only_dep" ]
+    } else {
+      _srcjar_deps += [ ":$_compile_resources_target" ]
+    }
 
     if (_native_libs_deps != [] || _secondary_abi_native_libs_deps != []) {
       _enable_chromium_linker_tests = false
@@ -2773,24 +2793,19 @@ if (enable_java_templates) {
       } else {
         type = "android_apk"
       }
+      r_text_path = _compile_resources_rtxt_out
       main_target_name = _template_name
       supports_android = true
       requires_android = true
       min_sdk_version = _min_sdk_version
       deps = _deps
-
       srcjar_deps = _srcjar_deps
       final_jar_path = _jar_path
-      if (_is_static_library_provider) {
-        intermediate_jar_path = "$_base_path.intermediate.jar"
-        final_jar_path = _static_library_apk_java_target_output
-      }
       dex_path = _lib_dex_path
       final_dex_path = _final_dex_path
 
       if (_is_bundle_module) {
         proto_resources_path = _proto_resources_path
-        module_rtxt_path = _compile_resources_rtxt_out
         if (_optimize_resources) {
           proto_resources_path = _optimized_proto_resources_path
           if (_short_resource_paths) {
@@ -2859,38 +2874,6 @@ if (enable_java_templates) {
       }
     }
 
-    if (_is_static_library_provider) {
-      _static_library_apk_java_target = "${_template_name}__combine_apk_jars"
-
-      # Since some of the static_libary_dependent_targets may have overlapping
-      # resource dependencies, we can't include all jars created by the
-      # static_library_dependent_targets or else proguard will fail with
-      # duplicate class definitions. This step combines these jars into
-      # a single jar with duplicates ignored.
-      action_with_pydeps(_static_library_apk_java_target) {
-        script = "//build/android/gyp/zip.py"
-        deps = [
-          ":$_build_config_target",
-          ":$_java_target",
-        ]
-        foreach(_dep, invoker.static_library_dependent_targets) {
-          _target_label = get_label_info(_dep.name, "label_no_toolchain")
-          deps += [ "${_target_label}__java" ]
-        }
-        inputs = [
-          _build_config,
-        ]
-        outputs = [
-          _static_library_apk_java_target_output,
-        ]
-        args = [
-          "--input-zips=@FileArg($_rebased_build_config:deps_info:static_library_dependent_apk_jars)",
-          "--output",
-          rebase_path(_static_library_apk_java_target_output, root_build_dir),
-        ]
-      }
-    }
-
     if (_proguard_enabled && _uses_static_library) {
       _final_dex_target_dep = "${invoker.static_library_provider}__dexsplitter"
     } else if (!(_is_bundle_module && _proguard_enabled)) {
@@ -2913,9 +2896,6 @@ if (enable_java_templates) {
           ":$_build_config_target",
           ":$_java_target",
         ]
-        if (_is_static_library_provider) {
-          deps += [ ":$_static_library_apk_java_target" ]
-        }
         if (_proguard_enabled) {
           forward_variables_from(invoker, [ "proguard_jar_path" ])
           deps += _deps + [ ":$_compile_resources_target" ]
@@ -2931,6 +2911,13 @@ if (enable_java_templates) {
         }
 
         if (_is_static_library_provider) {
+          # The list of input jars is already recorded in the .build_config, but
+          # we need to explicitly add the java deps here to ensure they're
+          # available to be used as inputs to the dex step.
+          foreach(_dep, invoker.static_library_dependent_targets) {
+            _target_label = get_label_info(_dep.name, "label_no_toolchain")
+            deps += [ "${_target_label}__java" ]
+          }
           output = _static_library_sync_dex_path
           is_static_library = true
         } else {
@@ -4510,6 +4497,11 @@ if (enable_java_templates) {
     group("${target_name}__java") {
       deps = _sync_module_java_targets
     }
+    group("${target_name}__compile_resources") {
+      deps = [
+        "${invoker.base_module_target}__compile_resources",
+      ]
+    }
 
     _build_config = "$target_gen_dir/${target_name}.build_config"
     _rebased_build_config = rebase_path(_build_config, root_build_dir)
@@ -4716,7 +4708,8 @@ if (enable_java_templates) {
       if (_enable_language_splits) {
         args += [
           "--base-whitelist-rtxt-path=@FileArg(" + "${_rebased_base_module_build_config}:deps_info:base_whitelist_rtxt_path)",
-          "--base-module-rtxt-path=@FileArg(" + "${_rebased_base_module_build_config}:deps_info:module_rtxt_path)",
+          "--base-module-rtxt-path=@FileArg(" +
+              "${_rebased_base_module_build_config}:deps_info:r_text_path)",
         ]
       }
 
@@ -4726,7 +4719,7 @@ if (enable_java_templates) {
           "--uncompressed-assets=@FileArg(" +
               "$_rebased_build_config:uncompressed_assets)",
           "--rtxt-in-paths=@FileArg(" +
-              "$_rebased_build_config:deps_info:module_rtxt_path)",
+              "$_rebased_build_config:deps_info:r_text_path)",
           "--pathmap-in-paths=@FileArg(" +
               "$_rebased_build_config:deps_info:module_pathmap_path)",
         ]

chrome/android/BUILD.gn

@@ -1756,6 +1756,8 @@ if (public_android_sdk) {
     android_manifest_dep = ":trichrome_library_android_manifest"
 
     if (trichrome_synchronized_proguard) {
+      shared_resources_whitelist_target = "//android_webview:system_webview_apk"
+      shared_resources_whitelist_locales = locales
       static_library_dependent_targets = [
         {
           name = "//android_webview:trichrome_webview_apk"
@@ -1777,6 +1779,8 @@ if (public_android_sdk) {
     android_manifest_dep = ":trichrome_library_android_manifest"
 
     if (trichrome_synchronized_proguard) {
+      shared_resources_whitelist_target = "//android_webview:system_webview_apk"
+      shared_resources_whitelist_locales = locales
       static_library_dependent_targets = [
         {
           name = "//android_webview:trichrome_webview_for_bundle_apk"

chrome/android/trichrome.gni

@@ -49,6 +49,8 @@ template("trichrome_library_apk_tmpl") {
                              "apk_name",
                              "min_sdk_version",
                              "proguard_jar_path",
+                             "shared_resources_whitelist_target",
+                             "shared_resources_whitelist_locales",
                              "static_library_dependent_targets",
                              "target_sdk_version",
                            ])
@@ -59,8 +61,12 @@ template("trichrome_library_apk_tmpl") {
     no_build_hooks = true
 
     alternative_android_sdk_dep = webview_framework_dep
-    app_as_shared_lib = true
-    r_java_root_package_name = "trichrome_lib"
+    if (!trichrome_synchronized_proguard) {
+      # TODO(crbug.com/901465): Remove r_java_root_package_name once shared
+      # Java code is moved to the shared library even in debug.
+      r_java_root_package_name = "trichrome_lib"
+      app_as_shared_lib = true
+    }
     use_chromium_linker = false
     uncompress_shared_libraries = true
     uncompress_dex = use_uncompressed_dex