Fix low-memory devices crashing on Chrome start up during UMA recodring

base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java

Index: base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
diff --git a/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java b/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
index 86bca4399d2abf908ebb34f501de147a7e2484b0..174b5b9eca74a6e7fd0ea16dbc3f7c650cbda2e7 100644
--- a/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
+++ b/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
@@ -50,9 +50,17 @@ public class LibraryLoader {
     private static boolean sIsUsingBrowserSharedRelros = false;
     private static boolean sLoadAtFixedAddressFailed = false;
 
-    // One-way switch recording whether the device supports memory mapping
-    // APK files with executable permissions. Only used in the browser.
-    private static boolean sLibraryLoadFromApkSupported = false;
+    // The name of the APK file.
+    private static String sApkFileName = null;
+
+    // Constants representing the support of the device for loading a library
+    // directly from the APK file.
+    //   NOT_SUPPORTED -> The functionality is not supported.
+    //   SUPPORTED     -> The functionality is supported.
+    //   UNKNOWN       -> The loader was unable to determine whether the funcionality is supported.
+    public static final int LIBRARY_LOAD_FROM_APK_NOT_SUPPORTED = 0;
+    public static final int LIBRARY_LOAD_FROM_APK_SUPPORTED = 1;
+    public static final int LIBRARY_LOAD_FROM_APK_UNKNOWN = 2;

[petrcermak, 2014/10/16 18:48:52]

I'm not completely satisfied with the way this support flag is passed from Java
to C++ (for UMA) here. I can think of four possible solutions:

1. Use two boolean flags (e.g. libraryLoadFromApkChecked and
libraryLoadFromApkSupported). While this is in line with the other UMA flags
(see library_loader_hooks.cc), it is not very elegant (one variable effectively
says whether the other one has any meaning) and scalable (it is likely that
there will be more options in the future).

2. Use an int. The main problem with this approach is that we now need to keep
three definitions in sync: histograms.xml:Supported,
library_loader_hooks.cc:LibraryLoadFromApkSupportCode, and
LibraryLoader.java:LIBRARY_LOAD_FROM_APK_*. However, multiple definitions are
not avoided by the other solutions either.

3. Use an enum. This would be an ideal choice from a software design point of
view. However, it would require adding a lot of boilerplate for transferring an
enum between the two languages.

4. Use a string (e.g. "SUP", "NOT", and "UNK"). This would require almost no
boilerplate at all. Moreover, it is somewhat less error-prone (from a human
point of view) than the int solution because the values have clear meanings. On
the other hand, this could introduce many problems associated with strings
(encoding, typos, ...).

Given all of this, I decided to use ints (option 2). However, I'm open to any
other suggestions.

[rmcilroy, 2014/10/17 09:12:59]

The JNI generator can help here. If you look at MemoryPressureList.template
(https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...)
you can see an example of how to create a Java side int constants which keep in
sync with the c++ side. I would follow this model.

[petrcermak, 2014/10/17 14:51:27]

Done.

 
     // One-way switch becomes true if the system library loading failed,
     // and the right native library was found and loaded by the hack.
@@ -160,21 +168,18 @@ public class LibraryLoader {
                 long startTime = SystemClock.uptimeMillis();
                 boolean useChromiumLinker = Linker.isUsed();
 
+                if (context != null) {
+                    sApkFileName = context.getApplicationInfo().sourceDir;
+                }
+
                 if (useChromiumLinker) {
                     // Load libraries using the Chromium linker.
                     Linker.prepareLibraryLoad();
 
-                    // Check if the device supports loading a library directly from the APK file.
-                    String apkfile = context.getApplicationInfo().sourceDir;
-                    if (Linker.isInBrowserProcess()) {
-                        sLibraryLoadFromApkSupported = Linker.checkLibraryLoadFromApkSupport(
-                                apkfile);
-                    }
-
                     for (String library : NativeLibraries.LIBRARIES) {
                         String zipfile = null;
                         if (Linker.isInZipFile()) {
-                            zipfile = apkfile;
+                            zipfile = sApkFileName;
                             Log.i(TAG, "Loading " + library + " from within " + zipfile);
                         } else {
                             Log.i(TAG, "Loading: " + library);
@@ -316,10 +321,17 @@ public class LibraryLoader {
     // onNativeInitializationComplete().
     private static void recordBrowserProcessHistogram() {
         if (Linker.isUsed()) {
-            assert Linker.isInBrowserProcess();
+            final int libraryLoadFromApkSupport;

[rmcilroy, 2014/10/17 09:12:59]

Nit - we don't usually use final on locals. Personally I'm fine with this but it
might be better to avoid it for consistancy.

[petrcermak, 2014/10/17 14:51:28]

Done (although I've seen it used in Linker.java for example and think it is a
good code practice).

+            if (sApkFileName != null) {
+                libraryLoadFromApkSupport = Linker.checkLibraryLoadFromApkSupport(sApkFileName) ?
+                        LIBRARY_LOAD_FROM_APK_SUPPORTED : LIBRARY_LOAD_FROM_APK_NOT_SUPPORTED;
+            } else {
+                libraryLoadFromApkSupport = LIBRARY_LOAD_FROM_APK_UNKNOWN;
+                Log.w(TAG, "Unknown APK filename due to null context");
+            }

[picksi1, 2014/10/17 08:23:24]

Might be more readable to swap the if/else bodies and remove the '!'?

[petrcermak, 2014/10/17 14:51:27]

I thought about it and I would prefer to keep it this way as the "!= null" case
is the "normal" one (so it seems more natural to me to have it first).

             nativeRecordChromiumAndroidLinkerBrowserHistogram(sIsUsingBrowserSharedRelros,
                                                               sLoadAtFixedAddressFailed,
-                                                              sLibraryLoadFromApkSupported);
+                                                              libraryLoadFromApkSupport);

[rmcilroy, 2014/10/17 09:54:45]

Actually, I'm just about to turn on loading form the APK for 64-bit.  Could we
add a seperate enum for LIBRARY_LOADED_FROM_APK and return this instead if
Linker.loadLibraryInZipFile is true and succeeds.

[petrcermak, 2014/10/17 14:51:27]

Done. Note that Linker.loadLibraryInZipFile doesn't return true but it throws an
exception if the load fails. There is one issue: In the (presumably) unlikely
event of first loading a library *not* from the APK and then loading the same
library *from* the APK, LIBRARY_LOAD_FROM_APK_SUCCESSFUL will be reported
(because the Linker doesn't load a library twice). Is this something we need to
take care of? If yes, we need to change the return type of
Linker.loadLibraryInZipFile or move the new sLibraryLoadedFromApk flag from the
LibraryLoader to the Linker.

         }
     }
 
@@ -348,11 +360,11 @@ public class LibraryLoader {
     // Method called to record statistics about the Chromium linker operation for the main
     // browser process. Indicates whether the linker attempted relro sharing for the browser,
     // and if it did, whether the library failed to load at a fixed address. Also records
-    // support for memory mapping APK files with executable permissions.
+    // support for loading a library directly from the APK file.
     private static native void nativeRecordChromiumAndroidLinkerBrowserHistogram(
             boolean isUsingBrowserSharedRelros,
             boolean loadAtFixedAddressFailed,
-            boolean apkMemoryMappingSupported);
+            int libraryLoadFromApkSupport);
 

[picksi1, 2014/10/17 08:23:24]

Now that you've started using enums are the first two parameters here really
still bools? :)

[petrcermak, 2014/10/17 14:51:27]

ditto (as line 181).

     // Method called to register (for later recording) statistics about the Chromium linker
     // operation for a renderer process. Indicates whether the linker attempted relro sharing,