Reland "android: Do not compress locale .pak files inside bundle splits."

This was reverted because it caused startup crashes on non-monochrome
bundle APKs (http://crbug.com/953932). The reason for the crash was that
the ProductConfig.UNCOMPRESSED_LOCALES was empty for non-monochrome
APKs. The only reason this worked on monochrome was because monochrome
happens to include webview resources uncompressed. Now we make sure to
add locale paks to bundle APKs uncompressed, which correctly fills out
ProductConfig.UNCOMPRESSED_LOCALES.

In addition, I added a task to clean up the old extracted files if we
detect they are no longer needed.

Diffs from original patch are from Patchset 1 on.

Original change's description:
> android: Do not compress locale .pak files inside bundle splits.
>
> This CL ensures that the locale-related pak files stored in
> app bundle splits are stored uncompressed inside the language-based
> splits (e.g. base-fr.apk for the French language), and are read
> directly from it.
>
> Benefits include:
>   - Less disk i/o during Chrome startup for bundles.
>
>   - Reduced overall install size, since the corresponding files
>     need not be extracted. I.e. since the compression ratio is
>     roughly 50% for locale .pak files, we have something like.
>
>       Before:   compressed 50 kiB (APK) + uncompressed 100 kiB (/data)
>       After:    uncompressed 100 kiB (APK)
>
> However, this increases the size of the base and split apks slightly.
>
> In more details:
>
> - Modify create_app_bundle.py to ensure that the split-specific
>   locale .pak files are never compressed.
>
>   Also, move the en-GB.pak file location from fallback-locales/
>   to the split-specific locales#lang_en/, this ensures that the
>   file will end up in the English-based split (base-en.apk)
>   instead.
>
> - Modify ResourceExtractor.java to only extract the locale .pak
>   files in regular APKs, ignoring the ones from bundles. Also make
>   the log messages disappear unless the DEBUG final variable is
>   set to true (less logs are always better for startup time).
>
> - Modify ResourceBundle.java to check for the presence of
>   split-specific locale .pak files as well, and to also check
>   that the corresponding file path being returned actually
>   exists inside the apk or bundle.
>
> - Modify ResourceBundle::LoadLocaleResources() to try loading
>   the split-specific locale pak files before the extracted version
>   if available, and add a technical note explaining what the code
>   is trying to do (it is sufficiently subtle to be explained).
>
> - Add missing dependencies to the vr_android_unittests and
>   shell_dialogs_unittest targets. They otherwise fail to
>   build due to http://crbug.com/951419.
>
> BUG=933943,933962, 951419
> R=​agrieve@chromium.org,yfriedman@chromium.org,estevenson@chromium.org,benmason@chromium.org,dtrainor@chromium.org,jaekyun@chromium.org
> TBR=sky@chromium.org
>
> Change-Id: I02c1688b8f8ed8f62cfdaa1f12c2ea15f8198e4a
> Binary-Size: Increments overall bundle size by 100 kiB, but reduces overall install size by about 50% of that.
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1514684
> Commit-Queue: David Turner <digit@chromium.org>
> Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#650753}

Bug: 933943, 933962
Binary-Size: Increments overall bundle size by 100 kiB, but reduces overall install size by about 50% of that.
Change-Id: If85211fa83809d814eb55fe66d81becc1723d9a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1918367
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715915}

build/android/gyp/create_app_bundle.py

@@ -25,11 +25,11 @@ import bundletool
 # Location of language-based assets in bundle modules.
 _LOCALES_SUBDIR = 'assets/locales/'
 
-# The fallback language should always have its .pak files included in
+# The fallback locale should always have its .pak file included in
 # the base apk, i.e. not use language-based asset targetting. This ensures
 # that Chrome won't crash on startup if its bundle is installed on a device
 # with an unsupported system locale (e.g. fur-rIT).
-_FALLBACK_LANGUAGE = 'en'
+_FALLBACK_LOCALE = 'en-US'
 
 # List of split dimensions recognized by this tool.
 _ALL_SPLIT_DIMENSIONS = [ 'ABI', 'SCREEN_DENSITY', 'LANGUAGE' ]
