Bug 1020609 - Implement Xrays to Arrays. r=bz, a=lmandel
authorBobby Holley <bobbyholley@gmail.com>
Wed, 11 Jun 2014 15:16:07 -0700
changeset 207135 2e56e197cdbfc0d28a2b6772a0c28897491e9646
parent 207134 d3a95ceb20df6efba5d79c6d69e2638b851e11ff
child 207136 1765a031396e7d7848cda57d2f4a7c6b122c23af
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, lmandel
bugs1020609
milestone32.0a2
Bug 1020609 - Implement Xrays to Arrays. r=bz, a=lmandel
js/xpconnect/tests/chrome/test_xrayToJS.xul
js/xpconnect/wrappers/WrapperFactory.cpp
js/xpconnect/wrappers/XrayWrapper.cpp
--- a/js/xpconnect/tests/chrome/test_xrayToJS.xul
+++ b/js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ -110,16 +110,18 @@ https://bugzilla.mozilla.org/show_bug.cg
     is(global(iwin.eval(toEval)), iwin, "eval creates objects in the correct global");
     is(iwin.eval(toEval).b.foo, 'bar', "eval-ed object looks right");
     is(Cu.waiveXrays(iwin.eval(toEval)).f(), Cu.waiveXrays(iwin), "evaled function works right");
 
     testDate();
 
     testObject();
 
+    testArray();
+
     // We could also test DataView and Iterator here for completeness, but it's
     // more trouble than it's worth.
 
 
     SimpleTest.finish();
   }
 
   // Maintain a static list of the properties that are available on each standard
@@ -140,16 +142,22 @@ https://bugzilla.mozilla.org/show_bug.cg
     "toLocaleDateString", "toLocaleTimeString", "toDateString", "toTimeString",
     "toISOString", "toJSON", "toSource", "toString", "valueOf", "constructor",
     "toGMTString"];
   gPrototypeProperties['Object'] = /* __proto__ is intentionally excluded here, because
                                       the JS engine filters it out of getOwnPropertyNames */
     ["constructor", "toSource", "toString", "toLocaleString", "valueOf", "watch",
      "unwatch", "hasOwnProperty", "isPrototypeOf", "propertyIsEnumerable",
      "__defineGetter__", "__defineSetter__", "__lookupGetter__", "__lookupSetter__"];
+  gPrototypeProperties['Array'] =
+    ["length", "toSource", "toString", "toLocaleString", "join", "reverse", "sort", "push",
+      "pop", "shift", "unshift", "splice", "concat", "slice", "lastIndexOf", "indexOf",
+      "forEach", "map", "reduce", "reduceRight", "filter", "some", "every", "mapPar",
+      "reducePar", "scanPar", "scatterPar", "filterPar", "find", "findIndex", "copyWithin",
+      "fill", "@@iterator", "entries", "keys", "constructor"];
 
   function filterOut(array, props) {
     return array.filter(p => props.indexOf(p) == -1);
   }
 
   function testXray(classname, xray, xray2, propsToSkip) {
     propsToSkip = propsToSkip || [];
     let xrayProto = Object.getPrototypeOf(xray);
@@ -220,24 +228,80 @@ https://bugzilla.mozilla.org/show_bug.cg
                    primitiveProp: 42, objectProp: { foo: 2 }, \
                    xoProp: top.location, hasOwnProperty: 10, \
                    get getterProp() { return 2; }, \
                    set setterProp(x) { }, \
                    get getterSetterProp() { return 3; }, \
                    set getterSetterProp(x) { }, \
                    callableProp: function() { }, \
                    nonXrayableProp: new WeakMap() })');
