Do not scamble message IDs with explicit ordinals

This prevents the bindings generator from re-assigning a message's ID to
a scrambled value whenever the message definition in mojom has an
explicit ordinal declared (i.e. @N notation).

As an added measure of stability, this also adds the requirement that
interfaces marked [Stable] MUST assign explicit ordinals to all methods.

Fixed: 1089230
Change-Id: Iaa6eff350f7ecee31feb54b19349c9caa4b2a2c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225327
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774041}

mojo/public/tools/bindings/generators/mojom_mojolpm_generator.py

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 """Generates C++ source files from a mojom.Module."""
 import os
+import sys
 from mojom_cpp_generator import _NameFormatter as CppNameFormatter
 from mojom_cpp_generator import Generator as CppGenerator
 from mojom_cpp_generator import IsNativeOnlyKind, NamespaceToArray
@@ -40,6 +41,12 @@ _kind_to_cpp_proto_type = {
 }
 
 
+def _IsStrOrUnicode(x):
+  if sys.version_info[0] < 3:
+    return isinstance(x, (unicode, str))
+  return isinstance(x, str)
+
+
 class _NameFormatter(CppNameFormatter):
   """A formatter for the names of kinds or values."""
 
@@ -479,7 +486,7 @@ class Generator(CppGenerator):
       if field.name == 'MAX':
         continue
       if field.value:
-        if isinstance(field.value, str):
+        if _IsStrOrUnicode(field.value):
           if field.value in values:
             return True
           values.add(field.value)

mojo/public/tools/bindings/mojom_bindings_generator.py

@@ -109,6 +109,8 @@ def ScrambleMethodOrdinals(interfaces, salt):
     i = 0
     already_generated.clear()
     for method in interface.methods:
+      if method.explicit_ordinal is not None:
+        continue
       while True:
         i = i + 1
         if i == 1000000:

mojo/public/tools/bindings/mojom_bindings_generator_unittest.py

@@ -9,14 +9,15 @@ from mojom_bindings_generator import ScrambleMethodOrdinals
 
 
 class FakeIface(object):
-  def __init__( self ):
-    self.name = None
+  def __init__(self):
+    self.mojom_name = None
     self.methods = None
 
 
 class FakeMethod(object):
-  def __init__( self ):
-    self.ordinal = None
+  def __init__(self, explicit_ordinal=None):
+    self.explicit_ordinal = explicit_ordinal
+    self.ordinal = explicit_ordinal
     self.ordinal_comment = None
 
 
@@ -25,18 +26,23 @@ class MojoBindingsGeneratorTest(unittest.TestCase):
 
   def testMakeImportStackMessage(self):
     """Tests MakeImportStackMessage()."""
-    self.assertEquals(MakeImportStackMessage(["x"]), "")
-    self.assertEquals(MakeImportStackMessage(["x", "y"]),
-        "\n  y was imported by x")
-    self.assertEquals(MakeImportStackMessage(["x", "y", "z"]),
-        "\n  z was imported by y\n  y was imported by x")
+    self.assertEqual(MakeImportStackMessage(["x"]), "")
+    self.assertEqual(MakeImportStackMessage(["x", "y"]),
+                     "\n  y was imported by x")
+    self.assertEqual(MakeImportStackMessage(["x", "y", "z"]),
+                     "\n  z was imported by y\n  y was imported by x")
 
   def testScrambleMethodOrdinals(self):
     """Tests ScrambleMethodOrdinals()."""
     interface = FakeIface()
-    interface.name = 'RendererConfiguration'
-    interface.methods = [FakeMethod(), FakeMethod(), FakeMethod()]
-    ScrambleMethodOrdinals([interface], "foo")
+    interface.mojom_name = 'RendererConfiguration'
+    interface.methods = [
+        FakeMethod(),
+        FakeMethod(),
+        FakeMethod(),
+        FakeMethod(explicit_ordinal=42)
+    ]
+    ScrambleMethodOrdinals([interface], "foo".encode('utf-8'))
     # These next three values are hard-coded. If the generation algorithm
     # changes from being based on sha256(seed + interface.name + str(i)) then
     # these numbers will obviously need to change too.