@@ -190,6 +190,9 @@ def _GenerateBundleConfigJson(uncompressed_assets, compress_shared_libraries,
   # Whether other .so files are compressed is controlled by
   # "uncompressNativeLibraries".
   uncompressed_globs = ['lib/*/crazy.*']
+  # Locale-specific pak files stored in bundle splits need not be compressed.
+  uncompressed_globs.extend(
+      ['assets/locales#lang_*/*.pak', 'assets/fallback-locales/*.pak'])
   uncompressed_globs.extend('assets/' + x for x in uncompressed_assets)
   # NOTE: Use '**' instead of '*' to work through directories!
   uncompressed_globs.extend('**.' + ext for ext in _UNCOMPRESSED_FILE_EXTS)
@@ -242,8 +245,8 @@ def _RewriteLanguageAssetPath(src_path):
   else:
     android_language = android_locale
 
-  if android_language == _FALLBACK_LANGUAGE:
-    # Fallback language .pak files must be placed in a different directory
+  if locale == _FALLBACK_LOCALE:
+    # Fallback locale .pak files must be placed in a different directory
     # to ensure they are always stored in the base module.
     result_path = 'assets/fallback-locales/%s.pak' % locale
   else:

chrome/android/BUILD.gn

@@ -1361,9 +1361,8 @@ if (current_toolchain == default_toolchain) {
       }
     }
 
-    # This target is separate from monochrome_apk_pak_assets because it does not
-    # disable compression.
     android_assets("${_variant}_locale_pak_assets") {
+      disable_compression = _is_bundle_module
       renaming_sources = []
       renaming_destinations = []
       foreach(_locale, locales - android_chrome_omitted_locales) {

chrome/browser/android/vr/BUILD.gn

@@ -260,5 +260,6 @@ test("vr_android_unittests") {
     "//testing/android/native_test:native_test_native_code",
     "//testing/gmock",
     "//testing/gtest",
+    "//ui/android:ui_java",  # TODO: Remove once http://crbug.com/951419 is fixed!
   ]
 }

ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java

@@ -109,34 +109,6 @@ public class LocalizationUtils {
         }
     }
 
