Bug 1366169 - Avoid reordered manifest entries creating unexpected overrides. r=gps

As described in changeset c94e87a18096, chrome manifest entries are
reordered and don't necessarily appear in the order they have in jar.mn.

And in some cases, the order does matter: when entries with flags are
followed by entries with more broad flags or no flags at all. Nobody
should be writing entries in that order on purpose, so error out during
packaging when we detect the pattern.

--HG--
extra : rebase_source : d9617bbcbd8560503c532a13c10c8afb0fd49411

python/mozbuild/mozpack/chrome/flags.py

@@ -62,6 +62,9 @@ class Flag(object):
             return self.name
         return '%s=%s' % (self.name, self.value)
 
+    def __eq__(self, other):
+        return str(self) == other
+
 
 class StringFlag(object):
     '''
@@ -125,6 +128,9 @@ class StringFlag(object):
                 res.append('%s!=%s' % (self.name, val))
         return ' '.join(res)
 
+    def __eq__(self, other):
+        return str(self) == other
+
 
 class VersionFlag(object):
     '''
@@ -198,6 +204,9 @@ class VersionFlag(object):
                 res.append('%s%s%s' % (self.name, comparison, val))
         return ' '.join(res)
 
+    def __eq__(self, other):
+        return str(self) == other
+
 
 class Flags(OrderedDict):
     '''

python/mozbuild/mozpack/packager/formats.py

@@ -6,11 +6,13 @@ from __future__ import absolute_import
 
 from mozpack.chrome.manifest import (
     Manifest,
+    ManifestEntryWithRelPath,
     ManifestInterfaces,
     ManifestChrome,
     ManifestBinaryComponent,
     ManifestResource,
 )
+from mozpack.errors import errors
 from urlparse import urlparse
 import mozpack.path as mozpath
 from mozpack.files import (
@@ -133,6 +135,7 @@ class FlatSubFormatter(object):
     def __init__(self, copier):
         assert isinstance(copier, (FileRegistry, FileRegistrySubtree))
         self.copier = copier
+        self._chrome_db = {}
 
     def add(self, path, content):
         self.copier.add(path, content)
@@ -156,6 +159,18 @@ class FlatSubFormatter(object):
                                             mozpath.basename(path))
                 self.add_manifest(Manifest(parent, relpath))
             self.copier.add(path, ManifestFile(entry.base))
+
+        if isinstance(entry, ManifestChrome):
+            data = self._chrome_db.setdefault(entry.name, {})
+            entries = data.setdefault(entry.type, [])
+            for e in entries:
+                # Ideally, we'd actually check whether entry.flags are more
+                # specific than e.flags, but in practice the following test
+                # is enough for now.
+                if not entry.flags or e.flags and entry.flags == e.flags:
+                    errors.fatal('"%s" overrides "%s"' % (entry, e))
+            entries.append(entry)
+
         self.copier[path].add(entry)
 
     def add_interfaces(self, path, content):

python/mozbuild/mozpack/test/test_packager_formats.py

@@ -20,6 +20,10 @@ from mozpack.chrome.manifest import (
     ManifestResource,
     ManifestBinaryComponent,
 )
+from mozpack.errors import (
+    errors,
+    ErrorMessage,
+)
 from mozpack.test.test_files import (
     MockDest,
     foo_xpt,
@@ -423,6 +427,46 @@ class TestFormatters(unittest.TestCase):
             self.assertTrue(is_resource(base, 'chrome/foo/bar/dummy_'))
             self.assertFalse(is_resource(base, 'chrome/foo/bar/dummy'))
 
+    def test_chrome_override(self):
+        registry = FileRegistry()
+        f = FlatFormatter(registry)
+        f.add_base('')
+        f.add_manifest(ManifestContent('chrome', 'foo', 'foo/unix'))
+        # A more specific entry for a given chrome name can override a more
+        # generic one.
+        f.add_manifest(ManifestContent('chrome', 'foo', 'foo/win', 'os=WINNT'))
+        f.add_manifest(ManifestContent('chrome', 'foo', 'foo/osx', 'os=Darwin'))
+
+        # Chrome with the same name overrides the previous registration.
+        with self.assertRaises(ErrorMessage) as e:
+            f.add_manifest(ManifestContent('chrome', 'foo', 'foo/'))
+
+        self.assertEqual(e.exception.message,
+            'Error: "content foo foo/" overrides '
+            '"content foo foo/unix"')
+
+        # Chrome with the same name and same flags overrides the previous
+        # registration.
+        with self.assertRaises(ErrorMessage) as e:
+            f.add_manifest(ManifestContent('chrome', 'foo', 'foo/', 'os=WINNT'))
+
+        self.assertEqual(e.exception.message,
+            'Error: "content foo foo/ os=WINNT" overrides '
+            '"content foo foo/win os=WINNT"')
+
+        # We may start with the more specific entry first
+        f.add_manifest(ManifestContent('chrome', 'bar', 'bar/win', 'os=WINNT'))
+        # Then adding a more generic one overrides it.
+        with self.assertRaises(ErrorMessage) as e:
+            f.add_manifest(ManifestContent('chrome', 'bar', 'bar/unix'))
+
+        self.assertEqual(e.exception.message,
+            'Error: "content bar bar/unix" overrides '
+            '"content bar bar/win os=WINNT"')
+
+        # Adding something more specific still works.
+        f.add_manifest(ManifestContent('chrome', 'bar', 'bar/win',
+                                       'os=WINNT osversion>=7.0'))
 
 if __name__ == '__main__':
     mozunit.main()