Bug 1319496 - 1. Fix GeckoBundle array handling; r=snorp
authorJim Chen <nchen@mozilla.com>
Tue, 29 Nov 2016 12:25:52 -0500
changeset 324717 7cf56f4e7c0de931e35f56d7220e4b99d0dd3fdf
parent 324716 23990c75a5e36eb5ecbb551cfcbcb8ae152c764d
child 324718 15407fdf9c663cfd195b28920da5a2d9084eba49
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewerssnorp
bugs1319496
milestone53.0a1
Bug 1319496 - 1. Fix GeckoBundle array handling; r=snorp Fix several bugs when handling arrays in GeckoBundle. 1. Correctly return null when getting an array that is not in the bundle, instead of crashing. 2. Convert object arrays to GeckoBundle arrays in EventDispatcher instead of leaving it as a single GeckoBundle with integer keys, due to lack of object array support in NativeJSObject.toBundle. 3. Return error when trying to convert a JS array of arrays to GeckoBundle, instead of crashing. 4. Add convenience methods for setting arrays; for example, setting boolean arrays from Boolean[] and Collection<Boolean>.
mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/GeckoBundle.java
widget/android/EventDispatcher.cpp
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
@@ -361,17 +361,17 @@ public final class EventDispatcher exten
             geckoListeners = mGeckoThreadListeners.get(type);
         }
         if (geckoListeners != null && !geckoListeners.isEmpty()) {
             final boolean onGeckoThread = ThreadUtils.isOnGeckoThread();
             final EventCallback wrappedCallback = JavaCallbackDelegate.wrap(callback);
             final GeckoBundle messageAsBundle;
             try {
                 messageAsBundle = jsMessage != null ?
-                        convertBundle(jsMessage.toBundle()) : bundleMessage;
+                        convertBundle(jsMessage.toBundle(), jsMessage) : bundleMessage;
             } catch (final NativeJSObject.InvalidPropertyException e) {
                 Log.e(LOGTAG, "Exception occurred while handling " + type, e);
                 return true;
             }
 
             for (final BundleEventListener listener : geckoListeners) {
                 // For other threads, we always dispatch asynchronously. However, for
                 // Gecko listeners only, we dispatch synchronously if we're already on
@@ -420,37 +420,47 @@ public final class EventDispatcher exten
                         "Dispatching Bundle message to Gecko listener " + type);
             }
         }
 
         return false;
     }
 
     // XXX: temporary helper for converting Bundle to GeckoBundle.
-    private GeckoBundle convertBundle(final Bundle bundle) {
+    private GeckoBundle convertBundle(final Bundle bundle, final NativeJSObject jsObj) {
         if (bundle == null) {
             return null;
         }
 
         final Set<String> keys = bundle.keySet();
         final GeckoBundle out = new GeckoBundle(keys.size());
 
         for (final String key : keys) {
             final Object value = bundle.get(key);
 
             if (value instanceof Bundle) {
-                out.putBundle(key, convertBundle((Bundle) value));
+                final Bundle bundleValue = (Bundle) value;
+                try {
+                    // XXX: NativeJSObject.toBundle doesn't support object arrays, and
+                    // instead converts it to a Bundle with integer members; correct that.
+                    final NativeJSObject[] objs = jsObj.getObjectArray(key);
+                    final GeckoBundle[] outArray = new GeckoBundle[objs.length];
+                    for (int i = 0; i < objs.length; i++) {
+                        outArray[i] = convertBundle(
+                                bundleValue.getBundle(String.valueOf(i)), objs[i]);
+                    }
+                    out.putBundleArray(key, outArray);
+
+                } catch (final Exception e) {
+                    // Not an array
+                    out.putBundle(key, convertBundle(bundleValue, jsObj.getObject(key)));
+                }
 
             } else if (value instanceof Bundle[]) {
-                final Bundle[] inArray = (Bundle[]) value;
-                final GeckoBundle[] outArray = new GeckoBundle[inArray.length];
-                for (int i = 0; i < inArray.length; i++) {
-                    outArray[i] = convertBundle(inArray[i]);
-                }
-                out.putBundleArray(key, outArray);
+                throw new IllegalStateException("toBundle should not have generated Bundle[] values");
 
             } else {
                 out.put(key, value);
             }
         }
         return out;
     }
 
@@ -475,17 +485,17 @@ public final class EventDispatcher exten
 
                 // There were native listeners, and they're gone.
                 return false;
             }
 
             final GeckoBundle messageAsBundle;
             try {
                 messageAsBundle = jsMessage != null ?
-                        convertBundle(jsMessage.toBundle()) : bundleMessage;
+                        convertBundle(jsMessage.toBundle(), jsMessage) : bundleMessage;
             } catch (final NativeJSObject.InvalidPropertyException e) {
                 Log.e(LOGTAG, "Exception occurred while handling " + type, e);
                 return true;
             }
 
             // Use a delegate to make sure callbacks happen on a specific thread.
             final EventCallback wrappedCallback = JavaCallbackDelegate.wrap(callback);
 
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/GeckoBundle.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/GeckoBundle.java
@@ -6,16 +6,17 @@
 package org.mozilla.gecko.util;
 
 import org.mozilla.gecko.annotation.RobocopTarget;
 import org.mozilla.gecko.annotation.WrapForJNI;
 
 import android.support.v4.util.SimpleArrayMap;
 
 import java.lang.reflect.Array;