@@ -44,9 +50,12 @@ class MojoBindingsGeneratorTest(unittest.TestCase):
     # Note that hashlib.sha256('fooRendererConfiguration1').digest()[:4] is
     # '\xa5\xbc\xf9\xca' and that hex(1257880741) = '0x4af9bca5'. The
     # difference in 0x4a vs 0xca is because we only take 31 bits.
-    self.assertEquals(interface.methods[0].ordinal, 1257880741)
-    self.assertEquals(interface.methods[1].ordinal, 631133653)
-    self.assertEquals(interface.methods[2].ordinal, 549336076)
+    self.assertEqual(interface.methods[0].ordinal, 1257880741)
+    self.assertEqual(interface.methods[1].ordinal, 631133653)
+    self.assertEqual(interface.methods[2].ordinal, 549336076)
+
+    # Explicit method ordinals should not be scrambled.
+    self.assertEqual(interface.methods[3].ordinal, 42)
 
 
 if __name__ == "__main__":

mojo/public/tools/mojom/check_stable_mojom_compatibility.py

@@ -12,6 +12,7 @@ This can be used e.g. by a presubmit check to prevent developers from making
 breaking changes to stable mojoms."""
 
 import argparse
+import codecs
 import errno
 import json
 import os
@@ -60,10 +61,10 @@ def _ValidateDelta(root, delta):
       modules = override_modules
     else:
       modules = unmodified_modules
-      with open(os.path.join(root, mojom)) as f:
+      with codecs.open(os.path.join(root, mojom), encoding='utf-8') as f:
         contents = ''.join(f.readlines())
 
-    ast = parser.Parse(contents.encode('utf-8'), mojom)
+    ast = parser.Parse(contents, mojom)
     for imp in ast.import_list:
       parseMojom(imp.import_filename, file_overrides, override_modules)
 

mojo/public/tools/mojom/mojom/generate/generator.py

@@ -175,6 +175,11 @@ def AddComputedData(module):
       method.param_struct = _GetStructFromMethod(method)
       if interface.stable:
         method.param_struct.attributes[mojom.ATTRIBUTE_STABLE] = True
+        if method.explicit_ordinal is None:
+          raise Exception(
+              'Stable interfaces must declare explicit method ordinals. The '
+              'method %s on stable interface %s does not declare an explicit '
+              'ordinal.' % (method.mojom_name, interface.qualified_name))
       interface.version = max(interface.version,
                               method.param_struct.versions[-1].version)
 

mojo/public/tools/mojom/mojom/generate/module.py

@@ -910,6 +910,7 @@ class Method(object):
     self.interface = interface
     self.mojom_name = mojom_name
     self.name = None
+    self.explicit_ordinal = ordinal
     self.ordinal = ordinal
     self.parameters = []
     self.param_struct = None

mojo/public/tools/mojom/mojom/generate/translate.py

@@ -12,12 +12,19 @@ already been parsed and converted to ASTs before.
 import itertools
 import os
 import re
+import sys
 
 from mojom.generate import generator
 from mojom.generate import module as mojom
 from mojom.parse import ast
 
 
+def _IsStrOrUnicode(x):
+  if sys.version_info[0] < 3:
+    return isinstance(x, (unicode, str))
+  return isinstance(x, str)
+
+
 def _DuplicateName(values):
   """Returns the 'mojom_name' of the first entry in |values| whose 'mojom_name'
   has already been encountered. If there are no duplicates, returns None."""
@@ -534,7 +541,7 @@ def _ResolveNumericEnumValues(enum):
       prev_value += 1
 
     # Integral value (e.g: BEGIN = -0x1).
-    elif isinstance(field.value, str):
+    elif _IsStrOrUnicode(field.value):
       prev_value = int(field.value, 0)
 
     # Reference to a previous enum value (e.g: INIT = BEGIN).

mojo/public/tools/mojom/mojom/parse/ast.py

@@ -9,6 +9,15 @@
 # failures, especially for more complex types.
 
 
+import sys
+
+
+def _IsStrOrUnicode(x):
+  if sys.version_info[0] < 3:
+    return isinstance(x, (unicode, str))
+  return isinstance(x, str)
+
+
 class NodeBase(object):
   """Base class for nodes in the AST."""
 
@@ -87,7 +96,7 @@ class Definition(NodeBase):
   include parameter definitions.) This class is meant to be subclassed."""
 
   def __init__(self, mojom_name, **kwargs):
