Bug 1550770 - Error instead of implicitly converting XPCOM interfaces to builtinclass. r=nika
authorAndrew McCreight <continuation@gmail.com>
Tue, 14 May 2019 17:39:14 +0000
changeset 535741 7005da6ab266f015214f06ba9e4f3fcf248408df
parent 535740 1db216f33d399e886e6d2e425a5e5e1019c5c704
child 535742 3cec13612ee64b570b834e0ec7b68806cc759d77
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnika
bugs1550770
milestone68.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 1550770 - Error instead of implicitly converting XPCOM interfaces to builtinclass. r=nika XPIDL has the requirement that [scriptable] interfaces with [notxpcom] methods or attributes are [builtinclass]. Currently, if you don't explicitly mark something builtinclass when it should be, then the XPIDL compiler will just silently treat it like builtinclass. This means that you can cause the JS implementation of an XPCOM to start failing without any warning by marking a method notxpcom. This patch instead makes it an error. A prior patch fixed the existing instances in the tree that relied on the implicit behavior. I also added a test that we reject such classes missing builtinclass at compile time, as well as classes that inherit from builtinclass interfaces without themselves being builtinclass. I left behind a part of the runtime test for this behavior, but now this test just ensures that you can't implement a [builtinclass] interface in JS. Differential Revision: https://phabricator.services.mozilla.com/D30984
xpcom/idl-parser/xpidl/jsonxpt.py
xpcom/idl-parser/xpidl/runtests.py
xpcom/idl-parser/xpidl/xpidl.py
xpcom/tests/NotXPCOMTest.idl
xpcom/tests/unit/test_notxpcom_scriptable.js
--- a/xpcom/idl-parser/xpidl/jsonxpt.py
+++ b/xpcom/idl-parser/xpidl/jsonxpt.py
@@ -231,17 +231,17 @@ def build_interface(iface):
     return {
         'name': iface.name,
         'uuid': iface.attributes.uuid,
         'methods': methods,
         'consts': consts,
         'parent': iface.base,
         'flags': flags(
             ('function', iface.attributes.function),
-            ('builtinclass', iface.attributes.builtinclass or iface.implicit_builtinclass),
+            ('builtinclass', iface.attributes.builtinclass),
             ('main_process_only', iface.attributes.main_process_scriptable_only),
         )
     }
 
 
 # These functions are the public interface of this module. They are very simple
 # functions, but are exported so that if we need to do something more
 # complex in them in the future we can.
--- a/xpcom/idl-parser/xpidl/runtests.py
+++ b/xpcom/idl-parser/xpidl/runtests.py
@@ -104,15 +104,66 @@ void getBar();
         self.assertTrue(isinstance(i, xpidl.IDL))
         i.resolve([], self.p, {})
 
         class FdMock:
             def write(self, s):
                 pass
         try:
             header.print_header(i, FdMock(), filename='f')
+            self.assertTrue(False, "Header printing failed to fail")
         except Exception as e:
             self.assertEqual(
                 e.args[0], "Unexpected overloaded virtual method GetBar in interface foo")
 
+    def testBuiltinClassParent(self):
+        i = self.p.parse("""
+        [scriptable, builtinclass, uuid(abc)] interface foo {};
+        [scriptable, uuid(def)] interface bar : foo {};
+        """, filename='f')
+        self.assertTrue(isinstance(i, xpidl.IDL))
+        try:
+            i.resolve([], self.p, {})
+            self.assertTrue(False,
+                            "non-builtinclasses can't inherit from builtinclasses")
+        except xpidl.IDLError as e:
+            self.assertEqual(
+                e.message,
+                "interface 'bar' is not builtinclass but derives from builtinclass 'foo'")
+
+    def testScriptableNotXPCOM(self):
+        # notxpcom method requires builtinclass on the interface
+        i = self.p.parse("""
+        [scriptable, uuid(abc)] interface nsIScriptableWithNotXPCOM {
+          [notxpcom] void method2();
+        };
+        """, filename='f')
+        self.assertTrue(isinstance(i, xpidl.IDL))
+        try:
+            i.resolve([], self.p, {})
+            self.assertTrue(False,
+                            "Resolve should fail for non-builtinclasses with notxpcom methods")
+        except xpidl.IDLError as e:
+            self.assertEqual(
+                e.message, ("scriptable interface 'nsIScriptableWithNotXPCOM' "
+                            "must be marked [builtinclass] because it contains a [notxpcom] "
+                            "method 'method2'"))
+
+        # notxpcom attribute requires builtinclass on the interface
+        i = self.p.parse("""
+        interface nsISomeInterface;
+        [scriptable, uuid(abc)] interface nsIScriptableWithNotXPCOM {
+          [notxpcom] attribute nsISomeInterface attrib;
+        };
+        """, filename='f')
+        self.assertTrue(isinstance(i, xpidl.IDL))
+        try:
+            i.resolve([], self.p, {})
+            self.assertTrue(False,
+                            "Resolve should fail for non-builtinclasses with notxpcom attributes")
+        except xpidl.IDLError as e:
+            self.assertEqual(
+                e.message, ("scriptable interface 'nsIScriptableWithNotXPCOM' must be marked "
+                            "[builtinclass] because it contains a [notxpcom] attribute 'attrib'"))
+
 
 if __name__ == '__main__':
     mozunit.main(runwith='unittest')