+import java.util.Collection;
 import java.util.Set;
 
 /**
  * A lighter-weight version of Bundle that adds support for type coercion (e.g.
  * int to double) in order to better cooperate with JS objects.
  */
 @RobocopTarget
 public final class GeckoBundle {
@@ -133,17 +134,18 @@ public final class GeckoBundle {
      * Returns the value associated with a boolean array mapping, or null if the mapping
      * does not exist.
      *
      * @param key Key to look for.
      * @return Boolean array value
      */
     public boolean[] getBooleanArray(final String key) {
         final Object value = mMap.get(key);
-        return Array.getLength(value) == 0 ? EMPTY_BOOLEAN_ARRAY : (boolean[]) value;
+        return value == null ? null :
+                Array.getLength(value) == 0 ? EMPTY_BOOLEAN_ARRAY : (boolean[]) value;
     }
 
     /**
      * Returns the value associated with a double mapping, or defaultValue if the mapping
      * does not exist.
      *
      * @param key Key to look for.
      * @param defaultValue Value to return if mapping does not exist.
@@ -178,17 +180,17 @@ public final class GeckoBundle {
      * Returns the value associated with a double array mapping, or null if the mapping
      * does not exist.
      *
      * @param key Key to look for.
      * @return Double array value
      */
     public double[] getDoubleArray(final String key) {
         final Object value = mMap.get(key);
-        return Array.getLength(value) == 0 ? EMPTY_DOUBLE_ARRAY :
+        return value == null ? null : Array.getLength(value) == 0 ? EMPTY_DOUBLE_ARRAY :
                value instanceof int[] ? getDoubleArray((int[]) value) : (double[]) value;
     }
 
     /**
      * Returns the value associated with an int mapping, or defaultValue if the mapping
      * does not exist.
      *
      * @param key Key to look for.
@@ -224,17 +226,17 @@ public final class GeckoBundle {
      * Returns the value associated with an int array mapping, or null if the mapping does
      * not exist.
      *
      * @param key Key to look for.
      * @return Int array value
      */
     public int[] getIntArray(final String key) {
         final Object value = mMap.get(key);
-        return Array.getLength(value) == 0 ? EMPTY_INT_ARRAY :
+        return value == null ? null : Array.getLength(value) == 0 ? EMPTY_INT_ARRAY :
                value instanceof double[] ? getIntArray((double[]) value) : (int[]) value;
     }
 
     /**
      * Returns the value associated with a String mapping, or defaultValue if the mapping
      * does not exist.
      *
      * @param key Key to look for.
@@ -273,17 +275,17 @@ public final class GeckoBundle {
      * Returns the value associated with a String array mapping, or null if the mapping
      * does not exist.
      *
      * @param key Key to look for.
      * @return String array value
      */
     public String[] getStringArray(final String key) {
         final Object value = mMap.get(key);
-        return Array.getLength(value) == 0 ? EMPTY_STRING_ARRAY :
+        return value == null ? null : Array.getLength(value) == 0 ? EMPTY_STRING_ARRAY :
                !(value instanceof String[]) ? new String[getNullArrayLength(value)] :
                                               (String[]) value;
     }
 
     /**
      * Returns the value associated with a GeckoBundle mapping, or null if the mapping
      * does not exist.
      *
@@ -298,17 +300,17 @@ public final class GeckoBundle {
      * Returns the value associated with a GeckoBundle array mapping, or null if the
      * mapping does not exist.
      *
      * @param key Key to look for.
      * @return GeckoBundle array value
      */
     public GeckoBundle[] getBundleArray(final String key) {
         final Object value = mMap.get(key);
-        return Array.getLength(value) == 0 ? EMPTY_BUNDLE_ARRAY :
+        return value == null ? null : Array.getLength(value) == 0 ? EMPTY_BUNDLE_ARRAY :
                !(value instanceof GeckoBundle[]) ? new GeckoBundle[getNullArrayLength(value)] :
                                                    (GeckoBundle[]) value;
     }
 
     /**
      * Returns whether this GeckoBundle has no mappings.
      *
      * @return True if no mapping exists.
@@ -366,16 +368,53 @@ public final class GeckoBundle {
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putBooleanArray(final String key, final boolean[] value) {
         mMap.put(key, value);
     }
 
     /**
+     * Map a key to a boolean array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putBooleanArray(final String key, final Boolean[] value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final boolean[] array = new boolean[value.length];
+        for (int i = 0; i < value.length; i++) {
+            array[i] = value[i];
+        }
+        mMap.put(key, array);
+    }
+
+    /**
+     * Map a key to a boolean array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putBooleanArray(final String key, final Collection<Boolean> value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final boolean[] array = new boolean[value.size()];
+        int i = 0;
+        for (final Boolean element : value) {
+            array[i++] = element;
+        }
+        mMap.put(key, array);
+    }
+
+    /**
      * Map a key to a double value.
      *
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putDouble(final String key, final double value) {
         mMap.put(key, value);
     }
@@ -386,16 +425,53 @@ public final class GeckoBundle {
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putDoubleArray(final String key, final double[] value) {
         mMap.put(key, value);
     }
 
     /**
+     * Map a key to a double array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putDoubleArray(final String key, final Double[] value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final double[] array = new double[value.length];
+        for (int i = 0; i < value.length; i++) {
+            array[i] = value[i];
+        }
+        mMap.put(key, array);
+    }
+
+    /**
+     * Map a key to a double array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putDoubleArray(final String key, final Collection<Double> value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final double[] array = new double[value.size()];
+        int i = 0;
+        for (final Double element : value) {
+            array[i++] = element;
+        }
+        mMap.put(key, array);
+    }
+
+    /**
      * Map a key to an int value.
      *
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putInt(final String key, final int value) {
         mMap.put(key, value);
     }
@@ -406,16 +482,53 @@ public final class GeckoBundle {
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putIntArray(final String key, final int[] value) {
         mMap.put(key, value);
     }
 
     /**
+     * Map a key to a int array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putIntArray(final String key, final Integer[] value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final int[] array = new int[value.length];
+        for (int i = 0; i < value.length; i++) {
+            array[i] = value[i];
+        }
+        mMap.put(key, array);
+    }
+
+    /**
+     * Map a key to a int array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putIntArray(final String key, final Collection<Integer> value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final int[] array = new int[value.size()];
+        int i = 0;
+        for (final Integer element : value) {
+            array[i++] = element;
+        }
+        mMap.put(key, array);
+    }
+
+    /**
      * Map a key to a String value.
      *
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putString(final String key, final String value) {
         mMap.put(key, value);
     }
@@ -426,16 +539,35 @@ public final class GeckoBundle {
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putStringArray(final String key, final String[] value) {
         mMap.put(key, value);
     }
 
     /**
+     * Map a key to a String array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putStringArray(final String key, final Collection<String> value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final String[] array = new String[value.size()];
+        int i = 0;
+        for (final String element : value) {
+            array[i++] = element;
+        }
+        mMap.put(key, array);
+    }
+
+    /**
      * Map a key to a GeckoBundle value.
      *
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putBundle(final String key, final GeckoBundle value) {
         mMap.put(key, value);
     }
@@ -446,16 +578,35 @@ public final class GeckoBundle {
      * @param key Key to map.
      * @param value Value to map to.
      */
     public void putBundleArray(final String key, final GeckoBundle[] value) {
         mMap.put(key, value);
     }
 
     /**
+     * Map a key to a GeckoBundle array value.
+     *
+     * @param key Key to map.
+     * @param value Value to map to.
+     */
+    public void putBundleArray(final String key, final Collection<GeckoBundle> value) {
+        if (value == null) {
+            mMap.put(key, null);
+            return;
+        }
+        final GeckoBundle[] array = new GeckoBundle[value.size()];
+        int i = 0;
+        for (final GeckoBundle element : value) {
+            array[i++] = element;
+        }
+        mMap.put(key, array);
+    }
+
+    /**
      * Remove a mapping.
      *
      * @param key Key to remove.
      */
     public void remove(final String key) {
         mMap.remove(key);
     }
 
--- a/widget/android/EventDispatcher.cpp
+++ b/widget/android/EventDispatcher.cpp
@@ -90,36 +90,37 @@ BoxArrayPrimitive(JSContext* aCx, JS::Ha
         NS_ENSURE_TRUE((element.get().*IsType)(), NS_ERROR_INVALID_ARG);
 
         data[i] = (element.get().*ToType)();
     }
     aOut = (*NewArray)(data.get(), aLength);
     return NS_OK;
 }
 
-template<bool (JS::Value::*IsType)() const,
-         class Type,
-         nsresult (*Box)(JSContext*, JS::HandleValue, jni::Object::LocalRef&)>
+template<class Type,
+         nsresult (*Box)(JSContext*, JS::HandleValue, jni::Object::LocalRef&),
+         typename IsType>
 nsresult
 BoxArrayObject(JSContext* aCx, JS::HandleObject aData,
                jni::Object::LocalRef& aOut, size_t aLength,
-               JS::HandleValue aElement)
+               JS::HandleValue aElement,
+               IsType&& aIsType)
 {
     auto out = jni::ObjectArray::New<Type>(aLength);
     JS::RootedValue element(aCx);
     jni::Object::LocalRef jniElement(aOut.Env());
 
     nsresult rv = (*Box)(aCx, aElement, jniElement);
     NS_ENSURE_SUCCESS(rv, rv);
     out->SetElement(0, jniElement);
 
     for (size_t i = 1; i < aLength; i++) {
         NS_ENSURE_TRUE(CheckJS(aCx, JS_GetElement(aCx, aData, i, &element)),
                        NS_ERROR_FAILURE);
-        NS_ENSURE_TRUE((element.get().*IsType)() || element.isNullOrUndefined(),
+        NS_ENSURE_TRUE(element.isNullOrUndefined() || aIsType(element),
                        NS_ERROR_INVALID_ARG);
 
         rv = (*Box)(aCx, element, jniElement);
         NS_ENSURE_SUCCESS(rv, rv);
         out->SetElement(i, jniElement);
     }
     aOut = out;
     return NS_OK;
@@ -163,29 +164,41 @@ BoxArray(JSContext* aCx, JS::HandleObjec
 
     if (element.isNumber()) {
         return BoxArrayPrimitive<
                 double, &JS::Value::isNumber, &JS::Value::toNumber,
                 jni::DoubleArray, &jni::DoubleArray::New>(aCx, aData, aOut,
                                                           length, element);
     }
 
-    if (element.isString() || element.isNullOrUndefined()) {
-        nsresult rv = BoxArrayObject<&JS::Value::isString,
-                                     jni::String, &BoxString>(
-                aCx, aData, aOut, length, element);
+    if (element.isNullOrUndefined() || element.isString()) {
+        const auto isString = [] (JS::HandleValue val) -> bool {
+            return val.isString();
+        };
+        nsresult rv = BoxArrayObject<jni::String, &BoxString>(
+                aCx, aData, aOut, length, element, isString);
         if (element.isString() || rv != NS_ERROR_INVALID_ARG) {
             return rv;
         }
         // First element was null/undefined, so it may still be an object array.
     }
 
-    if (element.isObject() || element.isNullOrUndefined()) {
-        return BoxArrayObject<&JS::Value::isObject, jni::Object, &BoxObject>(
-                aCx, aData, aOut, length, element);
+    const auto isObject = [aCx] (JS::HandleValue val) -> bool {
+        if (!val.isObject()) {
+            return false;
+        }
+        bool array = false;
+        JS::RootedObject obj(aCx, &val.toObject());
+        // We don't support array of arrays.
+        return CheckJS(aCx, JS_IsArrayObject(aCx, obj, &array)) && !array;
+    };
+
+    if (element.isNullOrUndefined() || isObject(element)) {
+        return BoxArrayObject<java::GeckoBundle, &BoxObject>(
+                aCx, aData, aOut, length, element, isObject);
     }
 
     NS_WARNING("Unknown type");
     return NS_ERROR_INVALID_ARG;
 }
 
 nsresult
 BoxValue(JSContext* aCx, JS::HandleValue aData, jni::Object::LocalRef& aOut);