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

--HG--
extra : moz-landing-system : lando

xpcom/idl-parser/xpidl/jsonxpt.py

@@ -236,7 +236,7 @@ def build_interface(iface):
         '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),
         )
     }

xpcom/idl-parser/xpidl/runtests.py

@@ -109,10 +109,61 @@ void getBar();
                 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')

xpcom/idl-parser/xpidl/xpidl.py

@@ -656,21 +656,11 @@ class Interface(object):
         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
 
@@ -715,9 +705,6 @@ class Interface(object):
                                "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)
 
@@ -963,6 +950,25 @@ class CEnum(object):
         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
@@ -1033,6 +1039,8 @@ class Attribute(object):
                            '[builtinclass] interfaces',
                            self.location)
 
+        ensureBuiltinClassIfNeeded(self)
+
     def toIDL(self):
         attribs = attlistToIDL(self.attlist)
         readonly = self.readonly and 'readonly ' or ''
@@ -1112,6 +1120,9 @@ class Method(object):
     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:

xpcom/tests/NotXPCOMTest.idl

@@ -10,14 +10,8 @@ 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();
-};

xpcom/tests/unit/test_notxpcom_scriptable.js

@@ -15,8 +15,7 @@ function run_test() {
 
   let testObject = {
     QueryInterface: ChromeUtils.generateQI([Ci.nsIScriptableOK,
-                                            Ci.nsIScriptableWithNotXPCOM,
-                                            Ci.nsIScriptableWithNotXPCOMBase]),
+                                            Ci.nsIScriptableWithNotXPCOM]),
 
     method1() {
       method1Called = true;
@@ -62,12 +61,4 @@ function run_test() {
     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);
 }