--- a/xpcom/idl-parser/xpidl/xpidl.py
+++ b/xpcom/idl-parser/xpidl/xpidl.py
@@ -651,31 +651,21 @@ class Interface(object):
         self.name = name
         self.attributes = InterfaceAttributes(attlist, location)
         self.base = base
         self.members = members
         self.location = location
         self.namemap = NameMap()
         self.doccomments = doccomments
         self.nativename = name
-        self.implicit_builtinclass = False
 
         for m in members:
             if not isinstance(m, CDATA):
                 self.namemap.set(m)
 
-            if ((m.kind == 'method' or m.kind == 'attribute') and
-                m.notxpcom and name != 'nsISupports'):
-                # An interface cannot be implemented by JS if it has a notxpcom
-                # method or attribute. Such a type is an "implicit builtinclass".
-                #
-                # XXX(nika): Why does nostdcall not imply builtinclass?
-                # It could screw up the shims as well...
-                self.implicit_builtinclass = True
-
     def __eq__(self, other):
         return self.name == other.name and self.location == other.location
 
     def resolve(self, parent):
         self.idl = parent
 
         # Hack alert: if an identifier is already present, libIDL assigns
         # doc comments incorrectly. This is quirks-mode extraordinaire!
@@ -710,19 +700,16 @@ class Interface(object):
                                (self.name, self.base), self.location, warning=True)
 
             if (self.attributes.scriptable and realbase.attributes.builtinclass and
                 not self.attributes.builtinclass):
                 raise IDLError("interface '%s' is not builtinclass but derives from "
                                "builtinclass '%s'" %
                                (self.name, self.base), self.location)
 
-            if realbase.implicit_builtinclass:
-                self.implicit_builtinclass = True  # Inherit implicit builtinclass from base
-
         for member in self.members:
             member.resolve(self)
 
         # The number 250 is NOT arbitrary; this number is the maximum number of
         # stub entries defined in xpcom/reflect/xptcall/genstubs.pl
         # Do not increase this value without increasing the number in that
         # location, or you WILL cause otherwise unknown problems!
         if self.countEntries() > 250 and not self.attributes.builtinclass:
@@ -958,16 +945,35 @@ class CEnum(object):
     def rustType(self, calltype):
         raise RustNoncompat('cenums unimplemented')
 
     def __str__(self):
         body = ', '.join('%s = %s' % v for v in self.variants)
         return "\tcenum %s : %d { %s };\n" % (self.name, self.width, body)
 
 
+# An interface cannot be implemented by JS if it has a notxpcom
+# method or attribute, so it must be marked as builtinclass.
+#
+# XXX(nika): Why does nostdcall not imply builtinclass?
+# It could screw up the shims as well...
+def ensureBuiltinClassIfNeeded(methodOrAttribute):
+    iface = methodOrAttribute.iface
+    if not iface.attributes.scriptable or iface.attributes.builtinclass:
+        return
+    if iface.name == 'nsISupports':
+        return
+    if methodOrAttribute.notxpcom:
+        raise IDLError(
+            ("scriptable interface '%s' must be marked [builtinclass] because it "
+             "contains a [notxpcom] %s '%s'") %
+            (iface.name, methodOrAttribute.kind, methodOrAttribute.name),
+            methodOrAttribute.location)
+
+
 class Attribute(object):
     kind = 'attribute'
     noscript = False
     notxpcom = False
     readonly = False
     symbol = False
     implicit_jscontext = False
     nostdcall = False
