Bug 1277214 - Stop reading env vars when the key no longer exists. r=grisha draft
authorMichael Comella <michael.l.comella@gmail.com>
Wed, 01 Jun 2016 16:29:26 -0700
changeset 374145 e2a035f75078b7a66ec7417a0998746b2ef150de
parent 374144 362a73a76f94e1f7403b1c08f804a97f41fbdd26
child 374146 a8274100196a42e3a3028f65109d9044fe79fbfa
push id19946
push usermichael.l.comella@gmail.com
push dateWed, 01 Jun 2016 23:55:51 +0000
reviewersgrisha
bugs1277214
milestone49.0a1
Bug 1277214 - Stop reading env vars when the key no longer exists. r=grisha In the previous implementation, we'd stop reading when the value would return null, however, this breaks with SafeIntents where null is returned if a value throws a runtime exception - i.e. we might stop reading at env2 if it throws even though env3+ exist. MozReview-Commit-ID: 7iZgUAjBSmB
mobile/android/base/java/org/mozilla/gecko/util/IntentUtils.java
--- a/mobile/android/base/java/org/mozilla/gecko/util/IntentUtils.java
+++ b/mobile/android/base/java/org/mozilla/gecko/util/IntentUtils.java
@@ -27,44 +27,58 @@ public class IntentUtils {
      * with the key -> value format:
      *   env# -> ENV_VAR=VALUE
      *
      * # in env# is expected to be increasing from 0.
      *
      * @return A Map of environment variable name to value, e.g. ENV_VAR -> VALUE
      */
     public static HashMap<String, String> getEnvVarMap(@NonNull final Intent unsafeIntent) {
+        // Optimization: get matcher for re-use. Pattern.matcher creates a new object every time so it'd be great
+        // to avoid the unnecessary allocation, particularly because we expect to be called on the startup path.
+        final Pattern envVarPattern = Pattern.compile(ENV_VAR_REGEX);
+        final Matcher matcher = envVarPattern.matcher(""); // argument does not matter here.
+
         // This is expected to be an external intent so we should use SafeIntent to prevent crashing.
         final SafeIntent safeIntent = new SafeIntent(unsafeIntent);
         final HashMap<String, String> out = new HashMap<>();
-        final Pattern envVarPattern = Pattern.compile(ENV_VAR_REGEX);
-        Matcher matcher = null;
-
-        String envValue = safeIntent.getStringExtra("env0");
-        int i = 1;
-        while (envValue != null) {
-            // Optimization to re-use matcher rather than creating new
-            // objects because this is used in the startup path.
-            if (matcher == null) {
-                matcher = envVarPattern.matcher(envValue);
-            } else {
-                matcher.reset(envValue);
+        int i = 0;
+        while (true) {
+            final String envKey = "env" + i;
+            i += 1;
+            if (!unsafeIntent.hasExtra(envKey)) {
+                break;
             }
 
-            if (matcher.matches()) {
-                final String envVarName = matcher.group(1);
-                final String envVarValue = matcher.group(2);
-                out.put(envVarName, envVarValue);
-            }
-            envValue = safeIntent.getStringExtra("env" + i);
-            i += 1;
+            maybeAddEnvVarToEnvVarMap(out, safeIntent, envKey, matcher);
         }
         return out;
     }
 
+    /**
+     * @param envVarMap the map to add the env var to
+     * @param intent the intent from which to extract the env var
+     * @param envKey the key at which the env var resides
+     * @param envVarMatcher a matcher initialized with the env var pattern to extract
+     */
+    private static void maybeAddEnvVarToEnvVarMap(@NonNull final HashMap<String, String> envVarMap,
+            @NonNull final SafeIntent intent, @NonNull final String envKey, @NonNull final Matcher envVarMatcher) {
+        final String envValue = intent.getStringExtra(envKey);
+        if (envValue == null) {
+            return; // nothing to do here!
+        }
+
+        envVarMatcher.reset(envValue);
+        if (envVarMatcher.matches()) {
+            final String envVarName = envVarMatcher.group(1);
+            final String envVarValue = envVarMatcher.group(2);
+            envVarMap.put(envVarName, envVarValue);
+        }
+    }
+
     public static Bundle getBundleExtraSafe(final Intent intent, final String name) {
         return new SafeIntent(intent).getBundleExtra(name);
     }
 
     public static String getStringExtraSafe(final Intent intent, final String name) {
         return new SafeIntent(intent).getStringExtra(name);
     }