Bug 1452786 part 2. Stop using a generated chromeonly isInstance method. r=qdot
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 12 Apr 2018 00:06:07 -0400
changeset 412929 3406e123d279f0959468a4de10e8983cc3d327dc
parent 412928 f87a1de08a8ac787d53034c1f36a13946f22b8a4
child 412930 f57ccba9c9b51bf5bb1c01517c8af1f5f3ebe099
push id33824
push userebalazs@mozilla.com
push dateThu, 12 Apr 2018 09:39:57 +0000
treeherdermozilla-central@246c614e1605 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersqdot
bugs1452786
milestone61.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 1452786 part 2. Stop using a generated chromeonly isInstance method. r=qdot This changes semantics in all sorts of ways (e.g. now we get the right proto from our |this| value instead of it being baked into the function). But if all our chrome callers are well-behaved this should be ok. We _could_ bake the proto id and depth into the function itself by using js::NewFunctionWithReserved if it were not for Xrays. Those already need the reserved slots on functions we Xray. MozReview-Commit-ID: 1bYrKWWIc1P
dom/bindings/BindingUtils.cpp
dom/bindings/BindingUtils.h
dom/bindings/Codegen.py
js/xpconnect/src/XPCJSRuntime.cpp
js/xpconnect/src/xpcprivate.h
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -778,16 +778,23 @@ CreateInterfaceObject(JSContext* cx, JS:
     JS::Rooted<jsid> hasInstanceId(cx,
       SYMBOL_TO_JSID(JS::GetWellKnownSymbol(cx, JS::SymbolCode::hasInstance)));
     if (!JS_DefineFunctionById(cx, constructor, hasInstanceId,
                                InterfaceHasInstance, 1,
                                // Flags match those of Function[Symbol.hasInstance]
                                JSPROP_READONLY | JSPROP_PERMANENT)) {
       return nullptr;
     }
+
+    if (isChrome && !JS_DefineFunction(cx, constructor, "isInstance",
+                                       InterfaceHasInstance, 1,
+                                       // Don't bother making it enumerable
+                                       0)) {
+      return nullptr;
+    }
   }
 
   if (properties) {
     if (properties->HasStaticMethods() &&
         !DefinePrefable(cx, constructor, properties->StaticMethods())) {
       return nullptr;
     }
 
@@ -978,17 +985,17 @@ CreateInterfaceObjects(JSContext* cx, JS
              "to cache it");
   MOZ_ASSERT(bool(constructorClass) == bool(constructorCache),
              "If, and only if, there is an interface object we need to cache "
              "it");
   MOZ_ASSERT(constructorProto || !constructorClass,
              "Must have a constructor proto if we plan to create a constructor "
              "object");
 
-  bool isChrome = nsContentUtils::ThreadsafeIsSystemCaller(aCx);
+  bool isChrome = nsContentUtils::ThreadsafeIsSystemCaller(cx);
 
   JS::Rooted<JSObject*> proto(cx);
   if (protoClass) {
     proto =
       CreateInterfacePrototypeObject(cx, global, protoProto, protoClass,
                                      properties,
                                      isChrome ? chromeOnlyProperties : nullptr,
                                      unscopableNames, isGlobal);
@@ -1747,16 +1754,36 @@ XrayResolveOwnProperty(JSContext* cx, JS
     if (id == GetJSIDByIndex(cx, XPCJSContext::IDX_PROTOTYPE)) {
       return nativePropertyHooks->mPrototypeID == prototypes::id::_ID_Count ||
              ResolvePrototypeOrConstructor(cx, wrapper, obj,
                                            nativePropertyHooks->mPrototypeID,
                                            JSPROP_PERMANENT | JSPROP_READONLY,
                                            desc, cacheOnHolder);
     }
 
+    if (id == GetJSIDByIndex(cx, XPCJSContext::IDX_ISINSTANCE) &&
+        DOMIfaceAndProtoJSClass::FromJSClass(js::GetObjectClass(obj))->
+          wantsInterfaceHasInstance) {
+      cacheOnHolder = true;
+      JSNativeWrapper interfaceIsInstanceWrapper = { InterfaceIsInstance,
+                                                      nullptr };
+      JSObject* funObj = XrayCreateFunction(cx, wrapper,
+                                            interfaceIsInstanceWrapper, 1, id);
+      if (!funObj) {
+        return false;
+      }
+
+      desc.value().setObject(*funObj);
+      desc.setAttributes(0);
+      desc.object().set(wrapper);
+      desc.setSetter(nullptr);
+      desc.setGetter(nullptr);
+      return true;
+    }
+
     if (id == SYMBOL_TO_JSID(JS::GetWellKnownSymbol(cx, JS::SymbolCode::hasInstance)) &&
         DOMIfaceAndProtoJSClass::FromJSClass(js::GetObjectClass(obj))->
           wantsInterfaceHasInstance) {
       cacheOnHolder = true;
       JSNativeWrapper interfaceHasInstanceWrapper = { InterfaceHasInstance,
                                                       nullptr };
       JSObject* funObj = XrayCreateFunction(cx, wrapper,
                                             interfaceHasInstanceWrapper, 1, id);
@@ -2499,43 +2526,63 @@ InterfaceHasInstance(JSContext* cx, int 
              "Why do we have a hasInstance hook if we don't have a prototype "
              "ID?");
 
   *bp = (domClass && domClass->mInterfaceChain[depth] == prototypeID);
   return true;
 }
 
 bool
-InterfaceIsInstance(JSContext* cx, unsigned argc, JS::Value* vp,
-                    prototypes::ID prototypeID, int depth)
+InterfaceIsInstance(JSContext* cx, unsigned argc, JS::Value* vp)
 {
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
-  if (MOZ_UNLIKELY(args.length() < 1)) {
-    nsPrintfCString message("%s.isInstance",
-                            NamesOfInterfacesWithProtos(prototypeID));
-    return ThrowErrorMessage(cx, MSG_MISSING_ARGUMENTS, message.get());
+
+  // If the thing we were passed is not an object, return false.
+  if (!args.get(0).isObject()) {
+    args.rval().setBoolean(false);
+    return true;
+  }
+
+  // If "this" isn't a DOM constructor or is a constructor for an interface
+  // without a prototype, return false.
+  if (!args.thisv().isObject()) {
+    args.rval().setBoolean(false);
+    return true;
   }
 
-  if (!args[0].isObject()) {
-    nsPrintfCString message("Argument 1 of %s.isInstance",
-                            NamesOfInterfacesWithProtos(prototypeID));
-    return ThrowErrorMessage(cx, MSG_NOT_OBJECT, message.get());
+  JS::Rooted<JSObject*> thisObj(cx, js::CheckedUnwrap(&args.thisv().toObject()));
+  if (!thisObj) {
+    args.rval().setBoolean(false);
+    return true;
+  }
+
+  const js::Class* thisClass = js::GetObjectClass(thisObj);
+  if (!IsDOMIfaceAndProtoClass(thisClass)) {
+    args.rval().setBoolean(false);
+    return true;
+  }
+
+  const DOMIfaceAndProtoJSClass* clasp =
+    DOMIfaceAndProtoJSClass::FromJSClass(thisClass);
+
+  if (clasp->mType != eInterface ||
+      clasp->mPrototypeID == prototypes::id::_ID_Count) {
+    args.rval().setBoolean(false);
+    return true;
   }
 
   JS::Rooted<JSObject*> instance(cx, &args[0].toObject());
-
   const DOMJSClass* domClass =
     GetDOMClass(js::UncheckedUnwrap(instance, /* stopAtWindowProxy = */ false));
 
-  if (domClass && domClass->mInterfaceChain[depth] == prototypeID) {
-    args.rval().setBoolean(true);
-    return true;
-  }
-
-  args.rval().setBoolean(false);
+  bool isInstance =
+    domClass &&
+    domClass->mInterfaceChain[clasp->mDepth] == clasp->mPrototypeID;
+
+  args.rval().setBoolean(isInstance);
   return true;
 }
 
 bool
 ReportLenientThisUnwrappingFailure(JSContext* cx, JSObject* obj)
 {
   JS::Rooted<JSObject*> rootedObj(cx, obj);
   GlobalObject global(cx, rootedObj);
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -2704,18 +2704,17 @@ InterfaceHasInstance(JSContext* cx, unsi
 
 bool
 InterfaceHasInstance(JSContext* cx, int prototypeID, int depth,
                      JS::Handle<JSObject*> instance,
                      bool* bp);
 
 // Used to implement the cross-context <Interface>.isInstance static method.
 bool
-InterfaceIsInstance(JSContext* cx, unsigned argc, JS::Value* vp,
-                    prototypes::ID prototypeID, int depth);
+InterfaceIsInstance(JSContext* cx, unsigned argc, JS::Value* vp);
 
 // Helper for lenient getters/setters to report to console.  If this
 // returns false, we couldn't even get a global.
 bool
 ReportLenientThisUnwrappingFailure(JSContext* cx, JSObject* obj);
 
 // Given a JSObject* that represents the chrome side of a JS-implemented WebIDL
 // interface, get the nsIGlobalObject corresponding to the content side, if any.
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -1767,33 +1767,16 @@ class CGClassObjectMovedHook(CGAbstractC
 
 
 def JSNativeArguments():
     return [Argument('JSContext*', 'cx'),
             Argument('unsigned', 'argc'),
             Argument('JS::Value*', 'vp')]
 
 
-class CGIsInstanceMethod(CGAbstractStaticMethod):
-    """
-    A class for generating the static isInstance method.
-    """
-    def __init__(self, descriptor):
-        assert descriptor.interface.hasInterfacePrototypeObject()
-        CGAbstractStaticMethod.__init__(self, descriptor, "isInstance", "bool",
-                                        JSNativeArguments())
-
-    def definition_body(self):
-        return fill(
-            """
-            return InterfaceIsInstance(cx, argc, vp, prototypes::id::${name},
-                                       PrototypeTraits<prototypes::id::${name}>::Depth);
-            """,
-            name=self.descriptor.name)
-
 class CGClassConstructor(CGAbstractStaticMethod):
     """
     JS-visible constructor for our objects
     """
     def __init__(self, descriptor, ctor, name=CONSTRUCT_HOOK_NAME):
         CGAbstractStaticMethod.__init__(self, descriptor, name, 'bool',
                                         JSNativeArguments())
         self._ctor = ctor
@@ -2331,28 +2314,16 @@ class MethodDefiner(PropertyDefiner):
             if m.isStatic():
                 method["nativeName"] = CppKeywords.checkMethodName(IDLToCIdentifier(m.identifier.name))
 
             if isChromeOnly(m):
                 self.chrome.append(method)
             else:
                 self.regular.append(method)
 
-        # Generate the isInstance static method.
-        if (static and
-            (self.descriptor.interface.hasInterfaceObject() and
-             self.descriptor.interface.hasInterfacePrototypeObject())):
-            self.chrome.append({
-                "name": "isInstance",
-                "methodInfo": False,
-                "length": 1,
-                "flags": "JSPROP_ENUMERATE",
-                "condition": MemberCondition(),
-            })
-
         # TODO: Once iterable is implemented, use tiebreak rules instead of
         # failing. Also, may be more tiebreak rules to implement once spec bug
         # is resolved.
         # https://www.w3.org/Bugs/Public/show_bug.cgi?id=28592
         def hasIterator(methods, regular):
             return (any("@@iterator" in m.aliases for m in methods) or
                     any("@@iterator" == r["name"] for r in regular))
 
@@ -2727,17 +2698,16 @@ class ConstDefiner(PropertyDefiner):
             lambda fields: '  { "%s", %s }' % fields,
             '  { 0, JS::UndefinedValue() }',
             'ConstantSpec',
             PropertyDefiner.getControllingCondition, specData)
 
 
 class PropertyArrays():
     def __init__(self, descriptor):
-        self.descriptor = descriptor
         self.staticMethods = MethodDefiner(descriptor, "StaticMethods",
                                            static=True)
         self.staticAttrs = AttrDefiner(descriptor, "StaticAttributes",
                                        static=True)
         self.methods = MethodDefiner(descriptor, "Methods", static=False)
         self.attrs = AttrDefiner(descriptor, "Attributes", static=False)
         self.unforgeableMethods = MethodDefiner(descriptor, "UnforgeableMethods",
                                                 static=False, unforgeable=True)
@@ -2746,21 +2716,17 @@ class PropertyArrays():
         self.consts = ConstDefiner(descriptor, "Constants")
 
     @staticmethod
     def arrayNames():
         return ["staticMethods", "staticAttrs", "methods", "attrs",
                 "unforgeableMethods", "unforgeableAttrs", "consts"]
 
     def hasChromeOnly(self):
-        # All interfaces that generate an interface object and interface
-        # prototype object have a chrome only isInstance static method.
-        return ((self.staticMethods.descriptor.interface.hasInterfaceObject() and
-                 self.staticMethods.descriptor.interface.hasInterfacePrototypeObject()) or
-                any(getattr(self, a).hasChromeOnly() for a in self.arrayNames()))
+        return any(getattr(self, a).hasChromeOnly() for a in self.arrayNames())
 
     def hasNonChromeOnly(self):
         return any(getattr(self, a).hasNonChromeOnly() for a in self.arrayNames())
 
     def __str__(self):
         define = ""
         for array in self.arrayNames():
             define += str(getattr(self, array))
@@ -12316,19 +12282,16 @@ class CGDescriptor(CGThing):
                                       toBindingNamespace(descriptor.parentPrototypeName)))
 
         jsonifierMethod = None
         crossOriginMethods, crossOriginGetters, crossOriginSetters = set(), set(), set()
         unscopableNames = list()
         for n in descriptor.interface.namedConstructors:
             cgThings.append(CGClassConstructor(descriptor, n,
                                                NamedConstructorName(n)))
-        if (descriptor.interface.hasInterfaceObject() and
-            descriptor.interface.hasInterfacePrototypeObject()):
-            cgThings.append(CGIsInstanceMethod(descriptor))
         for m in descriptor.interface.members:
             if m.isMethod() and m.identifier.name == 'QueryInterface':
                 continue
 
             props = memberProperties(m, descriptor)
 
             if m.isMethod():
                 if m.getExtendedAttribute("Unscopable"):
@@ -13864,23 +13827,19 @@ class CGBindingRoot(CGThing):
 
             return (any(isChromeOnly(a) or needsContainsHack(a) or
                         needsCallerType(a)
                         for a in desc.interface.members) or
                     desc.interface.getExtendedAttribute("ChromeOnly") is not None or
                     # JS-implemented interfaces with an interface object get a
                     # chromeonly _create method.  And interfaces with an
                     # interface object might have a ChromeOnly constructor.
-                    # Also interfaces whose interface prototype object is
-                    # generated (which is most of them) for the isInstance
-                    # method.
                     (desc.interface.hasInterfaceObject() and
                      (desc.interface.isJSImplemented() or
-                      (ctor and isChromeOnly(ctor)) or
-                      desc.interface.hasInterfacePrototypeObject())) or
+                      (ctor and isChromeOnly(ctor)))) or
                     # JS-implemented interfaces with clearable cached
                     # attrs have chromeonly _clearFoo methods.
                     (desc.interface.isJSImplemented() and
                      any(clearableCachedAttrs(desc))))
 
         # XXXkhuey ugly hack but this is going away soon.
         bindingHeaders['xpcprivate.h'] = webIDLFile.endswith("EventTarget.webidl")
 
--- a/js/xpconnect/src/XPCJSRuntime.cpp
+++ b/js/xpconnect/src/XPCJSRuntime.cpp
@@ -101,17 +101,18 @@ const char* const XPCJSRuntime::mStrings
     "undefined",            // IDX_UNDEFINED
     "",                     // IDX_EMPTYSTRING
     "fileName",             // IDX_FILENAME
     "lineNumber",           // IDX_LINENUMBER
     "columnNumber",         // IDX_COLUMNNUMBER
     "stack",                // IDX_STACK
     "message",              // IDX_MESSAGE
     "lastIndex",            // IDX_LASTINDEX
-    "then"                  // IDX_THEN
+    "then",                 // IDX_THEN
+    "isInstance",           // IDX_ISINSTANCE
 };
 
 /***************************************************************************/
 
 // *Some* NativeSets are referenced from mClassInfo2NativeSetMap.
 // *All* NativeSets are referenced from mNativeSetMap.
 // So, in mClassInfo2NativeSetMap we just clear references to the unmarked.
 // In mNativeSetMap we clear the references to the unmarked *and* delete them.
--- a/js/xpconnect/src/xpcprivate.h
+++ b/js/xpconnect/src/xpcprivate.h
@@ -457,16 +457,17 @@ public:
         IDX_EMPTYSTRING             ,
         IDX_FILENAME                ,
         IDX_LINENUMBER              ,
         IDX_COLUMNNUMBER            ,
         IDX_STACK                   ,
         IDX_MESSAGE                 ,
         IDX_LASTINDEX               ,
         IDX_THEN                    ,
+        IDX_ISINSTANCE              ,
         IDX_TOTAL_COUNT // just a count of the above
     };
 
     inline JS::HandleId GetStringID(unsigned index) const;
     inline const char* GetStringName(unsigned index) const;
 
     ShortLivedStringBuffer<nsString> mScratchStrings;
     ShortLivedStringBuffer<nsCString> mScratchCStrings;