@@ -1028,16 +1034,18 @@ class Attribute(object):
             raise IDLError('[infallible] only works on interfaces, domobjects, and builtin types '
                            '(numbers, booleans, cenum, and raw char types)',
                            self.location)
         if self.infallible and not iface.attributes.builtinclass:
             raise IDLError('[infallible] attributes are only allowed on '
                            '[builtinclass] interfaces',
                            self.location)
 
+        ensureBuiltinClassIfNeeded(self)
+
     def toIDL(self):
         attribs = attlistToIDL(self.attlist)
         readonly = self.readonly and 'readonly ' or ''
         return "%s%sattribute %s %s;" % (attribs, readonly, self.type, self.name)
 
     def isScriptable(self):
         if not self.iface.attributes.scriptable:
             return False
@@ -1107,16 +1115,19 @@ class Method(object):
 
         self.namemap = NameMap()
         for p in paramlist:
             self.namemap.set(p)
 
     def resolve(self, iface):
         self.iface = iface
         self.realtype = self.iface.idl.getName(self.type, self.location)
+
+        ensureBuiltinClassIfNeeded(self)
+
         for p in self.params:
             p.resolve(self)
         for p in self.params:
             if p.retval and p != self.params[-1]:
                 raise IDLError("'retval' parameter '%s' is not the last parameter" %
                                p.name, self.location)
             if p.size_is:
                 found_size_param = False
--- a/xpcom/tests/NotXPCOMTest.idl
+++ b/xpcom/tests/NotXPCOMTest.idl
@@ -5,19 +5,13 @@
 #include "nsISupports.idl"
 
 [scriptable, uuid(93142a4f-e4cf-424a-b833-e638f87d2607)]
 interface nsIScriptableOK : nsISupports
 {
   void method1();
 };
 
-[scriptable, uuid(237d01a3-771e-4c6e-adf9-c97f9aab2950)]
+[scriptable, builtinclass, uuid(237d01a3-771e-4c6e-adf9-c97f9aab2950)]
 interface nsIScriptableWithNotXPCOM : nsISupports
 {
   [notxpcom] void method2();
 };
-
-[scriptable, uuid(4f06ec60-3bb3-4712-ab18-b2b595285558)]
-interface nsIScriptableWithNotXPCOMBase : nsIScriptableWithNotXPCOM
-{
-  void method3();
-};
--- a/xpcom/tests/unit/test_notxpcom_scriptable.js
+++ b/xpcom/tests/unit/test_notxpcom_scriptable.js
@@ -10,18 +10,17 @@ function run_test() {
   let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
 
   ok(Ci.nsIScriptableWithNotXPCOM);
 
   let method1Called = false;
 
   let testObject = {
     QueryInterface: ChromeUtils.generateQI([Ci.nsIScriptableOK,
-                                            Ci.nsIScriptableWithNotXPCOM,
-                                            Ci.nsIScriptableWithNotXPCOMBase]),
+                                            Ci.nsIScriptableWithNotXPCOM]),
 
     method1() {
       method1Called = true;
     },
 
     method2() {
       ok(false, "method2 should not have been called!");
     },
@@ -57,17 +56,9 @@ function run_test() {
 
   try {
     xpcomObject.QueryInterface(Ci.nsIScriptableWithNotXPCOM);
     ok(false, "Should not have implemented nsIScriptableWithNotXPCOM");
   } catch (e) {
     ok(true, "Should not have implemented nsIScriptableWithNotXPCOM. Correctly threw error: " + e);
   }
   strictEqual(xpcomObject.method2, undefined);
-
-  try {
-    xpcomObject.QueryInterface(Ci.nsIScriptableWithNotXPCOMBase);
-    ok(false, "Should not have implemented nsIScriptableWithNotXPCOMBase");
-  } catch (e) {
-    ok(true, "Should not have implemented nsIScriptableWithNotXPCOMBase. Correctly threw error: " + e);
-  }
-  strictEqual(xpcomObject.method3, undefined);
 }