-    assert isinstance(mojom_name, str)
+    assert _IsStrOrUnicode(mojom_name)
     NodeBase.__init__(self, **kwargs)
     self.mojom_name = mojom_name
 
@@ -99,7 +108,7 @@ class Attribute(NodeBase):
   """Represents an attribute."""
 
   def __init__(self, key, value, **kwargs):
-    assert isinstance(key, str)
+    assert _IsStrOrUnicode(key)
     super(Attribute, self).__init__(**kwargs)
     self.key = key
     self.value = value
@@ -122,10 +131,10 @@ class Const(Definition):
   def __init__(self, mojom_name, attribute_list, typename, value, **kwargs):
     assert attribute_list is None or isinstance(attribute_list, AttributeList)
     # The typename is currently passed through as a string.
-    assert isinstance(typename, str)
+    assert _IsStrOrUnicode(typename)
     # The value is either a literal (currently passed through as a string) or a
     # "wrapped identifier".
-    assert isinstance(value, str) or isinstance(value, tuple)
+    assert _IsStrOrUnicode or isinstance(value, tuple)
     super(Const, self).__init__(mojom_name, **kwargs)
     self.attribute_list = attribute_list
     self.typename = typename
@@ -161,7 +170,7 @@ class EnumValue(Definition):
     # The optional value is either an int (which is current a string) or a
     # "wrapped identifier".
     assert attribute_list is None or isinstance(attribute_list, AttributeList)
-    assert value is None or isinstance(value, (str, tuple))
+    assert value is None or _IsStrOrUnicode(value) or isinstance(value, tuple)
     super(EnumValue, self).__init__(mojom_name, **kwargs)
     self.attribute_list = attribute_list
     self.value = value
@@ -184,7 +193,7 @@ class Import(NodeBase):
 
   def __init__(self, attribute_list, import_filename, **kwargs):
     assert attribute_list is None or isinstance(attribute_list, AttributeList)
-    assert isinstance(import_filename, str)
+    assert _IsStrOrUnicode(import_filename)
     super(Import, self).__init__(**kwargs)
     self.attribute_list = attribute_list
     self.import_filename = import_filename
@@ -305,10 +314,10 @@ class Parameter(NodeBase):
   """Represents a method request or response parameter."""
 
   def __init__(self, mojom_name, attribute_list, ordinal, typename, **kwargs):
-    assert isinstance(mojom_name, str)
+    assert _IsStrOrUnicode(mojom_name)
     assert attribute_list is None or isinstance(attribute_list, AttributeList)
     assert ordinal is None or isinstance(ordinal, Ordinal)
-    assert isinstance(typename, str)
+    assert _IsStrOrUnicode(typename)
     super(Parameter, self).__init__(**kwargs)
     self.mojom_name = mojom_name
     self.attribute_list = attribute_list
@@ -350,13 +359,14 @@ class StructField(Definition):
 
   def __init__(self, mojom_name, attribute_list, ordinal, typename,
                default_value, **kwargs):
-    assert isinstance(mojom_name, str)
+    assert _IsStrOrUnicode(mojom_name)
     assert attribute_list is None or isinstance(attribute_list, AttributeList)
     assert ordinal is None or isinstance(ordinal, Ordinal)
-    assert isinstance(typename, str)
+    assert _IsStrOrUnicode(typename)
     # The optional default value is currently either a value as a string or a
     # "wrapped identifier".
-    assert default_value is None or isinstance(default_value, (str, tuple))
+    assert default_value is None or _IsStrOrUnicode(default_value) or \
+        isinstance(default_value, tuple)
     super(StructField, self).__init__(mojom_name, **kwargs)
     self.attribute_list = attribute_list
     self.ordinal = ordinal
@@ -396,10 +406,10 @@ class Union(Definition):
 
 class UnionField(Definition):
   def __init__(self, mojom_name, attribute_list, ordinal, typename, **kwargs):
-    assert isinstance(mojom_name, str)
+    assert _IsStrOrUnicode(mojom_name)
     assert attribute_list is None or isinstance(attribute_list, AttributeList)
     assert ordinal is None or isinstance(ordinal, Ordinal)
-    assert isinstance(typename, str)
+    assert _IsStrOrUnicode(typename)
     super(UnionField, self).__init__(mojom_name, **kwargs)
     self.attribute_list = attribute_list
     self.ordinal = ordinal

mojo/public/tools/mojom/mojom/parse/parser.py

@@ -472,7 +472,7 @@ def Parse(source, filename):
   """Parse source file to AST.
 
   Args:
-    source: The source text as a str.
+    source: The source text as a str (Python 2 or 3) or unicode (Python 2).
     filename: The filename that |source| originates from.
 
   Returns:

mojo/public/tools/mojom/mojom_parser.py

@@ -11,6 +11,7 @@ generate usable language bindings.
 """
 
 import argparse
+import codecs
 import errno
 import json
 import os
@@ -193,7 +194,7 @@ def _ParseMojoms(mojom_files,
   abs_paths = dict(
       (path, abs_path) for abs_path, path in mojom_files_to_parse.items())
   for mojom_abspath, _ in mojom_files_to_parse.items():
-    with open(mojom_abspath) as f:
+    with codecs.open(mojom_abspath, encoding='utf-8') as f:
       ast = parser.Parse(''.join(f.readlines()), mojom_abspath)
       conditional_features.RemoveDisabledDefinitions(ast, enabled_features)
       loaded_mojom_asts[mojom_abspath] = ast

mojo/public/tools/mojom/stable_attribute_unittest.py

@@ -22,7 +22,7 @@ class StableAttributeTest(MojomParserTestCase):
         struct UnstableStruct { UnstableEnum a; };
         [Stable] union TestUnion { TestEnum a; TestStruct b; };
         union UnstableUnion { UnstableEnum a; UnstableStruct b; };
-        [Stable] interface TestInterface { Foo(TestUnion x) => (); };
+        [Stable] interface TestInterface { Foo@0(TestUnion x) => (); };
         interface UnstableInterface { Foo(UnstableUnion x) => (); };
         """)
     self.ParseMojoms([mojom])
@@ -101,22 +101,27 @@ class StableAttributeTest(MojomParserTestCase):
     """A [Stable] interface is valid if all its methods' parameter types are
     stable, including response parameters where applicable."""
     self.ExtractTypes('[Stable] interface F {};')
-    self.ExtractTypes('[Stable] interface F { A(int32 x); };')
-    self.ExtractTypes('[Stable] interface F { A(int32 x) => (bool b); };')
+    self.ExtractTypes('[Stable] interface F { A@0(int32 x); };')
+    self.ExtractTypes('[Stable] interface F { A@0(int32 x) => (bool b); };')
     self.ExtractTypes("""\
         [Stable] enum E { A, B, C };
         [Stable] struct S {};
-        [Stable] interface F { A(E e, S s) => (bool b, array<S> s); };
+        [Stable] interface F { A@0(E e, S s) => (bool b, array<S> s); };
         """)
 
     with self.assertRaisesRegexp(Exception, 'because it depends on E'):
-      self.ExtractTypes('enum E { A, B, C }; [Stable] interface F { A(E e); };')
+      self.ExtractTypes(
+          'enum E { A, B, C }; [Stable] interface F { A@0(E e); };')
     with self.assertRaisesRegexp(Exception, 'because it depends on E'):
       self.ExtractTypes(
-          'enum E { A, B, C }; [Stable] interface F { A(int32 x) => (E e); };')
+          'enum E { A, B, C }; [Stable] interface F { A@0(int32 x) => (E e); };'
+      )
     with self.assertRaisesRegexp(Exception, 'because it depends on S'):
       self.ExtractTypes(
-          'struct S {}; [Stable] interface F { A(int32 x) => (S s); };')
+          'struct S {}; [Stable] interface F { A@0(int32 x) => (S s); };')
     with self.assertRaisesRegexp(Exception, 'because it depends on S'):
       self.ExtractTypes(
-          'struct S {}; [Stable] interface F { A(S s) => (bool b); };')
+          'struct S {}; [Stable] interface F { A@0(S s) => (bool b); };')
+
+    with self.assertRaisesRegexp(Exception, 'explicit method ordinals'):
+      self.ExtractTypes('[Stable] interface F { A() => (); };')