Fixes for moving chrome to isolated split

This patch has a few fixes needed before enabling isolated splits in
canary:
- Make module_installer load classes from the application context
  ClassLoader instead of using Class.forName, since that may be using
  the base ClassLoader and not have the chrome split classes
- Fix some of the bundle smoke tests since the DFMs are merged into the
  chrome DFM.
- Fix the chrome resources package (org.chromium.chrome.R) clashing with
  the base module's resources when the APK package name matches.
- Ignore chime services which are not defined in the base module, since
  they will not be used with isolated splits.

Bug: 1126301
Change-Id: Ib149894636863408c5fd37054eacb48c214b5b3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2472707Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818724}

build/android/gyp/create_app_bundle.py

@@ -438,9 +438,23 @@ def _MaybeCheckServicesPresentInBase(bundle_path, module_zips):
       classes.update('%s.%s' % (name, c)
                      for c in package_dict['classes'].keys())
 
+  ignored_service_names = {
+      # Defined in the chime DFM manifest, but unused.
+      # org.chromium.chrome.browser.chime.ScheduledTaskService is used instead.
+      ("com.google.android.libraries.notifications.entrypoints.scheduled."
+       "ScheduledTaskService"),
+
+      # Defined in the chime DFM manifest, only used pre-O (where isolated
+      # splits are not supported).
+      ("com.google.android.libraries.notifications.executor.impl.basic."
+       "ChimeExecutorApiService"),
+  }
+
   # Ensure all services are present in base module.
   for service_name in service_names:
     if service_name not in classes:
+      if service_name in ignored_service_names:
+        continue
       raise Exception("Service %s should be present in the base module's dex."
                       " See b/169196314 for more details." % service_name)
 

build/config/android/rules.gni

