Add lib placeholders when packaging modules to make bundle multi ABI

This fixes a bug where modules can miss an ABI and fail the bundle
packaging step. This is due to two factors:

1. crrev.com/c/1876975 added an allotment step to each bundle that
   determined in which module native libs are packaged into. If a module
   only depends on libs that are also depended on by other modules this
   module will package no libs even though libs are specified when
   instantiating the module target.

2. Bundletool requires that all modules support the same set of ABIs.
   To make modules appear to support the WebView ABI for Monochrome and
   Trichrome we add placeholder libs for the WebView ABI. We do this in
   the chrome_feature_module template if we specify real libs for the
   browser ABI.

Given 1. and 2. it can happen that a module is only left with the
placeholder lib and appear single ABI, failing packaging. This CL fixes
that by telling the packaging step that the module should be multi ABI
and the packaging step will add a placeholder lib if necessary. This
makes placeholder logic in chrome_feature_module obsolete and therefore
it is removed.

This new multi ABI logic can likely be reused for other APKs that use
lib placeholders simply to make the APK multi ABI.

Bug: 870055
Change-Id: I25cf4edd56bf253cff2aef54d1857cbe9167cae3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881814
Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710204}

build/android/gyp/apkbuilder.py

@@ -73,6 +73,12 @@ def _ParseArgs(args):
   parser.add_argument('--secondary-android-abi',
                       help='The secondary Android architecture to use for'
                            'secondary native libraries')
+  parser.add_argument(
+      '--is-multi-abi',
+      action='store_true',
+      help='Will add a placeholder for the missing ABI if no native libs or '
+      'placeholders are set for either the primary or secondary ABI. Can only '
+      'be set if both --android-abi and --secondary-android-abi are set.')
   parser.add_argument(
       '--native-lib-placeholders',
       help='GYP-list of native library placeholders to add.')
@@ -132,6 +138,10 @@ def _ParseArgs(args):
       options.secondary_native_lib_placeholders):
     raise Exception('Must specify --secondary-android-abi with'
                     ' --secondary-native-libs')
+  if options.is_multi_abi and not (options.android_abi
+                                   and options.secondary_android_abi):
+    raise Exception('Must specify --is-multi-abi with both --android-abi '
+                    'and --secondary-android-abi.')
   return options
 
 
@@ -334,14 +344,29 @@ def main(args):
                             options.secondary_android_abi,
                             options.uncompress_shared_libraries)
 
-      for name in sorted(options.native_lib_placeholders):
+      # Add a placeholder lib if the APK should be multi ABI but is missing libs
+      # for one of the ABIs.
+      native_lib_placeholders = options.native_lib_placeholders
+      secondary_native_lib_placeholders = (
+          options.secondary_native_lib_placeholders)
+      if options.is_multi_abi:
+        if ((secondary_native_libs or secondary_native_lib_placeholders)
+            and not native_libs and not native_lib_placeholders):
+          native_lib_placeholders += ['libplaceholder.so']
+        if ((native_libs or native_lib_placeholders)
+            and not secondary_native_libs
+            and not secondary_native_lib_placeholders):
+          secondary_native_lib_placeholders += ['libplaceholder.so']
+
+      # Add placeholder libs.
+      for name in sorted(native_lib_placeholders):
         # Note: Empty libs files are ignored by md5check (can cause issues
         # with stale builds when the only change is adding/removing
         # placeholders).
         apk_path = 'lib/%s/%s' % (options.android_abi, name)
         build_utils.AddToZipHermetic(out_apk, apk_path, data='')
 
-      for name in sorted(options.secondary_native_lib_placeholders):
+      for name in sorted(secondary_native_lib_placeholders):
         # Note: Empty libs files are ignored by md5check (can cause issues
         # with stale builds when the only change is adding/removing
         # placeholders).

build/config/android/internal_rules.gni

