Bug 1328386 - Part 4: No longer allow to initialize arbitrary objects as Intl.Collator instances per ECMA-402, 2nd edition. r=Waldo
authorAndré Bargull <andre.bargull@gmail.com>
Mon, 23 Jan 2017 08:33:37 -0800
changeset 331046 e1415ba91dd2620247fab41c5472f42b33cbc299
parent 331045 16d0d9e90e254865c501d1fcb924a103305dcd53
child 331047 985a0c1baa892278e078fb4d796fc87a730aaeb4
push id86156
push userryanvm@gmail.com
push dateWed, 25 Jan 2017 22:49:02 +0000
treeherdermozilla-inbound@f35fe2367a4d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1328386
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1328386 - Part 4: No longer allow to initialize arbitrary objects as Intl.Collator instances per ECMA-402, 2nd edition. r=Waldo
js/src/builtin/Intl.cpp
js/src/builtin/Intl.js
js/src/jit/InlinableNatives.h
js/src/jit/MCallOptimize.cpp
js/src/tests/Intl/Collator/call.js
js/src/tests/jstests.list
js/src/vm/SelfHosting.cpp
--- a/js/src/builtin/Intl.cpp
+++ b/js/src/builtin/Intl.cpp
@@ -972,94 +972,63 @@ static const JSPropertySpec collator_pro
 };
 
 /**
  * 10.1.2 Intl.Collator([ locales [, options]])
  *
  * ES2017 Intl draft rev 94045d234762ad107a3d09bb6f7381a65f1a2f9b
  */
 static bool