@@ -2769,6 +2769,7 @@ if (enable_java_templates) {
                                "chromium_code",
                                "enable_jetify",
                                "jacoco_never_instrument",
+                               "jar_excluded_patterns",
                                "javac_args",
                                "native_lib_placeholders",
                                "processor_args_javac",
@@ -2781,10 +2782,14 @@ if (enable_java_templates) {
                              ])
       deps = _deps
       if (_uses_static_library_synchronized_proguard) {
+        if (!defined(jar_excluded_patterns)) {
+          jar_excluded_patterns = []
+        }
+
         # The static library will provide all R.java files, but we still need to
         # make the base module R.java files available at compile time since DFM
         # R.java classes extend base module classes.
-        jar_excluded_patterns = [
+        jar_excluded_patterns += [
           "*/R.class",
           "*/R\$*.class",
         ]
@@ -3615,6 +3620,7 @@ if (enable_java_templates) {
                                "input_jars_paths",
                                "is_base_module",
                                "jacoco_never_instrument",
+                               "jar_excluded_patterns",
                                "javac_args",
                                "jni_registration_header",
                                "jni_sources_exclusions",

chrome/android/BUILD.gn

@@ -43,10 +43,6 @@ if (android_channel != "default") {
   # name will cause the comparison to output many unnecessary differences.
   # See https://source.chromium.org/chromium/chromium/src/+/master:chrome/android/java/README.md
   _default_package += "." + android_channel
-} else if (enable_chrome_module) {
-  # TODO(crbug.com/1126301): There are issues with the APK package name matching
-  # the resource package when //chrome resources are in a DFM.
-  _default_package += ".module"
 }
 
 declare_args() {
@@ -2720,7 +2716,9 @@ instrumentation_test_runner("monochrome_public_bundle_smoke_test") {
   android_test_apk = ":chrome_bundle_smoke_test_apk"
   android_test_apk_name = "ChromeBundleSmokeTest"
   never_incremental = true
-  modules = [ "test_dummy" ]
+  if (!enable_chrome_module) {
+    modules = [ "test_dummy" ]
+  }
   extra_args = _bundle_smoke_test_extra_args
 }
 
@@ -2730,7 +2728,13 @@ instrumentation_test_runner(
   android_test_apk = ":chrome_bundle_smoke_test_apk"
   android_test_apk_name = "ChromeBundleSmokeTest"
   never_incremental = true
-  fake_modules = [ "test_dummy" ]
+
+  # TODO(crbug.com/1126301): This is identical to
+  # monochrome_public_bundle_smoke_test if enable_chrome_module is enabled, so
+  # should be removed if enable_chrome_module is ever enabled by default.
+  if (!enable_chrome_module) {
+    fake_modules = [ "test_dummy" ]
+  }
   extra_args =
       _bundle_smoke_test_extra_args + _bundle_fake_modules_smoke_test_extra_args
 }
@@ -2757,7 +2761,9 @@ instrumentation_test_runner("trichrome_chrome_bundle_smoke_test") {
   android_test_apk = ":chrome_bundle_smoke_test_apk"
   android_test_apk_name = "ChromeBundleSmokeTest"
   never_incremental = true
-  modules = [ "test_dummy" ]
+  if (!enable_chrome_module) {
+    modules = [ "test_dummy" ]
+  }
   additional_apks = [ "//chrome/android:trichrome_library_apk" ]
   extra_args = _bundle_smoke_test_extra_args
 }
@@ -2767,7 +2773,13 @@ instrumentation_test_runner("trichrome_chrome_bundle_fake_modules_smoke_test") {
   android_test_apk = ":chrome_bundle_smoke_test_apk"
   android_test_apk_name = "ChromeBundleSmokeTest"
   never_incremental = true
-  fake_modules = [ "test_dummy" ]
+
+  # TODO(crbug.com/1126301): This is identical to
+  # trichrome_chrome_bundle_smoke_test if enable_chrome_module is enabled, so
+  # should be removed if enable_chrome_module is ever enabled by default.
+  if (!enable_chrome_module) {
+    fake_modules = [ "test_dummy" ]
+  }
   additional_apks = [ "//chrome/android:trichrome_library_apk" ]
   extra_args =
       _bundle_smoke_test_extra_args + _bundle_fake_modules_smoke_test_extra_args
@@ -2922,6 +2934,15 @@ template("monochrome_or_trichrome_public_bundle_tmpl") {
       if (!_is_trichrome) {
         chrome_deps += [ "//chrome/android:monochrome_java" ]
       }
+
+      # These will be defined in the base module, and cause conflicts when the
+      # APK package name matches the resource package name from :chrome_java.
+      if (chrome_public_manifest_package == "org.chromium.chrome") {
+        chrome_jar_excluded_patterns = [
+          "org/chromium/chrome/R\$*.class",
+          "org/chromium/chrome/R.class",
+        ]
+      }
     }
 
     if (!is_java_debug) {

chrome/android/modules/chrome_bundle_tmpl.gni

@@ -84,6 +84,9 @@ template("chrome_bundle") {
       if (defined(invoker.chrome_deps)) {
         java_deps += invoker.chrome_deps
       }
+      if (defined(invoker.chrome_jar_excluded_patterns)) {
+        jar_excluded_patterns = invoker.chrome_jar_excluded_patterns
+      }
 
       # Native code for all DFMs is moved to the base module's library, and paks
       # for DFMs will use the value of load_native_on_get_impl from the module

chrome/android/modules/chrome_feature_module_tmpl.gni

@@ -105,6 +105,9 @@ template("chrome_feature_module") {
     if (defined(_module_desc.java_deps)) {
       deps += _module_desc.java_deps
     }
+    if (defined(_module_desc.jar_excluded_patterns)) {
+      jar_excluded_patterns = _module_desc.jar_excluded_patterns
+    }
 
     # Don't embed more translations than required (http://crbug.com/932017).
     aapt_locale_allowlist = locales

components/module_installer/android/java/src/org/chromium/components/module_installer/builder/Module.java

@@ -7,6 +7,7 @@ package org.chromium.components.module_installer.builder;
 import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.BundleUtils;
+import org.chromium.base.ContextUtils;
 import org.chromium.base.StrictModeContext;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.MainDex;
@@ -173,7 +174,10 @@ public class Module<T> {
      */
     private static Object instantiateReflectively(String className) {
         try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
-            return Class.forName(className).newInstance();
+            return ContextUtils.getApplicationContext()
+                    .getClassLoader()
+                    .loadClass(className)
+                    .newInstance();
         } catch (Exception e) {
             throw new RuntimeException(e);
         }

components/module_installer/android/java/src/org/chromium/components/module_installer/builder/ModuleEngine.java

@@ -6,6 +6,7 @@ package org.chromium.components.module_installer.builder;
 
 import android.app.Activity;
 
+import org.chromium.base.ContextUtils;
 import org.chromium.base.StrictModeContext;
 import org.chromium.components.module_installer.engine.EngineFactory;
 import org.chromium.components.module_installer.engine.InstallEngine;
@@ -41,7 +42,7 @@ class ModuleEngine implements InstallEngine {
         // Accessing classes in the module may cause its DEX file to be loaded. And on some
         // devices that causes a read mode violation.
         try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
-            Class.forName(mImplClassName);
+            ContextUtils.getApplicationContext().getClassLoader().loadClass(mImplClassName);
             return true;
         } catch (ClassNotFoundException e) {
             return false;