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 466647 e1415ba91dd2620247fab41c5472f42b33cbc299
parent 466646 16d0d9e90e254865c501d1fcb924a103305dcd53
child 466648 985a0c1baa892278e078fb4d796fc87a730aaeb4
push id42948
push userbmo:gasolin@mozilla.com
push dateThu, 26 Jan 2017 07:49:21 +0000
reviewersWaldo
bugs1328386
milestone54.0a1
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",