Modules: Rename partitioned shared libraries to match base library

When feature libraries for DFMs were introduced, the library name
matched the .so name (eg. libvr.so).  This made library loading
easier.  However, it necessitated placing these libraries into a build
subdirectory, so that Monochrome's libvr.so wouldn't collide with
Chrome's libvr.so (with the same applying to the test dummy module, and
any future DFMs).

Since then, we've learned that this puts a tax on other parts of the
system, such as stack decoding tools, when trying to locate such
libraries. Further, the module build framework has been improved to
package and load feature libraries automatically, which means the files
can have arbitrary names. This lets us move to a system where (using VR
as an example), we have libmonochrome.so and libmonochrome_vr.so created
in the conventional build output directory.

VR, for now, still has a factory that loads its own library. Normally,
this wouldn't be an issue, since loading a library twice results in a
no-op the second time. However, we've been warned that because the
soname ("vr") and the library file name don't match, that there could be
issues with Android actually loading the library twice.  This shouldn't
be a problem for VR, but if it is, we could temporarily blacklist VR
from being loaded by the module library, until it's native code is fully
in the VR DFM, and it's custom factory disappears.

Bug: 1015159
Change-Id: Icd20399756d1c4876df922d9c111cda0e67773d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897910Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712708}

base/android/bundle_utils.cc

@@ -51,7 +51,10 @@ std::string ResolveLibraryPath(const std::string& library_name) {
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jstring> java_path = Java_BundleUtils_getNativeLibraryPath(
       env, base::android::ConvertUTF8ToJavaString(env, library_name));
-  DCHECK(java_path);
+  // TODO(https://crbug.com/1019853): Remove this tolerance.
+  if (!java_path) {
+    return std::string();
+  }
   return base::android::ConvertJavaStringToUTF8(env, java_path);
 }
 
@@ -63,10 +66,13 @@ bool BundleUtils::IsBundle() {
 }
 
 // static