+    testTrickyObject(trickyObject);
+
+  }
+
+function testArray() {
+    // The |length| property is generally very weird, especially with respect
+    // to its behavior on the prototype. Array.prototype is actually an Array
+    // instance, and therefore has a vestigial .length. But we don't want to
+    // show that over Xrays, and generally want .length to just appear as an
+    // |own| data property. So we add it to the ignore list here, and check it
+    // separately.
+    let propsToSkip = ['length'];
+    testXray('Array', new iwin.Array(20), new iwin.Array(), propsToSkip);
+
+    var trickyArray =
+      iwin.eval("var trickyArray = []; \
+                 trickyArray.primitiveProp = 42; \
+                 trickyArray.objectProp = { foo: 2 }; \
+                 trickyArray.xoProp = top.location; \
+                 trickyArray.hasOwnProperty = 10; \
+                 Object.defineProperty(trickyArray, 'getterProp', { get: function() { return 2; }}); \
+                 Object.defineProperty(trickyArray, 'setterProp', { set: function(x) {}}); \
+                 Object.defineProperty(trickyArray, 'getterSetterProp', { get: function() { return 3; }, set: function(x) {}}); \
+                 trickyArray.callableProp = function() {}; \
+                 trickyArray.nonXrayableProp = new WeakMap(); \
+                 trickyArray;");
+
+    // Test indexed access.
+    trickyArray.wrappedJSObject[9] = "some indexed property";
+    is(trickyArray[9], "some indexed property", "indexed properties work correctly over Xrays");
+    is(trickyArray.length, 10, "Length works correctly over Xrays");
+    checkThrows(function() { "use strict"; delete trickyArray.length; }, /config/, "Can't delete non-configurable 'length' property");
+    delete trickyArray[9];
+    is(trickyArray[9], undefined, "Delete works correctly over Xrays");
+    is(trickyArray.wrappedJSObject[9], undefined, "Delete works correctly over Xrays (viewed via waiver)");
+    is(trickyArray.length, 10, "length doesn't change");
+    trickyArray[11] = "some other indexed property";
+    is(trickyArray.length, 12, "length now changes");
+    is(trickyArray.wrappedJSObject[11], "some other indexed property");
+    trickyArray.length = 0;
+    is(trickyArray.length, 0, "Setting length works over Xray");
+    is(trickyArray[11], undefined, "Setting length truncates over Xray");
+    Object.defineProperty(trickyArray, 'length', { configurable: false, enumerable: false, writable: false, value: 0 });
+    trickyArray[1] = "hi";
+    is(trickyArray.length, 0, "Length remains non-writable");
+    is(trickyArray[1], undefined, "Frozen length forbids new properties");
+
+    testTrickyObject(trickyArray);
+}
+
+// Parts of this function are kind of specific to testing Object, but we factor
+// it out so that we can re-use the trickyObject stuff on Arrays.
+function testTrickyObject(trickyObject) {
 
     // Make sure it looks right under the hood.
     is(trickyObject.wrappedJSObject.getterProp, 2, "Underlying object has getter");
     is(Cu.unwaiveXrays(trickyObject.wrappedJSObject.xoProp), top.location, "Underlying object has xo property");
 
     // Test getOwnPropertyNames.
+    var expectedNames = ['objectProp', 'primitiveProp'];
+    if (trickyObject instanceof iwin.Array)
+      expectedNames.push('length');
     is(Object.getOwnPropertyNames(trickyObject).sort().toSource(),
-       ['objectProp', 'primitiveProp'].toSource(), "getOwnPropertyNames should be filtered correctly");
+       expectedNames.sort().toSource(), "getOwnPropertyNames should be filtered correctly");
 
     // Test iteration and in-place modification. Beware of 'expando', which is the property
     // we placed on the xray proto.
     var propCount = 0;
     for (let prop in trickyObject) {
       if (prop == 'primitiveProp')
         trickyObject[prop] = trickyObject[prop] - 10;
       if (prop != 'expando')
--- a/js/xpconnect/wrappers/WrapperFactory.cpp
+++ b/js/xpconnect/wrappers/WrapperFactory.cpp
@@ -117,24 +117,26 @@ WrapperFactory::DoubleWrap(JSContext *cx
         JSAutoCompartment ac(cx, obj);
         return WaiveXray(cx, obj);
     }
     return obj;
 }
 
 // In general, we're trying to deprecate COWs incrementally as we introduce
 // Xrays to the corresponding object types. But switching off COWs for Object
