Record the return value of NativeLibraryPreloader.loadLibrary in UMA

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 a9a3141a3deb009e8ffb2579bbaabff597e3d616..354d07fd1a3b3f9951d2ec50399d0470dcaf34d1 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
@@ -86,6 +86,10 @@ public class LibraryLoader {
     // will be reported via UMA. Set once when the libraries are done loading.
     private long mLibraryLoadTimeMs;
 
+    // The return value of NativeLibraryPreloader.loadLibrary(), which will be reported
+    // via UMA.
+    private int mLibraryPreloaderStatus;

[Torne, 2016/05/26 10:53:24]

Initialise this to -1 or something? It will get default-initialised to 0, which
if we somehow end up uploading the value without having set it will look like a
success.

[michaelbai, 2016/05/26 19:10:35]

We don't need to do that, first, if so, this will assume the method never return
-1, second, this value is only recorded if NativeLibraryPreloader is set, and we
also have boolean to indicate whether the result is registered for renderer in
native side.

[Torne, 2016/05/27 10:21:13]

It won't, it's an errno-style error code, they're conventionally positive
numbers for errors (also we define the codes, so we can just not do that). Even
if it does, better to confuse it with an error value than with the success
value.
Right, it's not intended that it will upload an uninitialised value, it would be
a bug. What I'm saying is that if the "uninitialised" value is "success" then
we'd never *notice* if there was a bug since the stats would look like
everything was fine, and so it would be safer to initialise this to a "failure"
value.

[michaelbai, 2016/05/27 15:52:42]

I probably miss something here, I meant current implementation doesn't assume
anything of return value, would it be better than relying on return value can't
be -1?

+
     /**
      * Set native library preloader, if set, the NativeLibraryPreloader.loadLibrary will be invoked
      * before calling System.loadLibrary, this only applies when not using the chromium linker.
@@ -282,7 +286,7 @@ public class LibraryLoader {
                     linker.finishLibraryLoad();
                 } else {
                     if (sLibraryPreloader != null) {
-                        sLibraryPreloader.loadLibrary(context);
+                        mLibraryPreloaderStatus = sLibraryPreloader.loadLibrary(context);
                     }
                     // Load libraries using the system linker.
                     for (String library : NativeLibraries.LIBRARIES) {
@@ -383,6 +387,9 @@ public class LibraryLoader {
                                                               getLibraryLoadFromApkStatus(context),
                                                               mLibraryLoadTimeMs);
         }
+        if (sLibraryPreloader != null) {
+            nativeRecordLibraryPreloaderBrowserHistogram(mLibraryPreloaderStatus);
+        }
     }
 
     // Returns the device's status for loading a library directly from the APK file.
@@ -409,6 +416,9 @@ public class LibraryLoader {
                                                                  loadAtFixedAddressFailed,
                                                                  mLibraryLoadTimeMs);
         }
+        if (sLibraryPreloader != null) {
+            nativeRegisterLibraryPreloaderRendererHistogram(mLibraryPreloaderStatus);
+        }
     }
 
     /**
@@ -442,6 +452,10 @@ public class LibraryLoader {
             int libraryLoadFromApkStatus,
             long libraryLoadTime);
 
+    // Method called to record the return value of NativeLibraryPreloader.loadLibrary for the main
+    // browser process.
+    private native void nativeRecordLibraryPreloaderBrowserHistogram(int status);
+
     // Method called to register (for later recording) statistics about the Chromium linker
     // operation for a renderer process. Indicates whether the linker attempted relro sharing,
     // and if it did, whether the library failed to load at a fixed address. Also records the
@@ -451,6 +465,10 @@ public class LibraryLoader {
             boolean loadAtFixedAddressFailed,
             long libraryLoadTime);
 
+    // Method called to register (for later recording) the return value of
+    // NativeLibraryPreloader.loadLibrary for a renderer process.
+    private native void nativeRegisterLibraryPreloaderRendererHistogram(int status);
+
     // Get the version of the native library. This is needed so that we can check we
     // have the right version before initializing the (rest of the) JNI.
     private native String nativeGetVersionNumber();