-void* BundleUtils::DlOpenModuleLibraryPartition(
-    const std::string& library_name) {
+void* BundleUtils::DlOpenModuleLibraryPartition(const std::string& library_name,
+                                                const std::string& partition) {
+  // TODO(https://crbug.com/1019853): Remove this tolerance.
   std::string library_path = ResolveLibraryPath(library_name);
-  std::string partition = base::FilePath(library_path).BaseName().value();
+  if (library_path.empty()) {
+    return nullptr;
+  }
 
   // Linear search is required here because the partition descriptors are not
   // ordered. If a large number of partitions come into existence, lld could be

base/android/bundle_utils.h

@@ -21,8 +21,14 @@ class BASE_EXPORT BundleUtils {
   // dlopen wrapper that works for partitioned native libraries in dynamic
   // feature modules. This routine looks up the partition's address space in a
   // table of main library symbols, and uses it when loading the feature
-  // library.
-  static void* DlOpenModuleLibraryPartition(const std::string& library_name);
+  // library. It requires |library_name| (eg. chrome_foo) to resolve the file
+  // path (which may be in an interesting location due to SplitCompat) and
+  // |partition_name| to look up the load parameters in the main library. These
+  // two values may be identical, but since the partition name is set at compile
+  // time, and the code is linked into multiple libraries (eg. Chrome vs
+  // Monochrome), they may not be.
+  static void* DlOpenModuleLibraryPartition(const std::string& library_name,
+                                            const std::string& partition);
 };
 
 }  // namespace android

build/partitioned_shared_library.gni

 # Copyright 2019 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.
 
@@ -17,7 +18,7 @@ import("//build/config/clang/clang.gni")
 # segment.  After linking, the segments are split apart using objcopy into
 # separate libraries.  The main library is then packaged with the application
 # as usual, while feature libraries may be packaged, delivered and loaded
-# separately.
+# separately (via an Android Dynamic Feature Module).
 #
 # When loading a feature library, the intended address of the library must be
 # supplied to the loader, so that it can be mapped to the memory location.  The
@@ -27,18 +28,15 @@ import("//build/config/clang/clang.gni")
 # The template instantiates targets for the base library, as well as each
 # specified partition, based on the root target name.  Example:
 #
-#   - monochrome            (base library)
-#   - monochrome_foo        (partition library for feature 'foo')
-#   - monochrome_bar        (partition library for feature 'bar')
-#
-# The base library is placed in the root output directory, but additional
-# feature libraries are placed in a subdirectory named according to the base
-# library.  This avoids name collisions, since feature library names are not
-# sensitive to the base library to which they are paired.  Example:
+#   - libmonochrome            (base library)
+#   - libmonochrome_foo        (partition library for feature 'foo')
+#   - libmonochrome_bar        (partition library for feature 'bar')
 #
-#   - out/libmonochrome.so
-#   - out/monochrome_partitions/libfoo.so
-#   - out/monochrome_partitions/libbar.so
+# Note that the feature library filenames are chosen based on the main
+# library's name (eg. libmonochrome_foo.so), but the soname of the feature
+# library is based on the feature name (eg. "foo").  This should generally be
+# okay, with the caveat that loading the library multiple times *might* cause
+# problems in Android.
 #
 # This template uses shared_library's default configurations.
 #
@@ -101,7 +99,7 @@ template("partitioned_shared_library") {
       if (defined(invoker.partition) && invoker.partition != "") {
         args += [
           "--partition",
-          "lib${invoker.partition}.so",
+          "${invoker.partition}",
         ]
       }
       args += [ rebase_path(sources[0], root_build_dir) ]
@@ -129,10 +127,10 @@ template("partitioned_shared_library") {
   # same directory.
   foreach(_partition, invoker.partitions) {
     partition_action("${target_name}_${_partition}") {
-      partition = _partition
-      _partition_dir = "$root_out_dir/${invoker.target_name}_partitions"
-      unstripped_output = "$_partition_dir/lib.unstripped/lib${partition}.so"
-      stripped_output = "$_partition_dir/lib${partition}.so"
+      partition = "${_partition}_partition"
+      stripped_output = "$root_out_dir/lib${_output_name}_${partition}.so"
+      unstripped_output =
+          "$root_out_dir/lib.unstripped/lib${_output_name}_${partition}.so"
     }
   }
 }

chrome/android/modules/test_dummy/internal/BUILD.gn

@@ -37,7 +37,7 @@ component("test_dummy") {
 
   # Test dummy native entrypoints belong in the partition.
   if (use_native_partitions) {
-    cflags = [ "-fsymbol-partition=libtest_dummy.so" ]
+    cflags = [ "-fsymbol-partition=test_dummy_partition" ]
   }
 }
 

chrome/browser/android/vr/ui_module_factory.cc

@@ -28,8 +28,26 @@ std::unique_ptr<UiInterface> UiModuleFactory::Create(
   // Do not dlclose() the library. Doing so causes issues with cardboard on
   // Android M. It's not clear whether there is a use-after-free in VR code, or
   // a linker or system issue. See https://crbug.com/994029.
-  void* ui_library_handle =
-      base::android::BundleUtils::DlOpenModuleLibraryPartition("vr");
+
+  // TODO(https://crbug.com/1019853): When all VR native code moves into the
+  // feature module, this factory will completely disappear. In the meantime,
+  // make it tolerant of two different variants of the VR lib (one for Chrome,
+  // one for Monochrome).
+  const std::vector<const std::string> library_name_possibilities = {
+      "monochrome_vr",
+      "chrome_vr",
+  };
+
+  void* ui_library_handle = nullptr;
+  const std::string partition_name = "vr";
+  for (const auto& library_name : library_name_possibilities) {
+    ui_library_handle =
+        base::android::BundleUtils::DlOpenModuleLibraryPartition(
+            library_name, partition_name);
+    if (ui_library_handle != nullptr) {
+      break;
+    }
+  }
   DCHECK(ui_library_handle != nullptr)
       << "Could not open VR UI library:" << dlerror();
 

chrome/browser/vr/BUILD.gn

@@ -198,7 +198,7 @@ component("vr_ui") {
   if (use_native_partitions) {
     # Mark symbols in this library as belonging to the VR partition. Only
     # exported symbols (module entrypoints) are affected.
-    cflags = [ "-fsymbol-partition=libvr.so" ]
+    cflags = [ "-fsymbol-partition=vr_partition" ]
   }
 
   if (is_android) {

components/module_installer/android/module.cc

@@ -41,7 +41,8 @@ namespace {
 
 typedef bool JniRegistrationFunction(JNIEnv* env);
 
-void* LoadLibrary(const std::string& library_name) {
+void* LoadLibrary(const std::string& library_name,
+                  const std::string& module_name) {
   void* library_handle = nullptr;
 
 #if defined(LOAD_FROM_PARTITIONS)
@@ -50,7 +51,9 @@ void* LoadLibrary(const std::string& library_name) {
   // operation on the Java side, because JNI registration is done explicitly
   // (hence there is no reason for the Java ClassLoader to be aware of the
   // library, for lazy JNI registration).
-  library_handle = BundleUtils::DlOpenModuleLibraryPartition(library_name);
+  const std::string partition_name = module_name + "_partition";
+  library_handle =
+      BundleUtils::DlOpenModuleLibraryPartition(library_name, partition_name);
 #elif defined(COMPONENT_BUILD)
   const std::string lib_name = "lib" + library_name + ".so";
   library_handle = dlopen(lib_name.c_str(), RTLD_LOCAL);
@@ -58,7 +61,7 @@ void* LoadLibrary(const std::string& library_name) {
 #error "Unsupported configuration."
 #endif  // defined(COMPONENT_BUILD)
   CHECK(library_handle != nullptr)
-      << "Could not open feature library: " << dlerror();
+      << "Could not open feature library " << library_name << ": " << dlerror();
 
   return library_handle;
 }
@@ -94,7 +97,7 @@ static void JNI_Module_LoadNative(
   if (libraries.size() > 0) {
     void* library_handle = nullptr;
     for (const auto& library : libraries) {
-      library_handle = LoadLibrary(library);
+      library_handle = LoadLibrary(library, name);
     }
     // module libraries are ordered such that the root library will be the last
     // item in the list. We expect this library to provide the JNI registration

tools/binary_size/libsupersize/linker_map_parser.py

@@ -495,11 +495,11 @@ class MapFileParserLld(object):
       # PROVIDE_HIDDEN lines.
       if level == 1:
         # Ignore sections that belong to feature library partitions. Seeing a
-        # library name is an indicator that we've entered a list of feature
+        # partition name is an indicator that we've entered a list of feature
         # partitions. After these, a single .part.end section will follow to
         # reserve memory at runtime. Seeing the .part.end section also marks the
         # end of partition sections in the map file.
-        if tok.startswith('lib') and tok.endswith('.so'):
+        if tok.endswith('_partition'):
           in_partitions = True
         elif tok == '.part.end':
           # Note that we want to retain .part.end section, so it's fine to

tools/binary_size/libsupersize/testdata/linker_map_parser/Tokenize.golden

@@ -226,8 +226,8 @@
  2F56008        4 (3)        0 WebRtcSpl_MaxAbsValueW16
  2F56008        4 (3)        0 WebRtcSpl_MaxAbsValueW16
  2F56008        4 (3)        4 WebRtcSpl_MaxAbsValueW16
- 34BE000       34 (1) -------- libvr.so
- 34BE000       34 (2) -------- <internal>:(libvr.so)
+ 34BE000       34 (1) -------- vr_partition
+ 34BE000       34 (2) -------- <internal>:(vr_partition)
  34BE034      140 (1) -------- .phdrs
  34BE034      140 (2) -------- <internal>:(.phdrs)
  34BE174       13 (1) -------- .interp

tools/binary_size/libsupersize/testdata/linker_map_parser/test_lld-lto_v1.map

@@ -353,8 +353,8 @@
 
 # Partitions should be ignored at this point. Otherwise, their .text, .rodata,
 # etc. sections will overwrite those of the main partition.
- 34be000  34be000       34     1 libvr.so
- 34be000  34be000       34     1         <internal>:(libvr.so)
+ 34be000  34be000       34     1 vr_partition
+ 34be000  34be000       34     1         <internal>:(vr_partition)
  34be034  34be034      140     1 .phdrs
  34be034  34be034      140     1         <internal>:(.phdrs)
  34be174  34be174       13     1 .interp