-// instances would be too tumultuous at present, so we punt on that for later.
+// and Array instances would be too tumultuous at present, so we punt on that
+// for later.
 static bool
 ForceCOWBehavior(JSObject *obj)
 {
-    if (IdentifyStandardInstanceOrPrototype(obj) == JSProto_Object) {
+    JSProtoKey key = IdentifyStandardInstanceOrPrototype(obj);
+    if (key == JSProto_Object || key == JSProto_Array) {
         MOZ_ASSERT(GetXrayType(obj) == XrayForJSObject,
-                   "We should use XrayWrappers for standard ES Object instances "
-                   "modulo this hack");
+                   "We should use XrayWrappers for standard ES Object and Array "
+                   "instances modulo this hack");
         return true;
     }
     return false;
 }
 
 JSObject *
 WrapperFactory::PrepareForWrapping(JSContext *cx, HandleObject scope,
                                    HandleObject objArg, unsigned flags)
--- a/js/xpconnect/wrappers/XrayWrapper.cpp
+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
@@ -41,16 +41,17 @@ using namespace XrayUtils;
 
 // Whitelist for the standard ES classes we can Xray to.
 static bool
 IsJSXraySupported(JSProtoKey key)
 {
     switch (key) {
       case JSProto_Date:
       case JSProto_Object:
+      case JSProto_Array:
         return true;
       default:
         return false;
     }
 }
 
 XrayType
 GetXrayType(JSObject *obj)
@@ -467,17 +468,17 @@ bool JSXrayTraits::getOwnPropertyFromTar
             // Note - We're going add Xrays for Arrays/TypedArrays soon in
             // bug 987163, so we don't want to cause unnecessary compat churn
             // by making xrayedObj.arrayProp stop working temporarily, and then
             // start working again. At the same time, this is an important check,
             // and this patch wouldn't be as useful without it. So we just
             // forcibly override the behavior here for Arrays until bug 987163
             // lands.
             JSProtoKey key = IdentifyStandardInstanceOrPrototype(propObj);
-            if (key != JSProto_Array && key != JSProto_Uint8ClampedArray &&
+            if (key != JSProto_Uint8ClampedArray &&
                 key != JSProto_Int8Array && key != JSProto_Uint8Array &&
                 key != JSProto_Int16Array && key != JSProto_Uint16Array &&
                 key != JSProto_Int32Array && key != JSProto_Uint32Array &&
                 key != JSProto_Float32Array && key != JSProto_Float64Array)
             {
                 return SilentFailure(cx, id, "Value not Xrayable");
             }
         }
