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

base/android/library_loader/library_loader_hooks.cc

 // Copyright 2014 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.
 
 #include "base/android/library_loader/library_loader_hooks.h"
 
 #include "base/android/command_line_android.h"
 #include "base/android/jni_string.h"
 #include "base/at_exit.h"
 #include "base/metrics/histogram.h"
@@ skipping 33 matching lines @@
   MAX_BROWSER_HISTOGRAM_CODE = 3,
 };
 
 RendererHistogramCode g_renderer_histogram_code = NO_PENDING_HISTOGRAM_CODE;
 
 enum LibraryLoadFromApkSupportCode {
   // The device's support for loading a library directly from the APK file.
   NOT_SUPPORTED = 0,
   SUPPORTED = 1,
 
-  MAX_LIBRARY_LOAD_FROM_APK_SUPPORT_CODE = 2,
+  // The loader was unable to determine whether the funcionality is supported.

[Ilya Sherman, 2014/10/16 19:00:30]

nit: "funcionality" -> "functionality"

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

Done.

+  UNKNOWN = 2,

[Ilya Sherman, 2014/10/16 19:00:30]

nit: I'd recommend moving this to be the first item in the enum, in case you
ever add more values.

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

Done. It is very likely that more values will indeed be added in the future.

+
+  MAX_LIBRARY_LOAD_FROM_APK_SUPPORT_CODE = 3,
 };

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

You should probably have the enums named the same on both sides, in case a
future developer greps for them.

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

Done.

 
 } // namespace
 
 static void RegisterChromiumAndroidLinkerRendererHistogram(
     JNIEnv* env,
     jclass clazz,
     jboolean requested_shared_relro,
     jboolean load_at_fixed_address_failed) {
   // Note a pending histogram value for later recording.
   if (requested_shared_relro) {
@@ skipping 12 matching lines @@
                             g_renderer_histogram_code,
                             MAX_RENDERER_HISTOGRAM_CODE);
   g_renderer_histogram_code = NO_PENDING_HISTOGRAM_CODE;
 }
 
 static void RecordChromiumAndroidLinkerBrowserHistogram(
     JNIEnv* env,
     jclass clazz,
     jboolean is_using_browser_shared_relros,
     jboolean load_at_fixed_address_failed,
-    jboolean library_load_from_apk_supported) {
+    jint library_load_from_apk_support) {
   // For low-memory devices, record whether or not we successfully loaded the
   // browser at a fixed address. Otherwise just record a normal invocation.
   BrowserHistogramCode histogram_code;
   if (is_using_browser_shared_relros) {
     histogram_code = load_at_fixed_address_failed
                      ? LOW_MEMORY_LFA_BACKOFF_USED : LOW_MEMORY_LFA_SUCCESS;
   } else {
     histogram_code = NORMAL_LRA_SUCCESS;
   }
   UMA_HISTOGRAM_ENUMERATION("ChromiumAndroidLinker.BrowserStates",
                             histogram_code,
                             MAX_BROWSER_HISTOGRAM_CODE);
 
-  // Record whether the device supports loading a library directly from the APK
-  // file.
-  UMA_HISTOGRAM_ENUMERATION("ChromiumAndroidLinker.LibraryLoadFromApkSupported",
-                            library_load_from_apk_supported ?
-                                SUPPORTED : NOT_SUPPORTED,
+  // Record the device support for loading a library directly from the APK file.
+  if (library_load_from_apk_support < 0 ||
+      library_load_from_apk_support >= MAX_LIBRARY_LOAD_FROM_APK_SUPPORT_CODE) {
+    library_load_from_apk_support = UNKNOWN;

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

Definitely replace the '0' with NOT_SUPPORTED enum. But, as enums are cardinal
numbers not ordinal numbers (i.e. they're more like phone numbers that house
numbers!) doing less than or greater than checks can be risky when things change
in the future. It might be worth considering having a switch statement here with
all the valid entries as fall-through cases and the default entry setting the
result to UNKNOWN.

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

I actually think that might be more error prone since anyone adding an Enum
needs to remember to add an entry to the switch statement otherwise it will get
overwritten to zero. I think the check with MAX_LIBRARY... is fine - its quite
common in the codebase (at least in V8).

I agree with making unknown zero though.

[picksi1, 2014/10/17 09:23:51]

If we ditched the default entry in the case statement, the compiler would
complain about unhandled enums... but if this is a common pattern, I won't
complain. :)


FWIW Something I sometimes do is add 2 extra enums that cover the bounds,
setting them equal to the relevent Enums e.g.

NOT_SUPPORTED,
SUPPORTED,
UNKNOWN,
FIRST_VALID_APK_RESULT = NOT_SUPPORTED,
LAST_VALID_APK_RESULT = UNKNOWN

This does rely on future devs updating the enum, but it makes the code explicit
and more readable where these are used to do the bounds check. e.g.

if (( library_load_from_apk_support < FIRST_VALID_APK_RESULT ) || 
    ( library_load_from_apk_support > LAST_VALID_APK_RESULT))

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

+1 this sounds good.

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

Done.

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

After a discussion with simonb, we concluded that there is no point in checking
this since UMA_HISTOGRAM_ENUMERATION checks value range automatically (the only
difference being that there won't be an error log message).

+  }
+  UMA_HISTOGRAM_ENUMERATION("ChromiumAndroidLinker.LibraryLoadFromApkSupport",
+                            library_load_from_apk_support,
                             MAX_LIBRARY_LOAD_FROM_APK_SUPPORT_CODE);
 }
 
 void SetLibraryLoadedHook(LibraryLoadedHook* func) {
   g_registration_callback = func;
 }
 
 static void InitCommandLine(JNIEnv* env, jclass clazz,
                             jobjectArray init_command_line) {
   InitNativeCommandLineFromJavaArray(env, init_command_line);
@@ skipping 27 matching lines @@
 jstring GetVersionNumber(JNIEnv* env, jclass clazz) {
   return ConvertUTF8ToJavaString(env, g_library_version_number).Release();
 }
 
 static void RecordNativeLibraryHack(JNIEnv*, jclass, jboolean usedHack) {
   UMA_HISTOGRAM_BOOLEAN("LibraryLoader.NativeLibraryHack", usedHack);
 }
 
 }  // namespace android
 }  // namespace base