-Collator(JSContext* cx, const CallArgs& args, bool construct)
+Collator(JSContext* cx, const CallArgs& args)
 {
-    RootedObject obj(cx);
-
-    // We're following ECMA-402 1st Edition when Collator is called because of
-    // backward compatibility issues.
-    // See https://github.com/tc39/ecma402/issues/57
-    if (!construct) {
-        // ES Intl 1st ed., 10.1.2.1 step 3
-        JSObject* intl = GlobalObject::getOrCreateIntlObject(cx, cx->global());
-        if (!intl)
+    // Step 1 (Handled by OrdinaryCreateFromConstructor fallback code).
+
+    // Steps 2-5 (Inlined 9.1.14, OrdinaryCreateFromConstructor).
+    RootedObject proto(cx);
+    if (args.isConstructing() && !GetPrototypeFromCallableConstructor(cx, args, &proto))
+        return false;
+
+    if (!proto) {
+        proto = GlobalObject::getOrCreateCollatorPrototype(cx, cx->global());
+        if (!proto)
             return false;
-        RootedValue self(cx, args.thisv());
-        if (!self.isUndefined() && (!self.isObject() || self.toObject() != *intl)) {
-            // ES Intl 1st ed., 10.1.2.1 step 4
-            obj = ToObject(cx, self);
-            if (!obj)
-                return false;
-
-            // ES Intl 1st ed., 10.1.2.1 step 5
-            bool extensible;
-            if (!IsExtensible(cx, obj, &extensible))
-                return false;
-            if (!extensible)
-                return Throw(cx, obj, JSMSG_OBJECT_NOT_EXTENSIBLE);
-        } else {
-            // ES Intl 1st ed., 10.1.2.1 step 3.a
-            construct = true;
-        }
     }
 
-    if (construct) {
-        // Steps 2-5 (Inlined 9.1.14, OrdinaryCreateFromConstructor).
-        RootedObject proto(cx);
-        if (args.isConstructing() && !GetPrototypeFromCallableConstructor(cx, args, &proto))
-            return false;
-
-        if (!proto) {
-            proto = GlobalObject::getOrCreateCollatorPrototype(cx, cx->global());
-            if (!proto)
-                return false;
-        }
-
-        obj = NewObjectWithGivenProto<CollatorObject>(cx, proto);
-        if (!obj)
-            return false;
-
-        obj->as<CollatorObject>().setReservedSlot(CollatorObject::UCOLLATOR_SLOT,
-                                                  PrivateValue(nullptr));
-    }
+    Rooted<CollatorObject*> collator(cx, NewObjectWithGivenProto<CollatorObject>(cx, proto));
+    if (!collator)
+        return false;
+
+    collator->setReservedSlot(CollatorObject::UCOLLATOR_SLOT, PrivateValue(nullptr));
 
     RootedValue locales(cx, args.get(0));
     RootedValue options(cx, args.get(1));
 
     // Step 6.
-    if (!IntlInitialize(cx, obj, cx->names().InitializeCollator, locales, options))
+    if (!IntlInitialize(cx, collator, cx->names().InitializeCollator, locales, options))
         return false;
 
-    args.rval().setObject(*obj);
+    args.rval().setObject(*collator);
     return true;
 }
 
 static bool
 Collator(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
-    return Collator(cx, args, args.isConstructing());
+    return Collator(cx, args);
 }
 
 bool
 js::intl_Collator(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     MOZ_ASSERT(args.length() == 2);
     MOZ_ASSERT(!args.isConstructing());
-    // intl_Collator is an intrinsic for self-hosted JavaScript, so it cannot
-    // be used with "new", but it still has to be treated as a constructor.
-    return Collator(cx, args, true);
+
+    return Collator(cx, args);
 }
 
 void
 CollatorObject::finalize(FreeOp* fop, JSObject* obj)
 {
     MOZ_ASSERT(fop->onMainThread());
 
     // This is-undefined check shouldn't be necessary, but for internal
@@ -1197,17 +1166,17 @@ js::intl_availableCollations(JSContext* 
     return true;
 }
 
 /**
  * Returns a new UCollator with the locale and collation options
  * of the given Collator.
  */
 static UCollator*
-NewUCollator(JSContext* cx, HandleObject collator)
+NewUCollator(JSContext* cx, Handle<CollatorObject*> collator)
 {
     RootedValue value(cx);
 
     RootedObject internals(cx, GetInternals(cx, collator));
     if (!internals)
         return nullptr;
 
     if (!GetProperty(cx, internals, internals, cx->names().locale, &value))
@@ -1375,56 +1344,33 @@ bool
 js::intl_CompareStrings(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     MOZ_ASSERT(args.length() == 3);
     MOZ_ASSERT(args[0].isObject());
     MOZ_ASSERT(args[1].isString());
     MOZ_ASSERT(args[2].isString());
 
-    RootedObject collator(cx, &args[0].toObject());
-
-    // Obtain a UCollator object, cached if possible.
+    Rooted<CollatorObject*> collator(cx, &args[0].toObject().as<CollatorObject>());
+
+    // Obtain a cached UCollator object.
     // XXX Does this handle Collator instances from other globals correctly?
-    bool isCollatorInstance = collator->is<CollatorObject>();
-    UCollator* coll;
-    if (isCollatorInstance) {
-        void* priv =
-            collator->as<CollatorObject>().getReservedSlot(CollatorObject::UCOLLATOR_SLOT)
-                                          .toPrivate();
-        coll = static_cast<UCollator*>(priv);
-        if (!coll) {
-            coll = NewUCollator(cx, collator);
-            if (!coll)
-                return false;
-            collator->as<CollatorObject>().setReservedSlot(CollatorObject::UCOLLATOR_SLOT,
-                                                           PrivateValue(coll));
-        }
-    } else {
-        // There's no good place to cache the ICU collator for an object
-        // that has been initialized as a Collator but is not a Collator
-        // instance. One possibility might be to add a Collator instance as an
-        // internal property to each such object.
+    void* priv = collator->getReservedSlot(CollatorObject::UCOLLATOR_SLOT).toPrivate();
+    UCollator* coll = static_cast<UCollator*>(priv);
+    if (!coll) {
         coll = NewUCollator(cx, collator);
         if (!coll)
             return false;
+        collator->setReservedSlot(CollatorObject::UCOLLATOR_SLOT, PrivateValue(coll));
     }
 
     // Use the UCollator to actually compare the strings.
     RootedString str1(cx, args[1].toString());
     RootedString str2(cx, args[2].toString());
-    RootedValue result(cx);
-    bool success = intl_CompareStrings(cx, coll, str1, str2, &result);
-
-    if (!isCollatorInstance)
-        ucol_close(coll);
-    if (!success)
-        return false;
-    args.rval().set(result);
-    return true;
+    return intl_CompareStrings(cx, coll, str1, str2, args.rval());
 }
 
 
 /******************** NumberFormat ********************/
 
 const ClassOps NumberFormatObject::classOps_ = {
     nullptr, /* addProperty */
     nullptr, /* delProperty */
--- a/js/src/builtin/Intl.js
+++ b/js/src/builtin/Intl.js
@@ -1393,18 +1393,17 @@ var collatorKeyMappings = {
     kn: {property: "numeric", type: "boolean"},
     kf: {property: "caseFirst", type: "string", values: ["upper", "lower", "false"]}
 };
 
 
 /**
  * Compute an internal properties object from |lazyCollatorData|.
  */
-function resolveCollatorInternals(lazyCollatorData)
-{
+function resolveCollatorInternals(lazyCollatorData) {
     assert(IsObject(lazyCollatorData), "lazy data not an object?");
 
     var internalProps = std_Object_create(null);
 
     // Step 7.
     internalProps.usage = lazyCollatorData.usage;
 
     // Step 8.
@@ -1480,20 +1479,22 @@ function resolveCollatorInternals(lazyCo
 
     // The caller is responsible for associating |internalProps| with the right
     // object using |setInternalProperties|.
     return internalProps;
 }
 
 
 /**
- * Returns an object containing the Collator internal properties of |obj|, or
- * throws a TypeError if |obj| isn't Collator-initialized.
+ * Returns an object containing the Collator internal properties of |obj|.
  */
 function getCollatorInternals(obj, methodName) {
+    assert(IsObject(obj), "getCollatorInternals called with non-object");
+    assert(IsCollator(obj), "getCollatorInternals called with non-Collator");
+
     var internals = getIntlObjectInternals(obj, "Collator", methodName);
     assert(internals.type === "Collator", "bad type escaped getIntlObjectInternals");
 
     // If internal properties have already been computed, use them.
     var internalProps = maybeInternalProperties(internals);
     if (internalProps)
         return internalProps;
 
@@ -1511,21 +1512,22 @@ function getCollatorInternals(obj, metho
  * as a *lazy* concept.  Everything that must happen now, does -- but we defer
  * all the work we can until the object is actually used as a Collator.  This
  * later work occurs in |resolveCollatorInternals|; steps not noted here occur
  * there.
  *
  * Spec: ECMAScript Internationalization API Specification, 10.1.1.
  */
 function InitializeCollator(collator, locales, options) {
-    assert(IsObject(collator), "InitializeCollator");
-
-    // Step 1.
-    if (isInitializedIntlObject(collator))
-        ThrowTypeError(JSMSG_INTL_OBJECT_REINITED);
+    assert(IsObject(collator), "InitializeCollator called with non-object");
+    assert(IsCollator(collator), "InitializeCollator called with non-Collator");
+
+    // Steps 1-2 (These steps are no longer required and should be removed
+    // from the spec; https://github.com/tc39/ecma402/issues/115).
+    assert(!isInitializedIntlObject(collator), "collator mustn't be initialized");
 
     // Step 2.
     var internals = initializeIntlObject(collator);
 
     // Lazy Collator data has the following structure:
     //
     //   {
     //     requestedLocales: List of locales,
@@ -1684,24 +1686,27 @@ function collatorCompareToBind(x, y) {
  * and returns a number less than 0 if x < y, 0 if x = y, or a number greater
  * than 0 if x > y according to the sort order for the locale and collation
  * options of this Collator object.
  *
  * Spec: ECMAScript Internationalization API Specification, 10.3.2.
  */
 function Intl_Collator_compare_get() {
     // Check "this Collator object" per introduction of section 10.3.
+    if (!IsObject(this) || !IsCollator(this))
+        ThrowTypeError(JSMSG_INTL_OBJECT_NOT_INITED, "Collator", "compare", "Collator");
+
     var internals = getCollatorInternals(this, "compare");
 
     // Step 1.
     if (internals.boundCompare === undefined) {
         // Step 1.a.
         var F = collatorCompareToBind;
 
-        // Step 1.b-d.
+        // Steps 1.b-d.
         var bc = callFunction(FunctionBind, F, this);
         internals.boundCompare = bc;
     }
 
     // Step 2.
     return internals.boundCompare;
 }
 _SetCanonicalName(Intl_Collator_compare_get, "get compare");
@@ -1709,16 +1714,19 @@ function Intl_Collator_compare_get() {
 
 /**
  * Returns the resolved options for a Collator object.
  *
  * Spec: ECMAScript Internationalization API Specification, 10.3.3 and 10.4.
  */
 function Intl_Collator_resolvedOptions() {
     // Check "this Collator object" per introduction of section 10.3.
+    if (!IsObject(this) || !IsCollator(this))
+        ThrowTypeError(JSMSG_INTL_OBJECT_NOT_INITED, "Collator", "resolvedOptions", "Collator");
+
     var internals = getCollatorInternals(this, "resolvedOptions");
 
     var result = {
         locale: internals.locale,
         usage: internals.usage,
         sensitivity: internals.sensitivity,
         ignorePunctuation: internals.ignorePunctuation
     };
--- a/js/src/jit/InlinableNatives.h
+++ b/js/src/jit/InlinableNatives.h
@@ -23,16 +23,17 @@
     _(AtomicsStore)                 \
     _(AtomicsAdd)                   \
     _(AtomicsSub)                   \
     _(AtomicsAnd)                   \
     _(AtomicsOr)                    \
     _(AtomicsXor)                   \
     _(AtomicsIsLockFree)            \
                                     \
+    _(IntlIsCollator)               \
     _(IntlIsPluralRules)            \
                                     \
     _(MathAbs)                      \
     _(MathFloor)                    \
     _(MathCeil)                     \
     _(MathRound)                    \
     _(MathClz32)                    \
     _(MathSqrt)                     \
--- a/js/src/jit/MCallOptimize.cpp
+++ b/js/src/jit/MCallOptimize.cpp
@@ -104,16 +104,18 @@ IonBuilder::inlineNativeCall(CallInfo& c
       case InlinableNative::AtomicsAnd:
       case InlinableNative::AtomicsOr:
       case InlinableNative::AtomicsXor:
         return inlineAtomicsBinop(callInfo, inlNative);
       case InlinableNative::AtomicsIsLockFree:
         return inlineAtomicsIsLockFree(callInfo);
 
       // Intl natives.
+      case InlinableNative::IntlIsCollator:
+        return inlineHasClass(callInfo, &CollatorObject::class_);
       case InlinableNative::IntlIsPluralRules:
         return inlineHasClass(callInfo, &PluralRulesObject::class_);
 
       // Math natives.
       case InlinableNative::MathAbs:
         return inlineMathAbs(callInfo);
       case InlinableNative::MathFloor:
         return inlineMathFloor(callInfo);
new file mode 100644
--- /dev/null
+++ b/js/src/tests/Intl/Collator/call.js
@@ -0,0 +1,69 @@
+// |reftest| skip-if(!this.hasOwnProperty("Intl"))
+
+function IsConstructor(o) {
+  try {
+    new (new Proxy(o, {construct: () => ({})}));
+    return true;
+  } catch (e) {
+    return false;
+  }
+}
+
+function IsObject(o) {
+    return Object(o) === o;
+}
+
+function thisValues() {
+    const intlConstructors = Object.getOwnPropertyNames(Intl).map(name => Intl[name]).filter(IsConstructor);
+
+    return [
+        // Primitive values.
+        ...[undefined, null, true, "abc", Symbol(), 123],
+
+        // Object values.
+        ...[{}, [], /(?:)/, function(){}, new Proxy({}, {})],
+
+        // Intl objects.
+        ...[].concat(...intlConstructors.map(ctor => [
+            // Instance of an Intl constructor.
+            new ctor(),
+
+            // Instance of a subclassed Intl constructor.
+            new class extends ctor {},
+
+            // Object inheriting from an Intl constructor prototype.
+            Object.create(ctor.prototype),
+
+            // Intl object not inheriting from its default prototype.
+            Object.setPrototypeOf(new ctor(), Object.prototype),
+        ])),
+    ];
+}
+
+// Invoking [[Call]] for Intl.Collator always returns a new Collator instance.
+for (let thisValue of thisValues()) {
+    let obj = Intl.Collator.call(thisValue);
+    assertEq(Object.is(obj, thisValue), false);
+    assertEq(obj instanceof Intl.Collator, true);
+
+    // Ensure Intl.[[FallbackSymbol]] wasn't installed on |thisValue|.
+    if (IsObject(thisValue))
+        assertEqArray(Object.getOwnPropertySymbols(thisValue), []);
+}
+
+// Intl.Collator doesn't use the legacy Intl constructor compromise semantics.
+for (let thisValue of thisValues()) {
+    // Ensure instanceof operator isn't invoked for Intl.Collator.
+    Object.defineProperty(Intl.Collator, Symbol.hasInstance, {
+        get() {
+            assertEq(false, true, "@@hasInstance operator called");
+        }, configurable: true
+    });
+    let obj = Intl.Collator.call(thisValue);
+    delete Intl.Collator[Symbol.hasInstance];
+    assertEq(Object.is(obj, thisValue), false);
+    assertEq(obj instanceof Intl.Collator, true);
+}
+
+if (typeof reportCompare === "function")
+    reportCompare(true, true);
--- a/js/src/tests/jstests.list
+++ b/js/src/tests/jstests.list
@@ -51,16 +51,21 @@ skip script test262/ch09/9.3/9.3.1/S9.3.
 skip script test262/ch13/13.2/S13.2.3_A1.js
 skip script test262/ch10/10.6/10.6-14-1-s.js
 skip script test262/ch10/10.6/10.6-13-b-3-s.js
 skip script test262/ch10/10.6/10.6-14-b-1-s.js
 skip script test262/ch10/10.6/10.6-14-b-4-s.js
 skip script test262/ch10/10.6/10.6-13-b-1-s.js
 skip script test262/ch10/10.6/10.6-13-b-2-s.js
 
+# ES2017 Intl legacy constructor semantics changes made these tests invalid
+# (bug 1328386).
+skip script test262/intl402/ch10/10.1/10.1.1_1.js
+skip script test262/intl402/ch10/10.1/10.1.2_a.js
+
 #######################################################################
 # Tests disabled due to jstest limitations wrt imported test262 tests #
 #######################################################################
 
 # These tests are disabled because jstest doesn't understand @negative (without
 # a pattern) yet.
 skip script test262/ch07/7.2/S7.2_A5_T1.js
 skip script test262/ch07/7.2/S7.2_A5_T2.js
--- a/js/src/vm/SelfHosting.cpp
+++ b/js/src/vm/SelfHosting.cpp
@@ -2477,16 +2477,19 @@ static const JSFunctionSpec intrinsic_fu
     JS_FN("intl_NumberFormat", intl_NumberFormat, 2,0),
     JS_FN("intl_NumberFormat_availableLocales", intl_NumberFormat_availableLocales, 0,0),
     JS_FN("intl_numberingSystem", intl_numberingSystem, 1,0),
     JS_FN("intl_patternForSkeleton", intl_patternForSkeleton, 2,0),
     JS_FN("intl_PluralRules_availableLocales", intl_PluralRules_availableLocales, 0,0),
     JS_FN("intl_GetPluralCategories", intl_GetPluralCategories, 2, 0),
     JS_FN("intl_SelectPluralRule", intl_SelectPluralRule, 2,0),
 
+    JS_INLINABLE_FN("IsCollator",
+                    intrinsic_IsInstanceOfBuiltin<CollatorObject>, 1,0,
+                    IntlIsCollator),
     JS_INLINABLE_FN("IsPluralRules",
                     intrinsic_IsInstanceOfBuiltin<PluralRulesObject>, 1,0,
                     IntlIsPluralRules),
 
     JS_INLINABLE_FN("IsRegExpObject",
                     intrinsic_IsInstanceOfBuiltin<RegExpObject>, 1,0,
                     IsRegExpObject),
     JS_FN("CallRegExpMethodIfWrapped",