@@ -514,20 +515,28 @@ JSXrayTraits::resolveOwnProperty(JSConte
     // Call the common code.
     bool ok = XrayTraits::resolveOwnProperty(cx, jsWrapper, wrapper, holder,
                                              id, desc);
     if (!ok || desc.object())
         return ok;
 
     RootedObject target(cx, getTargetObject(wrapper));
     if (!isPrototype(holder)) {
-        // For object instances, we expose some properties from the underlying
-        // object, but only after filtering them carefully.
+        // For Object and Array instances, we expose some properties from the
+        // underlying object, but only after filtering them carefully.
+        //
+        // Note that, as far as JS observables go, Arrays are just Objects with
+        // a different prototype and a magic (own, non-configurable) |.length| that
+        // serves as a non-tight upper bound on |own| indexed properties. So while
+        // it's tempting to try to impose some sort of structure on what Arrays
+        // "should" look like over Xrays, the underlying object is squishy enough
+        // that it makes sense to just treat them like Objects for Xray purposes.
         switch (getProtoKey(holder)) {
           case JSProto_Object:
+          case JSProto_Array:
             {
                 JSAutoCompartment ac(cx, target);
                 if (!getOwnPropertyFromTargetIfSafe(cx, target, wrapper, id, desc))
                     return false;
             }
             return JS_WrapPropertyDescriptor(cx, desc);
 
           default:
@@ -664,18 +673,20 @@ JSXrayTraits::resolveOwnProperty(JSConte
 bool
 JSXrayTraits::delete_(JSContext *cx, HandleObject wrapper, HandleId id, bool *bp)
 {
     RootedObject holder(cx, ensureHolder(cx, wrapper));
 
     // If we're using Object Xrays, we allow callers to attempt to delete any
     // property from the underlying object that they are able to resolve. Note
     // that this deleting may fail if the property is non-configurable.
-    bool isObjectInstance = getProtoKey(holder) == JSProto_Object && !isPrototype(holder);
-    if (isObjectInstance) {
+    JSProtoKey key = getProtoKey(holder);
+    bool isObjectOrArrayInstance = (key == JSProto_Object || key == JSProto_Array) &&
+                                   !isPrototype(holder);
+    if (isObjectOrArrayInstance) {
         RootedObject target(cx, getTargetObject(wrapper));
         JSAutoCompartment ac(cx, target);
         Rooted<JSPropertyDescriptor> desc(cx);
         if (!getOwnPropertyFromTargetIfSafe(cx, target, wrapper, id, &desc))
             return false;
         if (desc.object())
             return JS_DeletePropertyById2(cx, target, id, bp);
     }
@@ -690,44 +701,46 @@ JSXrayTraits::defineProperty(JSContext *
                                bool *defined)
 {
     *defined = false;
     RootedObject holder(cx, ensureHolder(cx, wrapper));
     if (!holder)
         return false;
 
 
-    // Object instances are special. For that case, we forward property
+    // Object and Array instances are special. For those cases, we forward property
     // definitions to the underlying object if the following conditions are met:
     // * The property being defined is a value-prop.
     // * The property being defined is either a primitive or subsumed by the target.
     // * As seen from the Xray, any existing property that we would overwrite is an
     //   |own| value-prop.
     //
-    // To avoid confusion, we disallow expandos on Object instances, and
+    // To avoid confusion, we disallow expandos on Object and Array instances, and
     // therefore raise an exception here if the above conditions aren't met.
-    bool isObjectInstance = getProtoKey(holder) == JSProto_Object && !isPrototype(holder);
-    if (isObjectInstance) {
+    JSProtoKey key = getProtoKey(holder);
+    bool isObjectOrArrayInstance = (key == JSProto_Object || key == JSProto_Array) &&
+                                   !isPrototype(holder);
+    if (isObjectOrArrayInstance) {
         RootedObject target(cx, getTargetObject(wrapper));
         if (desc.hasGetterOrSetter()) {
-            JS_ReportError(cx, "Not allowed to define accessor property on [Object] XrayWrapper");
+            JS_ReportError(cx, "Not allowed to define accessor property on [Object] or [Array] XrayWrapper");
             return false;
         }
         if (desc.value().isObject() &&
             !AccessCheck::subsumes(target, js::UncheckedUnwrap(&desc.value().toObject())))
         {
-            JS_ReportError(cx, "Not allowed to define cross-origin object as property on [Object] XrayWrapper");
+            JS_ReportError(cx, "Not allowed to define cross-origin object as property on [Object] or [Array] XrayWrapper");
             return false;
         }
         if (existingDesc.hasGetterOrSetter()) {
-            JS_ReportError(cx, "Not allowed to overwrite accessor property on [Object] XrayWrapper");
+            JS_ReportError(cx, "Not allowed to overwrite accessor property on [Object] or [Array] XrayWrapper");
             return false;
         }
         if (existingDesc.object() && existingDesc.object() != wrapper) {
-            JS_ReportError(cx, "Not allowed to shadow non-own Xray-resolved property on [Object] XrayWrapper");
+            JS_ReportError(cx, "Not allowed to shadow non-own Xray-resolved property on [Object] or [Array] XrayWrapper");
             return false;
         }
 
         JSAutoCompartment ac(cx, target);
         if (!JS_WrapPropertyDescriptor(cx, desc) ||
             !JS_DefinePropertyById(cx, target, id, desc.value(), desc.attributes(),
                                    JS_PropertyStub, JS_StrictPropertyStub))
         {
@@ -745,20 +758,21 @@ JSXrayTraits::enumerateNames(JSContext *
                              AutoIdVector &props)
 {
     RootedObject target(cx, getTargetObject(wrapper));
     RootedObject holder(cx, ensureHolder(cx, wrapper));
     if (!holder)
         return false;
 
     if (!isPrototype(holder)) {
-        // For object instances, we expose some properties from the underlying
+        // For Object and Array instances, we expose some properties from the underlying
         // object, but only after filtering them carefully.
         switch (getProtoKey(holder)) {
           case JSProto_Object:
+          case JSProto_Array:
             MOZ_ASSERT(props.empty());
             {
                 JSAutoCompartment ac(cx, target);
                 AutoIdVector targetProps(cx);
                 if (!js::GetPropertyNames(cx, target, flags | JSITER_OWNONLY, &targetProps))
                     return false;
                 // Loop over the properties, and only pass along the ones that
                 // we determine to be safe.