[DFM] Add ModuleDescriptor.getLoadNativeOnGetImpl() to DFM natiive loading.

Previously DFM load (for native libraries / resources) takes place when
a DFM's Module.getImpl() is called. However, VR is an exception; its
Module.getImpl() is called early, before main library load (which is
needed by DFM load). A worked around was to defer loading at first, and
flush then in Module.doDeferredNativeRegistrations().

This CL cleans up the above by giving more control to each DFM to
specify whether to automatically load DFMs in Module.getImpl(). A DFM can
override the default (true) behavior to manage its own native loading,
which may take place on start-up, on first use, and/or on install.

This CL also makes the following changes to specific DFMs:
* VR: Disable load-on-getImpl, with explicit load added to
  VrShellDelegate.onNativeLibraryAvailable(), which gets called on
  start-up and on install.
* DevUI: Disables load-on-getImpl since Module.getImpl() doesn't get
  used anyway. Keep existing calls for load-on-use and load-on-install.

Other implementation details:
* Remove Module.doDeferredNativeRegistrations() and it list of deferred
  DFM loads.
* Add flag |load_native_on_get_impl| to be given in DFM target
  *_module_desc. This is compulsory iff DFM specifies native libraries
  or resources.
* Replace Module.loadNative() with Module.ensureNativeLoaded(), which
  is specific to a module (i.e., no longer static). The name emphasizes
  that repeated calls are simply ignored. It assumes that the DFM
  installed, and requires Chrome's native library to be loaded.
* Replace static map |Module.sInitializedModules| by per-module flag
  |mIsLoaded|.

Bug: 1048740
Change-Id: I2fa844239fc3d741d91d28f384b2bc75a712b574
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2044489
Commit-Queue: Samuel Huang <huangs@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743321}

chrome/android/BUILD.gn

@@ -2072,6 +2072,7 @@ if (enable_vr) {
   # This is necessary since vr tests simulate a bundle build.
   module_desc_java("vr_test_module_desc_java") {
     module_name = "vr"
+    load_native_on_get_impl = false
   }
 
   android_apk("vr_nfc_simulator_apk") {

chrome/android/features/dev_ui/dev_ui_module.gni

@@ -15,4 +15,8 @@ dev_ui_module_desc = {
       "//chrome/android/features/dev_ui/internal/java/AndroidManifest.xml"
   paks = [ "$root_gen_dir/chrome/dev_ui_resources.pak" ]
   pak_deps = [ "//chrome/browser/resources:dev_ui_paks" ]
+
+  # DevUI DFM does not need to call Module.getImpl(), and manages loading on
+  # install and on use. Therefore disable auto-load on Module.getImpl().
+  load_native_on_get_impl = false
 }

chrome/android/features/vr/java/src/org/chromium/chrome/browser/vr/VrShellDelegate.java

@@ -336,6 +336,7 @@ public class VrShellDelegate
      * Called when the native library is first available.
      */
     public static void onNativeLibraryAvailable() {
+        VrModule.ensureNativeLoaded();
         VrModuleProvider.registerJni();
         VrShellDelegateJni.get().onLibraryAvailable();
     }

chrome/android/features/vr/vr_module.gni

@@ -13,4 +13,9 @@ vr_module_desc = {
   android_manifest = "//chrome/android/features/vr/java/AndroidManifest.xml"
   native_deps = [ "//chrome/browser/vr:vr_ui" ]
   native_entrypoints = "//chrome/browser/vr/module_exports.lst"
+
+  # For VR, Module.getImpl() gets called before native library loads (required
+  # by native library load). Therefore disable auto-load on Module.getImpl();
+  # the VR DFM manages its own loading on start-up and on install.
+  load_native_on_get_impl = false
 }

chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java

@@ -164,7 +164,6 @@ import org.chromium.components.browser_ui.widget.MenuOrKeyboardActionController;
 import org.chromium.components.browser_ui.widget.textbubble.TextBubble;
 import org.chromium.components.feature_engagement.EventConstants;
 import org.chromium.components.feature_engagement.Tracker;
-import org.chromium.components.module_installer.builder.Module;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.SelectionPopupController;
 import org.chromium.content_public.browser.WebContents;
@@ -733,11 +732,6 @@ public abstract class ChromeActivity<C extends ChromeActivityComponent>
         IntentHandler.setTestIntentsEnabled(
                 CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TEST_INTENTS));
         mIntentHandler = new IntentHandler(createIntentHandlerDelegate(), getPackageName());
-
-        // This also ensures that subsequent native library and resource loading takes place
-        // immediately, needed by restored tabs that use DFM.
-        // TODO(https://crbug.com/1008162): Have the Module system register its own observer.
-        Module.doDeferredNativeRegistrations();
     }
 
     @Override