-    /**
-     * Return one default locale-specific PAK file name associated with a given language.
-     *
-     * @param language Language name (e.g. "en").
-     * @return A Chromium-specific locale name (e.g. "en-US") matching the input language
-     *         that can be used to access compressed locale pak files.
-     */
-    public static String getDefaultCompressedPakLocaleForLanguage(String language) {
-        // IMPORTANT: Keep in sync with the mapping found in:
-        // //build/android/gyp/resource_utils.py
-
-        // NOTE: All languages provide locale files named '<language>.pak', except
-        //       for a few exceptions listed below. E.g. for the English language,
-        //       the 'en-US.pak' and 'en-GB.pak' files are provided, and there is
-        //       no 'en.pak' file.
-        switch (language) {
-            case "en":
-                return "en-US";
-            case "pt":
-                return "pt-PT";
-            case "zh":
-                return "zh-CN";
-            default:
-                // NOTE: for Spanish (es), both es.pak and es-419.pak are used. Hence this works.
-                return language;
-        }
-    }
-
     /**
      * Return true iff a locale string matches a specific language string.
      *

ui/android/java/src/org/chromium/ui/base/ResourceBundle.java

@@ -4,9 +4,16 @@
 
 package org.chromium.ui.base;
 
+import android.content.res.AssetFileDescriptor;
+import android.content.res.AssetManager;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.LocaleUtils;
+import org.chromium.base.Log;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 
+import java.io.IOException;
 import java.util.Arrays;
 
 /**
@@ -20,6 +27,7 @@ import java.util.Arrays;
  */
 @JNINamespace("ui")
 public final class ResourceBundle {
+    private static final String TAG = "ResourceBundle";
     private static String[] sCompressedLocales;
     private static String[] sUncompressedLocales;
 
@@ -54,11 +62,47 @@ public final class ResourceBundle {
         return sCompressedLocales;
     }
 
+    /**
+     * Return the location of a locale-specific uncompress .pak file asset.
+     *
+     * @param locale Chromium locale name.
+     * @param inBundle If true, return the path of the uncompressed .pak file
+     *                 containing Chromium UI strings within app bundles. If
+     *                 false, return the path of the uncompressed WebView UI
+     *                 strings instead. Note that APK .pak files are stored
+     *                 compressed and handled differently.
+     * @return Asset path to uncompressed .pak file, or null if the locale is
+     *         not supported by this version of Chromium, or the file is
+     *         missing.
+     */
     @CalledByNative
-    private static String getLocalePakResourcePath(String locale) {
-        assert sUncompressedLocales != null;
-        if (Arrays.binarySearch(sUncompressedLocales, locale) >= 0) {
-            return "assets/stored-locales/" + locale + ".pak";
+    private static String getLocalePakResourcePath(String locale, boolean inBundle) {
+        if (sUncompressedLocales == null) {
+            // Locales may be null in unit tests.
+            return null;
+        }
+        if (Arrays.binarySearch(sUncompressedLocales, locale) < 0) {
+            // This locale is not supported by Chromium.
+            return null;
+        }
+        String pathPrefix = "assets/stored-locales/";
+        if (inBundle) {
+            if (locale.equals("en-US")) {
+                pathPrefix = "assets/fallback-locales/";
+            } else {
+                String lang = LocalizationUtils.getSplitLanguageForAndroid(
+                        LocaleUtils.toLanguage(locale));
+                pathPrefix = "assets/locales#lang_" + lang + "/";
+            }
+        }
+        String assetPath = pathPrefix + locale + ".pak";
+        AssetManager manager = ContextUtils.getApplicationContext().getAssets();
+        // The file may not exist if the language split for this locale has not been installed
+        // yet, so make sure it exists before returning the asset path.
+        try (AssetFileDescriptor afd = manager.openNonAssetFd(assetPath)) {
+            return assetPath;
+        } catch (IOException e) {
+            Log.e(TAG, "Error while loading asset %s: %s", assetPath, e);
         }
         return null;
     }

ui/android/java/src/org/chromium/ui/resources/ResourceExtractor.java

@@ -38,7 +38,6 @@ public class ResourceExtractor {
     private static final String V8_SNAPSHOT_DATA_FILENAME = "snapshot_blob.bin";
     private static final String FALLBACK_LOCALE = "en-US";
     private static final String COMPRESSED_LOCALES_DIR = "locales";
-    private static final String COMPRESSED_LOCALES_FALLBACK_DIR = "fallback-locales";
 
     private class ExtractTask implements Runnable {
         private final List<Runnable> mCompletionCallbacks = new ArrayList<Runnable>();
@@ -169,62 +168,29 @@ public class ResourceExtractor {
             activeLocales.add(FALLBACK_LOCALE);
         }
 
+        // * For bundles, locale pak files are always stored uncompressed
+        //   either under base.apk!/assets/fallback-locales/<locale>.pak or
+        //   base-<lang>.apk!/assets/locales#lang_<lang>/<locale>.pak. They
+        //   never need to be extracted.
+        //
         // * For regular APKs, the locale pak files are stored under:
         //      base.apk!/assets/locales/<locale>.pak
         //
         //   where <locale> is a Chromium-specific locale name.
         //
-        // * When using app bundles, the locale pak files are stored in
-        //   language-specific directories that look like:
-        //      <split>.apk!/assets/locales#lang_<lang>/<locale>.pak
-        //
-        //   Where <lang> is an Android-specific ISO-639-1 language identifier.
-        //
-        // * With the exception of the fallback (English) pak files which are stored
-        //   in the base module under:
-        //       assets/locales-fallback/<locale>.pak
-        //
-        //   Moreover, when the bundle uses APK splits, there is no guarantee that the split
-        //   corresponding to the current device locale is installed yet, but the one matching
-        //   uiLanguage should be there, since the value is determined by loading a resource string
-        //   from the current application's asset manager.
-        //
         AssetManager assetManager = ContextUtils.getApplicationAssets();
-        String localesSrcDir;
-        String androidSplitLanguage = LocalizationUtils.getSplitLanguageForAndroid(uiLanguage);
-        String langSpecificPath = COMPRESSED_LOCALES_DIR + "#lang_" + androidSplitLanguage;
-        String defaultLocalePakName =
-                LocalizationUtils.getDefaultCompressedPakLocaleForLanguage(uiLanguage) + ".pak";
-
-        if (assetPathHasFile(assetManager, langSpecificPath, defaultLocalePakName)) {
-            // This is an app bundle, and the split containing the pak files for
-            // the current locale is installed.
-            localesSrcDir = langSpecificPath;
-        } else if (assetPathHasFile(
-                           assetManager, COMPRESSED_LOCALES_DIR, activeLocales.get(0) + ".pak")) {
-            // This is a regular APK, and all pak files are available.
-            localesSrcDir = COMPRESSED_LOCALES_DIR;
-        } else if (assetPathHasFile(assetManager, COMPRESSED_LOCALES_FALLBACK_DIR,
-                           activeLocales.get(0) + ".pak")) {
-            // This is a fallback language pak file.
-            localesSrcDir = COMPRESSED_LOCALES_FALLBACK_DIR;
-        } else {
-            // This is an app bundle, but the split containing the pak files for the current UI
-            // locale is *not* installed yet. This should never happen in theory, and there is
-            // little that can be done at this point, so return an empty list. Nothing will get
-            // extracted, and Chromium may later crash when trying to access the PAK file from
-            // native code.
-            Log.e(TAG, "Android Locale: %s misses split for .pak files: %s", defaultLocale,
-                    Arrays.toString(activeLocales.toArray()));
+        if (!assetPathHasFile(
+                    assetManager, COMPRESSED_LOCALES_DIR, activeLocales.get(0) + ".pak")) {
+            Log.i(TAG, "No locale pak files to extract, assuming app bundle.");
             return new String[] {};
         }
 
+        // This is a regular APK, and all pak files are available.
         // Return the list of locale pak file paths corresponding to the current language.
         String[] localePakFiles = new String[activeLocales.size()];
         for (int n = 0; n < activeLocales.size(); ++n) {
-            localePakFiles[n] = localesSrcDir + '/' + activeLocales.get(n) + ".pak";
+            localePakFiles[n] = COMPRESSED_LOCALES_DIR + '/' + activeLocales.get(n) + ".pak";
         }
-        Log.i(TAG, "Using app bundle locale directory: " + localesSrcDir);
         Log.i(TAG, "UI Language: %s requires .pak files: %s", uiLanguage,
                 Arrays.toString(activeLocales.toArray()));
 
@@ -318,11 +284,11 @@ public class ResourceExtractor {
             return;
         }
 
-        // If a previous release extracted resources, and the current release does not,
-        // deleteFiles() will not run and some files will be left. This currently
-        // can happen for ContentShell, but not for Chrome proper, since we always extract
-        // locale pak files.
+        // If a previous release extracted resources, and the current release does not, delete the
+        // old files since they are no longer needed.
         if (shouldSkipPakExtraction()) {
+            PostTask.postTask(
+                    TaskTraits.BEST_EFFORT, () -> { deleteFiles(getOutputDir().list()); });
             return;
         }
 

ui/android/junit/src/org/chromium/ui/base/LocalizationUtilsTest.java

@@ -22,16 +22,6 @@ import org.chromium.base.test.BaseRobolectricTestRunner;
 @RunWith(BaseRobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class LocalizationUtilsTest {
-    @Test
-    @SmallTest
-    public void testGetDefaultCompressedPakLocaleForLanguage() {
-        assertEquals("fr", LocalizationUtils.getDefaultCompressedPakLocaleForLanguage("fr"));
-        assertEquals("es", LocalizationUtils.getDefaultCompressedPakLocaleForLanguage("es"));
-        assertEquals("en-US", LocalizationUtils.getDefaultCompressedPakLocaleForLanguage("en"));
-        assertEquals("pt-PT", LocalizationUtils.getDefaultCompressedPakLocaleForLanguage("pt"));
-        assertEquals("zh-CN", LocalizationUtils.getDefaultCompressedPakLocaleForLanguage("zh"));
-    }
-
     @Test
     @SmallTest
     public void testGetSplitLanguageForAndroid() {

ui/base/resource/resource_bundle_android.cc

@@ -54,9 +54,10 @@ bool LoadFromApkOrFile(const char* apk_path,
 }
 
 int LoadLocalePakFromApk(const std::string& app_locale,
+                         bool in_split,
                          base::MemoryMappedFile::Region* out_region) {
   std::string locale_path_within_apk =
-      GetPathForAndroidLocalePakWithinApk(app_locale);
+      GetPathForAndroidLocalePakWithinApk(app_locale, in_split);
   if (locale_path_within_apk.empty()) {
     LOG(WARNING) << "locale_path_within_apk.empty() for locale "
                  << app_locale;
@@ -95,8 +96,10 @@ void ResourceBundle::LoadCommonResources() {
 // static
 bool ResourceBundle::LocaleDataPakExists(const std::string& locale) {
   if (g_locale_paks_in_apk) {
-    return !GetPathForAndroidLocalePakWithinApk(locale).empty();
+    return !GetPathForAndroidLocalePakWithinApk(locale, false).empty();
   }
+  if (!GetPathForAndroidLocalePakWithinApk(locale, true).empty())
+    return true;
   return !GetLocaleFilePath(locale).empty();
 }
 
@@ -111,22 +114,74 @@ std::string ResourceBundle::LoadLocaleResources(
   }
   std::string app_locale = l10n_util::GetApplicationLocale(pref_locale);
 
+  // Some Chromium apps have two sets of .pak files for their UI strings, i.e.:
+  //
+  // a) WebView strings, which are always stored uncompressed under
+  //    assets/stored-locales/ inside the APK or App Bundle.
+  //
+  // b) For APKs, the Chrome UI strings are stored under assets/locales/
+  //    in compressed form. The relevant pak files is extracted on startup
+  //    and stored on the /data partition, with a version-specific suffix.
+  //
+  // c) For App Bundles, Chrome UI strings are stored uncompressed under
+  //    assets/locales#lang_<lang>/ (where <lang> is an Android language code)
+  //    and assets/fallback-locales/ (for en-US.pak only).
+  //
+  // Which .pak files to load are determined here by two global variables with
+  // the following meaning:
+  //
+  //  g_locale_paks_in_apk:
+  //    If true, load the WebView strings from stored-locales/<locale>.pak file
+  //    as the primary locale pak file.
+  //
+  //    If false, try to load it from the app bundle specific location
+  //    (e.g. locales#lang_<language>/<locale>.pak). If the latter does not
+  //    exist, try to lookup the extracted APK-specific locale .pak file
+  //    from /data/app/.../<locale>.pak@<version> instead.
+  //
+  //    g_locale_paks_in_apk is set by SetLocalePaksStoredInApk() which
+  //    is called from the WebView startup code.
+  //
+  //  g_load_secondary_locale_paks:
+  //    If true, load the Webview strings from stored-locales/<locale>.pak file
+  //    as the secondary locale pak file. Otherwise don't load a secondary
+  //    locale at all.
+  //
+  //    This is set by SetLoadSecondaryLocalePaks() which is called
+  //    during ChromeMainDelegate::PostEarlyInitialization() with a value
+  //    that is true iff there are stored-locale/ .pak files.
+  //
+  // In other words, if both |g_locale_paks_in_apk| and
+  // |g_load_secondary_locale_paks| are true, the stored-locales file will be
+  // loaded twice as both the primary and secondary. However, this should
+  // never happen in practice.
+
   // Load primary locale .pak file.
   if (g_locale_paks_in_apk) {
-    g_locale_pack_fd = LoadLocalePakFromApk(app_locale, &g_locale_pack_region);
+    g_locale_pack_fd =
+        LoadLocalePakFromApk(app_locale, false, &g_locale_pack_region);
   } else {
+    // Support overridden pak path for testing.
     base::FilePath locale_file_path = GetOverriddenPakPath();
-    if (locale_file_path.empty())
-      locale_file_path = GetLocaleFilePath(app_locale);
-
     if (locale_file_path.empty()) {
-      // It's possible that there is no locale.pak.
-      LOG(WARNING) << "locale_file_path.empty() for locale " << app_locale;
-      return std::string();
+      // Try to find the uncompressed split-specific asset file.
+      g_locale_pack_fd =
+          LoadLocalePakFromApk(app_locale, true, &g_locale_pack_region);
+    }
+    if (g_locale_pack_fd < 0) {
+      // Otherwise, try to locate the extracted locale .pak file.
+      if (locale_file_path.empty())
+        locale_file_path = GetLocaleFilePath(app_locale);
+
+      if (locale_file_path.empty()) {
+        // It's possible that there is no locale.pak.
+        LOG(WARNING) << "locale_file_path.empty() for locale " << app_locale;
+        return std::string();
+      }
+      int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
+      g_locale_pack_fd = base::File(locale_file_path, flags).TakePlatformFile();
+      g_locale_pack_region = base::MemoryMappedFile::Region::kWholeFile;
     }
-    int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
-    g_locale_pack_fd = base::File(locale_file_path, flags).TakePlatformFile();
-    g_locale_pack_region = base::MemoryMappedFile::Region::kWholeFile;
   }
 
   locale_resources_data_ = LoadDataPackFromLocalePak(
@@ -141,7 +196,7 @@ std::string ResourceBundle::LoadLocaleResources(
   // would have a copy of all the resources in the secondary locale pak.
   if (g_load_secondary_locale_paks) {
     g_secondary_locale_pack_fd = LoadLocalePakFromApk(
-        app_locale, &g_secondary_locale_pack_region);
+        app_locale, false, &g_secondary_locale_pack_region);
 
     secondary_locale_resources_data_ = LoadDataPackFromLocalePak(
         g_secondary_locale_pack_fd, g_secondary_locale_pack_region);
@@ -208,11 +263,12 @@ int GetSecondaryLocalePackFd(base::MemoryMappedFile::Region* out_region) {
   return g_secondary_locale_pack_fd;
 }
 
-std::string GetPathForAndroidLocalePakWithinApk(const std::string& locale) {
+std::string GetPathForAndroidLocalePakWithinApk(const std::string& locale,
+                                                bool in_bundle) {
   JNIEnv* env = base::android::AttachCurrentThread();
   base::android::ScopedJavaLocalRef<jstring> ret =
       Java_ResourceBundle_getLocalePakResourcePath(
-          env, base::android::ConvertUTF8ToJavaString(env, locale));
+          env, base::android::ConvertUTF8ToJavaString(env, locale), in_bundle);
   if (ret.obj() == nullptr) {
     return std::string();
   }

ui/base/resource/resource_bundle_android.h

@@ -48,8 +48,12 @@ UI_BASE_EXPORT void SetLoadSecondaryLocalePaks(bool value);
 // Returns the path within the apk for the given locale's .pak file, or an
 // empty string if it doesn't exist.
 // Only locale paks for the active Android language can be retrieved.
+// If |inSplit| is true, look into bundle split-specific location (e.g.
+// 'assets/locales#lang_<lang>/<locale>.pak', otherwise use the default
+// WebView-related location, i.e. 'assets/stored-locales/<locale>.pak'.
 UI_BASE_EXPORT std::string GetPathForAndroidLocalePakWithinApk(
-    const std::string& locale);
+    const std::string& locale,
+    bool in_split = false);
 
 // Get the density of the primary display. Use this instead of using Display
 // to avoid initializing Display in child processes.

ui/shell_dialogs/BUILD.gn

@@ -111,6 +111,11 @@ test("shell_dialogs_unittests") {
   if (is_mac) {
     deps += [ "//components/remote_cocoa/app_shim" ]
   }
+
+  # TODO: Remove once http://crbug.com/951419 is fixed.
+  if (is_android) {
+    deps += [ "//ui/android:ui_java" ]
+  }
 }
 
 source_set("test_support") {