@@ -3634,6 +3634,8 @@ if (enable_java_templates) {
 #   module_name: The module's name.
 #   native_libraries_config: Path to file listing native libraries to be
 #     packaged into each module.
+#   is_multi_abi: If true will add a library placeholder for the missing ABI if
+#     either the primary or the secondary ABI has no native libraries set.
 template("create_android_app_bundle_module") {
   _rebased_build_config = rebase_path(invoker.build_config, root_build_dir)
   _rebased_native_libraries_config =
@@ -3695,6 +3697,9 @@ template("create_android_app_bundle_module") {
         "--secondary-android-abi=$android_app_secondary_abi",
       ]
     }
+    if (defined(invoker.is_multi_abi) && invoker.is_multi_abi) {
+      args += [ "--is-multi-abi" ]
+    }
 
     # Use either provided dex path or build config path based on type of module.
     if (defined(invoker.dex_path)) {

build/config/android/rules.gni

@@ -4345,6 +4345,9 @@ if (enable_java_templates) {
   #    verify_proguard_flags: Enables verification of expected merged proguard
   #      flags based on a golden file.
   #
+  #    is_multi_abi: If true will add a library placeholder for the missing ABI
+  #      if either the primary or the secondary ABI has no native libraries set.
+  #
   # Example:
   #   android_app_bundle("chrome_public_bundle") {
   #      base_module_target = "//chrome/android:chrome_public_apk"
@@ -4588,6 +4591,7 @@ if (enable_java_templates) {
       _module_zip_path = "$target_gen_dir/$target_name/${_module.name}.zip"
 
       create_android_app_bundle_module(_create_module_target) {
+        forward_variables_from(invoker, [ "is_multi_abi" ])
         module_name = _module.name
         build_config = _module_build_config
         module_zip_path = _module_zip_path

chrome/android/chrome_bundle_tmpl.gni

@@ -9,11 +9,14 @@ import("//chrome/android/modules/chrome_feature_module_tmpl.gni")
 
 # Instantiates a Chrome-specific app bundle.
 #
-# Supports most attributes of chrome_feature_module and android_app_bundle,
-# plus:
+# Supports most variables of chrome_feature_module and android_app_bundle, plus:
 #   module_descs: List of descriptors for modules that are part of this bundle.
 #     See //chrome/android/modules/chrome_feature_modules.gni for the format of
 #     a module descriptor.
+#   is_64_bit_browser: (Optional) Whether Chrome (as opposed to WebView) runs in
+#     64 bit.
+#   include_32_bit_webview: (Optional) Whether to include 32 bit code for
+#     WebView.
 template("chrome_bundle") {
   _bundle_target_name = target_name
   _package_id = 126  # == 0x7e.
@@ -25,7 +28,6 @@ template("chrome_bundle") {
       forward_variables_from(invoker,
                              [
                                "base_module_target",
-                               "include_32_bit_webview",
                                "is_64_bit_browser",
                                "is_monochrome_or_trichrome",
                                "manifest_package",
@@ -47,6 +49,24 @@ template("chrome_bundle") {
     _package_id -= 1
   }
 
+  # Determine whether the bundle has native libraries for both the primary and
+  # the secondary ABI. This is the case if we package WebView with the
+  # complementary ABI of the browser.
+  _is_64_bit_browser =
+      defined(invoker.is_64_bit_browser) && invoker.is_64_bit_browser
+  _include_32_bit_webview =
+      defined(invoker.include_32_bit_webview) && invoker.include_32_bit_webview
+  _is_multi_abi = false
+  if (_is_64_bit_browser) {
+    _is_multi_abi = _include_32_bit_webview
+  } else {
+    assert(!_include_32_bit_webview)
+
+    # WebView is included with 64bit ABI if |android_64bit_target_cpu| is true.
+    _is_multi_abi = android_64bit_target_cpu
+  }
+  assert(!_is_multi_abi || build_apk_secondary_abi)
+
   android_app_bundle(target_name) {
     forward_variables_from(invoker,
                            [
@@ -69,6 +89,7 @@ template("chrome_bundle") {
     enable_language_splits = true
     extra_modules = _extra_modules
     system_image_locale_whitelist = locales - android_chrome_omitted_locales
+    is_multi_abi = _is_multi_abi
 
     # NOTE: Only sign bundle for official builds since this is very slow.
     if (enable_chrome_android_internal && use_signing_keys &&

chrome/android/modules/chrome_feature_module_tmpl.gni

@@ -9,15 +9,13 @@ import("//components/module_installer/android/module_desc_java.gni")
 
 # Instantiates a Chrome-specific app bundle module.
 #
-# Supports most attributes of the android_app_bundle_module target, plus:
+# Supports most variables of the android_app_bundle_module, plus:
 #   module_desc: Descriptor of this module. See
 #     //chrome/android/modules/chrome_feature_modules.gni for the format.
 #   is_monochrome_or_trichrome: (Optional) Whether this module is packaged into
 #     Monochrome or Trichrome.
 #   is_64_bit_browser: (Optional) Whether Chrome (as opposed to WebView) runs in
 #     64 bit.
-#   include_32_bit_webview: (Optional) Whether to include 32 bit code for
-#     WebView.
 template("chrome_feature_module") {
   assert(defined(invoker.module_desc))
   _module_desc = invoker.module_desc
@@ -25,8 +23,6 @@ template("chrome_feature_module") {
                                 invoker.is_monochrome_or_trichrome
   _is_64_bit_browser =
       defined(invoker.is_64_bit_browser) && invoker.is_64_bit_browser
-  _include_32_bit_webview =
-      defined(invoker.include_32_bit_webview) && invoker.include_32_bit_webview
 
   _loadable_modules_32_bit = []
   if (defined(_module_desc.loadable_modules_32_bit)) {
@@ -92,56 +88,22 @@ template("chrome_feature_module") {
     proguard_enabled = !is_java_debug
     package_name = _module_desc.name
 
-    # Specify native libraries and placeholders.
-    if (_loadable_modules_32_bit != [] || _loadable_modules_64_bit != [] ||
-        _shared_libraries != []) {
-      # Decision logic: Assign decision variables:
-      #   _loadable_modules_to_use: Either |_loadable_modules_64_bit| or
-      #     |_loadable_modules_32_bit|.
-      #   _native_is_primary: Whether |_loadable_modules_to_use| and/or
-      #     |_shared_libraries| should be assigned as primary ABI or
-      #     secondary ABI.
-      #   _native_need_placeholder: Whether a placeholder is needed for the
-      #     complementary ABI to the library.
-      if (_is_64_bit_browser) {
-        not_needed([ "_loadable_modules_32_bit" ])
-        _loadable_modules_to_use = _loadable_modules_64_bit
-        _native_is_primary =
-            !build_apk_secondary_abi || android_64bit_target_cpu
-        _native_need_placeholder =
-            build_apk_secondary_abi && _include_32_bit_webview
-        not_needed([ "_include_32_bit_webview" ])
+    # Determine whether to assign native libraries to the primary or secondary
+    # ABI.
+    if (_is_64_bit_browser) {
+      not_needed([ "_loadable_modules_32_bit" ])
+      assert(android_64bit_target_cpu)
+      loadable_modules = _loadable_modules_64_bit
+      shared_libraries = _shared_libraries
+    } else {
+      not_needed([ "_loadable_modules_64_bit" ])
+      if (android_64bit_target_cpu) {
+        secondary_abi_loadable_modules = _loadable_modules_32_bit
+        secondary_abi_shared_libraries = _shared_libraries
       } else {
-        not_needed([
-                     "_loadable_modules_64_bit",
-                     "_include_32_bit_webview",
-                   ])
-        _loadable_modules_to_use = _loadable_modules_32_bit
-        _native_is_primary =
-            !build_apk_secondary_abi || !android_64bit_target_cpu
-        _native_need_placeholder =
-            build_apk_secondary_abi && android_64bit_target_cpu
-      }
-
-      # Realization logic: Assign libraries and placeholders.
-      if (_native_is_primary) {
-        loadable_modules = _loadable_modules_to_use
+        loadable_modules = _loadable_modules_32_bit
         shared_libraries = _shared_libraries
-        if (_native_need_placeholder) {
-          secondary_native_lib_placeholders = [ "libdummy.so" ]
-        }
-      } else {
-        secondary_abi_loadable_modules = _loadable_modules_to_use
-        secondary_abi_shared_libraries = _shared_libraries
-        if (_native_need_placeholder) {
-          native_lib_placeholders = [ "libdummy.so" ]
-        }
       }
-    } else {
-      not_needed([
-                   "_is_64_bit_browser",
-                   "_include_32_bit_webview",
-                 ])
     }
   }
 }