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

base/android/library_loader/library_loader_hooks.cc

Index: base/android/library_loader/library_loader_hooks.cc
diff --git a/base/android/library_loader/library_loader_hooks.cc b/base/android/library_loader/library_loader_hooks.cc
index 819fe3df5e49486d5e2e53a5c0f0668ad69c7ca8..3db67cfb4eaf744e085a018d5bdcb252b9f6ecc5 100644
--- a/base/android/library_loader/library_loader_hooks.cc
+++ b/base/android/library_loader/library_loader_hooks.cc
@@ -51,7 +51,10 @@ enum LibraryLoadFromApkSupportCode {
   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
@@ -85,7 +88,7 @@ static void RecordChromiumAndroidLinkerBrowserHistogram(
     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;
@@ -99,11 +102,13 @@ static void RecordChromiumAndroidLinkerBrowserHistogram(
                             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);
 }