chrome/android/modules/chrome_bundle_tmpl.gni

@@ -23,6 +23,15 @@ template("chrome_bundle") {
   _extra_modules = []
   foreach(_module_desc, invoker.module_descs) {
     assert(_package_id > 2, "Too many modules, ran out of package IDs!")
+
+    # Assert that |load_native_on_get_impl| is defined iff native libraries or
+    # resources are defined.
+    if (defined(_module_desc.native_deps) || defined(_module_desc.paks)) {
+      assert(defined(_module_desc.load_native_on_get_impl))
+    } else {
+      assert(!defined(_module_desc.load_native_on_get_impl))
+    }
+
     chrome_feature_module(
         "${_bundle_target_name}__${_module_desc.name}_bundle_module") {
       forward_variables_from(invoker,

chrome/android/modules/chrome_feature_module_tmpl.gni

@@ -78,6 +78,9 @@ template("chrome_feature_module") {
     if (defined(_module_desc.pak_deps)) {
       paks = _module_desc.paks
     }
+    if (defined(_module_desc.load_native_on_get_impl)) {
+      load_native_on_get_impl = _module_desc.load_native_on_get_impl
+    }
   }
 
   android_app_bundle_module(target_name) {

chrome/android/modules/dev_ui/provider/dev_ui_module_provider.cc

@@ -50,8 +50,9 @@ void DevUiModuleProvider::InstallModule(
                                          listener->j_listener());
 }
 
-void DevUiModuleProvider::LoadModule() {
-  Java_DevUiModuleProvider_loadModule(base::android::AttachCurrentThread());
+void DevUiModuleProvider::EnsureLoaded() {
+  Java_DevUiModuleProvider_ensureNativeLoaded(
+      base::android::AttachCurrentThread());
 }
 
 DevUiModuleProvider::DevUiModuleProvider() = default;

chrome/android/modules/dev_ui/provider/dev_ui_module_provider.h

@@ -28,7 +28,7 @@ class DevUiModuleProvider {
 
   // Assuming that the DevUI module is installed, loads DevUI resources if not
   // already loaded. Virtual to enable testing.
-  virtual void LoadModule();
+  virtual void EnsureLoaded();
 
  protected:
   DevUiModuleProvider();

chrome/android/modules/dev_ui/provider/java/src/org/chromium/chrome/modules/dev_ui/DevUiModuleProvider.java

@@ -20,8 +20,7 @@ public class DevUiModuleProvider {
     }
 
     @CalledByNative
-    private static void loadModule() {
-        // Native resource are loaded as side effect of first getImpl() call.
-        DevUiModule.getImpl();
+    private static void ensureNativeLoaded() {
+        DevUiModule.ensureNativeLoaded();
     }
 }

chrome/android/modules/stack_unwinder/stack_unwinder_module.gni

@@ -9,4 +9,5 @@ stack_unwinder_module_desc = {
   android_manifest = "//chrome/android/modules/stack_unwinder/internal/java/AndroidManifest.xml"
   java_deps = [ "//chrome/android/modules/stack_unwinder/internal:java" ]
   native_deps = [ "//chrome/android/modules/stack_unwinder/internal:native" ]
+  load_native_on_get_impl = false
 }

chrome/android/modules/test_dummy/test_dummy_module.gni

@@ -19,4 +19,5 @@ test_dummy_module_desc = {
   paks = [ "$root_gen_dir/chrome/test_dummy_resources.pak" ]
   pak_deps =
       [ "//chrome/android/features/test_dummy/internal:resources_native" ]
+  load_native_on_get_impl = true
 }

chrome/browser/dev_ui/android/dev_ui_loader_throttle.cc

@@ -98,10 +98,8 @@ DevUiLoaderThrottle::MaybeCreateThrottleFor(content::NavigationHandle* handle) {
   if (!ShouldInstallDevUiDfm(handle->GetURL()))
     return nullptr;
 
-  // If module is already installed, ensure that it is loaded.
   if (dev_ui::DevUiModuleProvider::GetInstance()->ModuleInstalled()) {
-    // Synchronously load module (if not already loaded).
-    dev_ui::DevUiModuleProvider::GetInstance()->LoadModule();
+    dev_ui::DevUiModuleProvider::GetInstance()->EnsureLoaded();
     return nullptr;
   }
 
@@ -132,7 +130,7 @@ DevUiLoaderThrottle::WillStartRequest() {
 
 void DevUiLoaderThrottle::OnDevUiDfmInstallWithStatus(bool success) {
   if (success) {
-    dev_ui::DevUiModuleProvider::GetInstance()->LoadModule();
+    dev_ui::DevUiModuleProvider::GetInstance()->EnsureLoaded();
     Resume();
   } else {
     std::string html = BuildErrorPageHtml();

chrome/browser/dev_ui/android/dev_ui_loader_throttle_unittest.cc

@@ -50,7 +50,7 @@ class MockDevUiModuleProvider : public DevUiModuleProvider {
   }
 
   // DevUiModuleProvider:
-  void LoadModule() override { is_loaded_ = true; }
+  void EnsureLoaded() override { is_loaded_ = true; }
 
   void Reset() {
     is_installed_ = false;

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

@@ -10,14 +10,11 @@ import org.chromium.base.BundleUtils;
 import org.chromium.base.StrictModeContext;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeMethods;
+import org.chromium.base.library_loader.LibraryLoader;
 import org.chromium.components.module_installer.engine.InstallEngine;
 import org.chromium.components.module_installer.engine.InstallListener;
 import org.chromium.components.module_installer.util.Timer;
 
-import java.util.HashSet;
-import java.util.Set;
-import java.util.TreeSet;
-
 /**
  * Represents a feature module. Can be used to install the module, access its interface, etc. See
  * {@link ModuleInterface} for how to conveniently create an instance of the module class for a
@@ -27,27 +24,12 @@ import java.util.TreeSet;
  */
 @JNINamespace("module_installer")
 public class Module<T> {
-    private static final Set<String> sInitializedModules = new HashSet<>();
-    private static Set<String> sPendingNativeRegistrations = new TreeSet<>();
-
     private final String mName;
     private final Class<T> mInterfaceClass;
     private final String mImplClassName;
     private T mImpl;
     private InstallEngine mInstaller;
-
-    /**
-     * To be called after the main native library has been loaded. Any module instances created
-     * before the native library is loaded have their native component queued for loading and
-     * registration. Calling this methed completes that process.
-     */
-    public static void doDeferredNativeRegistrations() {
-        if (sPendingNativeRegistrations == null) return;
-        for (String name : sPendingNativeRegistrations) {
-            loadNative(name);
-        }
-        sPendingNativeRegistrations = null;
-    }
+    private boolean mIsNativeLoaded;
 
     /**
      * Instantiates a module.
@@ -113,33 +95,35 @@ public class Module<T> {
     public T getImpl() {
         try (Timer timer = new Timer()) {
             if (mImpl != null) return mImpl;
-
             assert isInstalled();
-            // Load the module's native code and/or resources if they are present, and the Chrome
-            // native library itself has been loaded.
-            if (sPendingNativeRegistrations == null) {
-                loadNative(mName);
-            } else {
-                // We have to defer native initialization because VR is calling getImpl in early
-                // startup. As soon as VR stops doing that we want to deprecate deferred native
-                // initialization.
-                sPendingNativeRegistrations.add(mName);
+
+            ModuleDescriptor moduleDescriptor = loadModuleDescriptor(mName);
+            if (moduleDescriptor.getLoadNativeOnGetImpl()) {
+                // Load the module's native code and/or resources if they are present, and the
+                // Chrome native library itself has been loaded.
+                ensureNativeLoaded();
             }
+
             mImpl = mInterfaceClass.cast(instantiateReflectively(mImplClassName));
             return mImpl;
         }
     }
 
-    private static void loadNative(String name) {
+    /**
+     * Loads native libraries and/or resources if and only if this is not already done, assuming
+     * that the module is installed, and enableNativeLoad() has been called.
+     */
+    public void ensureNativeLoaded() {
         // Can only initialize native once per lifetime of Chrome.
-        if (sInitializedModules.contains(name)) return;
-        ModuleDescriptor moduleDescriptor = loadModuleDescriptor(name);
+        if (mIsNativeLoaded) return;
+        assert LibraryLoader.getInstance().isInitialized();
+        ModuleDescriptor moduleDescriptor = loadModuleDescriptor(mName);
         String[] libraries = moduleDescriptor.getLibraries();
         String[] paks = moduleDescriptor.getPaks();
         if (libraries.length > 0 || paks.length > 0) {
-            ModuleJni.get().loadNative(name, libraries, paks);
+            ModuleJni.get().loadNative(mName, libraries, paks);
         }
-        sInitializedModules.add(name);
+        mIsNativeLoaded = true;
     }
 
     /**
@@ -164,6 +148,11 @@ public class Module<T> {
                 public String[] getPaks() {
                     return new String[0];
                 }
+
+                @Override
+                public boolean getLoadNativeOnGetImpl() {
+                    return false;
+                }
             };
         }
 

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

@@ -16,4 +16,8 @@ public interface ModuleDescriptor {
      * Returns the list of PAK resources files this module contains.
      */
     String[] getPaks();
+    /**
+     * Returns whether to auto-load native libraries / resources on getImpl().
+     */
+    boolean getLoadNativeOnGetImpl();
 }

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

@@ -107,6 +107,12 @@ public class ModuleInterfaceProcessor extends AbstractProcessor {
                                              .addStatement("sModule.installDeferred()")
                                              .build();
 
+        MethodSpec ensureNativeLoaded = MethodSpec.methodBuilder("ensureNativeLoaded")
+                                                .returns(TypeName.VOID)
+                                                .addModifiers(Modifier.PUBLIC, Modifier.STATIC)
+                                                .addStatement("sModule.ensureNativeLoaded()")
+                                                .build();
+
         MethodSpec getImpl = MethodSpec.methodBuilder("getImpl")
                                      .returns(interfaceClassName)
                                      .addModifiers(Modifier.PUBLIC, Modifier.STATIC)
@@ -136,6 +142,7 @@ public class ModuleInterfaceProcessor extends AbstractProcessor {
                 .addMethod(isInstalled)
                 .addMethod(install)
                 .addMethod(installDeferred)
+                .addMethod(ensureNativeLoaded)
                 .addMethod(getImpl)
                 .addMethod(getInstallEngine)
                 .addMethod(setInstallEngine)

components/module_installer/android/module_desc_java.gni

@@ -18,6 +18,8 @@ import("//build/config/python.gni")
 #     requires at runtime. Will consider all transitively depended on
 #     shared_libraries.
 #   paks: (Optional) PAK files going into the module.
+#   load_native_on_get_impl: (Optional) Whether the module's native libraries /
+#     resources are automatically loaded when Module.getImpl() is called.
 template("module_desc_java") {
   _target_name = target_name
 
@@ -48,6 +50,12 @@ template("module_desc_java") {
       "--output",
       rebase_path(_srcjar, root_out_dir),
     ]
+
+    if (defined(invoker.load_native_on_get_impl) &&
+        invoker.load_native_on_get_impl) {
+      args += [ "--load-native-on-get-impl" ]
+    }
+
     if (defined(invoker.paks)) {
       _rebased_paks = []
       foreach(_pak, invoker.paks) {

components/module_installer/android/module_desc_java.py

@@ -38,6 +38,11 @@ public class ModuleDescriptor_{MODULE} implements ModuleDescriptor {{
     public String[] getPaks() {{
         return PAKS;
     }}
+
+    @Override
+    public boolean getLoadNativeOnGetImpl() {{
+        return {LOAD_NATIVE_ON_GET_IMPL};
+    }}
 }}
 """
 
@@ -50,6 +55,9 @@ def main():
   parser.add_argument('--paks', help='GN list of PAK file paths')
   parser.add_argument(
       '--output', required=True, help='Path to the generated srcjar file.')
+  parser.add_argument('--load-native-on-get-impl', action='store_true',
+      default=False,
+      help='Load module automatically on calling Module.getImpl().')
   options = parser.parse_args(build_utils.ExpandFileArgs(sys.argv[1:]))
   options.libraries = build_utils.ParseGnList(options.libraries)
   options.paks = build_utils.ParseGnList(options.paks)
@@ -68,6 +76,8 @@ def main():
       'MODULE': options.module,
       'LIBRARIES': ','.join(['"%s"' % l for l in libraries]),
       'PAKS': ','.join(['"%s"' % os.path.basename(p) for p in paks]),
+      'LOAD_NATIVE_ON_GET_IMPL': (
+          'true' if options.load_native_on_get_impl else 'false'),
   }
   with build_utils.AtomicOutput(options.output) as f:
     with zipfile.ZipFile(f.name, 'w') as srcjar_file: