Bug 1688198 - Symbol.toStringTag is incorrectly defined on the global object, rather than on its prototype. r=edgar.

For [Global] WebIDL interfaces, properties from the property array are installed
on the object itself, instead of on the prototype. To fix the bug I changed how
we install the @@toStringTag property, instead of adding it in the property
array of every interface we'll now instead install the property directly in
CreateInterfacePrototypeObject, which is also a codesize win. This does mean
that we need to look up the value dynamically in XrayResolveOwnProperty (we
can't resolve it from the property array anymore), but luckily we can use
NamesOfInterfacesWithProtos for that.

Differential Revision: https://phabricator.services.mozilla.com/D113664

dom/bindings/BindingUtils.cpp

@@ -789,24 +789,34 @@ static JSObject* CreateConstructor(JSContext* cx, JS::Handle<JSObject*> global,
 }
 
 static bool DefineConstructor(JSContext* cx, JS::Handle<JSObject*> global,
-                              const char* name,
+                              JS::Handle<jsid> name,
                               JS::Handle<JSObject*> constructor) {
   bool alreadyDefined;
-  if (!JS_AlreadyHasOwnProperty(cx, global, name, &alreadyDefined)) {
+  if (!JS_AlreadyHasOwnPropertyById(cx, global, name, &alreadyDefined)) {
     return false;
   }
 
   // This is Enumerable: False per spec.
   return alreadyDefined ||
-         JS_DefineProperty(cx, global, name, constructor, JSPROP_RESOLVING);
+         JS_DefinePropertyById(cx, global, name, constructor, JSPROP_RESOLVING);
 }
 
+static bool DefineConstructor(JSContext* cx, JS::Handle<JSObject*> global,
+                              const char* name,
+                              JS::Handle<JSObject*> constructor) {
+  PinnedStringId nameStr;
+  return nameStr.init(cx, name) &&
+         DefineConstructor(cx, global, nameStr, constructor);
+}
+
+// name must be a pinned string (or JS::PropertyKey::fromPinnedString will
+// assert).
 static JSObject* CreateInterfaceObject(
     JSContext* cx, JS::Handle<JSObject*> global,
     JS::Handle<JSObject*> constructorProto, const JSClass* constructorClass,
     unsigned ctorNargs, const LegacyFactoryFunction* namedConstructors,
     JS::Handle<JSObject*> proto, const NativeProperties* properties,
-    const NativeProperties* chromeOnlyProperties, const char* name,
+    const NativeProperties* chromeOnlyProperties, JS::Handle<JSString*> name,
     bool isChrome, bool defineOnGlobal, const char* const* legacyWindowAliases,
     bool isNamespace) {
   JS::Rooted<JSObject*> constructor(cx);
@@ -824,15 +834,7 @@ static JSObject* CreateInterfaceObject(
       return nullptr;
     }
 
-    // Might as well intern, since we're going to need an atomized
-    // version of name anyway when we stick our constructor on the
-    // global.
-    JS::Rooted<JSString*> nameStr(cx, JS_AtomizeAndPinString(cx, name));
-    if (!nameStr) {
-      return nullptr;
-    }
-
-    if (!JS_DefineProperty(cx, constructor, "name", nameStr, JSPROP_READONLY)) {
+    if (!JS_DefineProperty(cx, constructor, "name", name, JSPROP_READONLY)) {
       return nullptr;
     }
   }
@@ -899,7 +901,8 @@ static JSObject* CreateInterfaceObject(
     return nullptr;
   }
 
-  if (defineOnGlobal && !DefineConstructor(cx, global, name, constructor)) {
+  JS::Rooted<jsid> nameStr(cx, JS::PropertyKey::fromPinnedString(name));
+  if (defineOnGlobal && !DefineConstructor(cx, global, nameStr, constructor)) {
     return nullptr;
   }
 
@@ -940,7 +943,8 @@ static JSObject* CreateInterfacePrototypeObject(
     JS::Handle<JSObject*> parentProto, const JSClass* protoClass,
     const NativeProperties* properties,
     const NativeProperties* chromeOnlyProperties,
-    const char* const* unscopableNames, bool isGlobal) {
+    const char* const* unscopableNames, JS::Handle<JSString*> name,
+    bool isGlobal) {
   JS::Rooted<JSObject*> ourProto(
       cx, JS_NewObjectWithGivenProto(cx, protoClass, parentProto));
   if (!ourProto ||
@@ -974,6 +978,13 @@ static JSObject* CreateInterfacePrototypeObject(
     }
   }
 
+  JS::Rooted<jsid> toStringTagId(cx, SYMBOL_TO_JSID(JS::GetWellKnownSymbol(
+                                         cx, JS::SymbolCode::toStringTag)));
+  if (!JS_DefinePropertyById(cx, ourProto, toStringTagId, name,
+                             JSPROP_READONLY)) {
+    return nullptr;
+  }
+
   return ourProto;
 }
 
@@ -1042,8 +1053,6 @@ void CreateInterfaceObjects(
                  chromeOnlyProperties->HasStaticAttributes()))) ||
                  constructorClass,
              "Static methods but no constructorClass!");
-  MOZ_ASSERT(bool(name) == bool(constructorClass),
-             "Must have name precisely when we have an interface object");
   MOZ_ASSERT(!protoClass == !protoCache,
              "If, and only if, there is an interface prototype object we need "
              "to cache it");
@@ -1056,11 +1065,20 @@ void CreateInterfaceObjects(
 
   bool isChrome = nsContentUtils::ThreadsafeIsSystemCaller(cx);
 
+  // Might as well intern, since we're going to need an atomized
+  // version of name anyway when we stick our constructor on the
+  // global.
+  JS::Rooted<JSString*> nameStr(cx, JS_AtomizeAndPinString(cx, name));
+  if (!nameStr) {
+    return;
+  }
+
   JS::Rooted<JSObject*> proto(cx);
   if (protoClass) {
     proto = CreateInterfacePrototypeObject(
         cx, global, protoProto, protoClass, properties,
-        isChrome ? chromeOnlyProperties : nullptr, unscopableNames, isGlobal);
+        isChrome ? chromeOnlyProperties : nullptr, unscopableNames, nameStr,
+        isGlobal);
     if (!proto) {
       return;
     }
@@ -1074,7 +1092,7 @@ void CreateInterfaceObjects(
   if (constructorClass) {
     interface = CreateInterfaceObject(
         cx, global, constructorProto, constructorClass, ctorNargs,
-        namedConstructors, proto, properties, chromeOnlyProperties, name,
+        namedConstructors, proto, properties, chromeOnlyProperties, nameStr,
         isChrome, defineOnGlobal, legacyWindowAliases, isNamespace);
     if (!interface) {
       if (protoCache) {
@@ -1464,13 +1482,7 @@ static bool XrayResolveAttribute(JSContext* cx, JS::Handle<JSObject*> wrapper,
     return true;
   }
 
-  if (!attrSpec.isAccessor()) {
-    MOZ_ASSERT(id.isWellKnownSymbol(JS::SymbolCode::toStringTag));
-
-    desc.setAttributes(attrSpec.attributes());
-    desc.object().set(wrapper);
-    return attrSpec.getValue(cx, desc.value());
-  }
+  MOZ_ASSERT(attrSpec.isAccessor());
 
   MOZ_ASSERT(
       !attrSpec.isSelfHosted(),
@@ -1778,6 +1790,24 @@ static bool ResolvePrototypeOrConstructor(
                                            0, desc, cacheOnHolder);
     }
 
+    if (id.isWellKnownSymbol(JS::SymbolCode::toStringTag)) {
+      const JSClass* objClass = JS::GetClass(obj);
+      prototypes::ID prototypeID =
+          DOMIfaceAndProtoJSClass::FromJSClass(objClass)->mPrototypeID;
+      JS::Rooted<JSString*> nameStr(
+          cx, JS_AtomizeString(cx, NamesOfInterfacesWithProtos(prototypeID)));
+      if (!nameStr) {
+        return false;
+      }
+
+      desc.value().setString(nameStr);
+      desc.setAttributes(JSPROP_READONLY);
+      desc.object().set(wrapper);
+      desc.setSetter(nullptr);
+      desc.setGetter(nullptr);
+      return true;
+    }
+
     // The properties for globals live on the instance, so return here as there
     // are no properties on their interface prototype object.
     if (type == eGlobalInterfacePrototype) {

dom/bindings/BindingUtils.h

@@ -706,6 +706,14 @@ struct LegacyFactoryFunction {
  *                  on objects in chrome compartments. This must be null if the
  *                  interface doesn't have any ChromeOnly properties or if the
  *                  object is being created in non-chrome compartment.
+ * name the name to use for 1) the WebIDL class string, which is the value
+ *      that's used for @@toStringTag, 2) the name property for interface
+ *      objects and 3) the property on the global object that would be set to
+ *      the interface object. In general this is the interface identifier.
+ *      LegacyNamespace would expect something different for 1), but we don't
+ *      support that. The class string for default iterator objects is not
+ *      usable as 2) or 3), but default iterator objects don't have an interface
+ *      object.
  * defineOnGlobal controls whether properties should be defined on the given
  *                global for the interface object (if any) and named
  *                constructors (if any) for this interface.  This can be

dom/bindings/Codegen.py

@@ -3086,15 +3086,6 @@ class AttrDefiner(PropertyDefiner):
         self.regular = [m for m in attributes if not isChromeOnly(m["attr"])]
         self.static = static
 
-        if (
-            not static
-            and not unforgeable
-            and descriptor.interface.hasInterfacePrototypeObject()
-        ):
-            self.regular.append(
-                {"name": "@@toStringTag", "attr": None, "flags": "JSPROP_READONLY"}
-            )
-
         if static:
             if not descriptor.interface.hasInterfaceObject():
                 # static attributes go on the interface object
@@ -3118,15 +3109,10 @@ class AttrDefiner(PropertyDefiner):
 
     @staticmethod
     def condition(m, d):
-        if m["name"] == "@@toStringTag":
-            return MemberCondition()
         return PropertyDefiner.getControllingCondition(m["attr"], d)
 
     @staticmethod
     def specData(entry, descriptor, static=False, crossOriginOnly=False):
-        if entry["name"] == "@@toStringTag":
-            return (entry["name"], entry["flags"], descriptor.interface.getClassName())
-
         def getter(attr):
             if crossOriginOnly and not attr.getExtendedAttribute("CrossOriginReadable"):
                 return "nullptr, nullptr"
@@ -3222,13 +3208,6 @@ class AttrDefiner(PropertyDefiner):
 
     @staticmethod
     def formatSpec(fields):
-        if fields[0] == "@@toStringTag":
-            return '  JS_STRING_SYM_PS(%s, "%s", %s)' % (
-                fields[0][2:],
-                fields[2],
-                fields[1],
-            )
-
         return '  JSPropertySpec::nativeAccessors("%s", %s, %s, %s)' % fields
 
     def generateArray(self, array, name):
@@ -3714,6 +3693,15 @@ class CGCreateInterfaceObjectsMethod(CGAbstractMethod):
         else:
             chromeProperties = "nullptr"
 
+        # We use getClassName here. This should be the right thing to pass as
+        # the name argument to CreateInterfaceObjects. This is generally the
+        # interface identifier, except for the synthetic interfaces created for
+        # the default iterator objects. If needInterfaceObject is true then
+        # we'll use the name to install a property on the global object, so
+        # there shouldn't be any spaces in the name.
+        name = self.descriptor.interface.getClassName()
+        assert not (needInterfaceObject and " " in name)
+
         call = fill(
             """
             JS::Heap<JSObject*>* protoCache = ${protoCache};
@@ -3724,7 +3712,7 @@ class CGCreateInterfaceObjectsMethod(CGAbstractMethod):
                                         interfaceCache,
                                         ${properties},
                                         ${chromeProperties},
-                                        ${name}, aDefineOnGlobal,
+                                        "${name}", aDefineOnGlobal,
                                         ${unscopableNames},
                                         ${isGlobal},
                                         ${legacyWindowAliases},
@@ -3740,9 +3728,7 @@ class CGCreateInterfaceObjectsMethod(CGAbstractMethod):
             interfaceCache=interfaceCache,
             properties=properties,
             chromeProperties=chromeProperties,
-            name='"' + self.descriptor.interface.identifier.name + '"'
-            if needInterfaceObject
-            else "nullptr",
+            name=name,
             unscopableNames="unscopableNames" if self.haveUnscopables else "nullptr",
             isGlobal=toStringBool(isGlobal),
             legacyWindowAliases="legacyWindowAliases"

dom/bindings/parser/WebIDL.py

@@ -994,6 +994,9 @@ class IDLInterfaceOrNamespace(IDLInterfaceOrInterfaceMixinOrNamespace):
     def isIteratorInterface(self):
         return self.iterableInterface is not None
 
+    def getClassName(self):
+        return self.identifier.name
+
     def finish(self, scope):
         if self._finished:
             return
@@ -1811,7 +1814,7 @@ class IDLInterface(IDLInterfaceOrNamespace):
     def getClassName(self):
         if self.classNameOverride:
             return self.classNameOverride
-        return self.identifier.name
+        return IDLInterfaceOrNamespace.getClassName(self)
 
     def addExtendedAttributes(self, attrs):
         for attr in attrs: