AndroidManifest.xml: Tweak DIFF-ANCHOR computation

* Remove anchors for <application> since every manfiest change will cause
  it to change.
* Use android:name to compute anchors when available since that will
  lead to stable anchors when attributes / children are changed.
* Prevent self-closing tags for <service>, <activity>, etc.
* Added test.

Bug: 1064151
Change-Id: Ia0c242c713b58c6cb54556e6d0b82a559ad9858a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340644
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795923}

build/android/PRESUBMIT.py

@@ -75,6 +75,7 @@ def CommonChecks(input_api, output_api):
               J('.', 'emma_coverage_stats_test.py'),
               J('.', 'list_class_verification_failures_test.py'),
               J('gyp', 'util', 'build_utils_test.py'),
+              J('gyp', 'util', 'manifest_utils_test.py'),
               J('gyp', 'util', 'md5_check_test.py'),
               J('gyp', 'util', 'resource_utils_test.py'),
               J('pylib', 'constants', 'host_paths_unittest.py'),

build/android/gyp/compile_resources.py

@@ -992,7 +992,8 @@ def _WriteOutputs(options, build):
 def _CreateNormalizedManifest(options):
   with build_utils.TempDir() as tempdir:
     fixed_manifest, _ = _FixManifest(options, tempdir)
-    return manifest_utils.NormalizeManifest(fixed_manifest)
+    with open(fixed_manifest) as f:
+      return manifest_utils.NormalizeManifest(f.read())
 
 
 def _OnStaleMd5(options):

build/android/gyp/util/manifest_utils.py

@@ -4,6 +4,7 @@
 
 """Contains common helpers for working with Android manifests."""
 
+import hashlib
 import os
 import shlex
 import xml.dom.minidom as minidom
@@ -151,7 +152,7 @@ def _SplitElement(line):
 
 
 def _CreateNodeHash(lines):
-  """Computes a hash for the first XML node found in |lines|.
+  """Computes a hash (md5) for the first XML node found in |lines|.
 
   Args:
     lines: List of strings containing pretty-printed XML.
@@ -160,12 +161,23 @@ def _CreateNodeHash(lines):
     Positive 32-bit integer hash of the node (including children).
   """
   target_indent = lines[0].find('<')
+  tag_closed = False
   for i, l in enumerate(lines[1:]):
     cur_indent = l.find('<')
     if cur_indent != -1 and cur_indent <= target_indent:
       tag_lines = lines[:i + 1]
-      return hash(tuple(tag_lines)) % 0x100000000  # Make 32-bit non-negative.
-  assert False, 'Did not find end of node:\n' + '\n'.join(lines)
+      break
+    elif not tag_closed and 'android:name="' in l:
+      # To reduce noise of node tags changing, use android:name as the
+      # basis the hash since they usually unique.
+      tag_lines = [l]
+      break
+    tag_closed = tag_closed or '>' in l
+  else:
+    assert False, 'Did not find end of node:\n' + '\n'.join(lines)
+
+  # Insecure and truncated hash as it only needs to be unique vs. its neighbors.
+  return hashlib.md5('\n'.join(tag_lines)).hexdigest()[:8]
 
 
 def _IsSelfClosing(lines):
@@ -190,23 +202,32 @@ def _AddDiffTags(lines):
   hash_stack = []
   for i, l in enumerate(lines):
     stripped = l.lstrip()
-    # If it's an indented tag that is not self-closing (unless wrapped):
-    if l[0] == ' ' and stripped[0] == '<' and l[-2:] != '/>':
-      if stripped[1] != '/':
-        cur_hash = _CreateNodeHash(lines[i:])
-        if not _IsSelfClosing(lines[i:]):
-          hash_stack.append(cur_hash)
-      else:
-        cur_hash = hash_stack.pop()
-      lines[i] += '  # DIFF-ANCHOR: {:x}'.format(cur_hash)
+    # Ignore non-indented tags and lines that are not the start/end of a node.
+    if l[0] != ' ' or stripped[0] != '<':
+      continue
+    # Ignore self-closing nodes that fit on one line.
+    if l[-2:] == '/>':
+      continue
+    # Ignore <application> since diff tag changes with basically any change.
+    if stripped.lstrip('</').startswith('application'):
+      continue
+
+    # Check for the closing tag (</foo>).
+    if stripped[1] != '/':
+      cur_hash = _CreateNodeHash(lines[i:])
+      if not _IsSelfClosing(lines[i:]):
+        hash_stack.append(cur_hash)
+    else:
+      cur_hash = hash_stack.pop()
+    lines[i] += '  # DIFF-ANCHOR: {}'.format(cur_hash)
   assert not hash_stack, 'hash_stack was not empty:\n' + '\n'.join(hash_stack)
 
 
-def NormalizeManifest(path):
-  with open(path) as f:
-    # This also strips comments and sorts node attributes alphabetically.
-    root = ElementTree.fromstring(f.read())
-    package = GetPackage(root)
+def NormalizeManifest(manifest_contents):
+  _RegisterElementTreeNamespaces()
+  # This also strips comments and sorts node attributes alphabetically.
+  root = ElementTree.fromstring(manifest_contents)
+  package = GetPackage(root)
 
   # Trichrome's static library version number is updated daily. To avoid
   # frequent manifest check failures, we remove the exact version number
@@ -242,16 +263,23 @@ def NormalizeManifest(path):
   dom = minidom.parseString(ElementTree.tostring(root))
   out_lines = []
   for l in dom.toprettyxml(indent='  ').splitlines():
-    if l.strip():
-      if len(l) > _WRAP_LINE_LENGTH and any(x in l for x in _WRAP_CANDIDATES):
-        indent = ' ' * l.find('<')
-        start_tag, attrs, end_tag = _SplitElement(l)
-        out_lines.append('{}{}'.format(indent, start_tag))
-        for attribute in attrs:
-          out_lines.append('{}    {}'.format(indent, attribute))
-        out_lines[-1] += end_tag
-      else:
-        out_lines.append(l)
+    if not l or l.isspace():
+      continue
+    if len(l) > _WRAP_LINE_LENGTH and any(x in l for x in _WRAP_CANDIDATES):
+      indent = ' ' * l.find('<')
+      start_tag, attrs, end_tag = _SplitElement(l)
+      out_lines.append('{}{}'.format(indent, start_tag))
+      for attribute in attrs:
+        out_lines.append('{}    {}'.format(indent, attribute))
+      out_lines[-1] += '>'
+      # Heuristic: Do not allow multi-line tags to be self-closing since these
+      # can generally be allowed to have nested elements. When diffing, it adds
+      # noise if the base file is self-closing and the non-base file is not
+      # self-closing.
+      if end_tag == '/>':
+        out_lines.append('{}{}>'.format(indent, start_tag.replace('<', '</')))
+    else:
+      out_lines.append(l)
 
   # Make output more diff-friendly.
   _AddDiffTags(out_lines)

build/android/gyp/util/manifest_utils_test.py

+#!/usr/bin/env python
+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import collections
+import os
+import sys
+import unittest
+
+sys.path.insert(1, os.path.join(os.path.dirname(__file__), '..'))
+from util import manifest_utils
+
+_TEST_MANIFEST = """\
+<?xml version="1.0" ?>
+<manifest package="test.pkg"
+    tools:ignore="MissingVersion"
+    xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:tools="http://schemas.android.com/tools">
+  <!-- Should be one line. -->
+  <uses-sdk android:minSdkVersion="24"
+      android:targetSdkVersion="30"/>
+  <!-- Should have attrs sorted-->
+  <uses-feature android:required="false" android:version="1"
+    android:name="android.hardware.vr.headtracking" />
+  <!-- Should not be wrapped since < 100 chars. -->
+  <application
+      android:name="testname">
+    <activity
+        {extra_activity_attr}
+        android:icon="@drawable/ic_devices_48dp"
+        android:label="label with spaces"
+        android:name="to be hashed"
+        android:theme="@style/Theme.Chromium.Activity.TranslucentNoAnimations">
+      <intent-filter>
+        {extra_intent_filter_elem}
+        <action android:name="android.intent.action.SEND"/>
+        <category android:name="android.intent.category.DEFAULT"/>
+        <data android:mimeType="text/plain"/>
+      </intent-filter>
+    </activity>
+    <!-- Should be made non-self-closing. -->
+    <receiver android:exported="false" android:name="\
+org.chromium.chrome.browser.announcement.AnnouncementNotificationManager$Rcvr"/>
+  </application>
+</manifest>
+"""
+
+_TEST_MANIFEST_NORMALIZED = """\
+<?xml version="1.0" ?>
+<manifest
+    package="test.pkg"
+    tools:ignore="MissingVersion"
+    xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:tools="http://schemas.android.com/tools">
+  <uses-sdk android:minSdkVersion="24" android:targetSdkVersion="30"/>
+  <uses-feature android:name="android.hardware.vr.headtracking" \
+android:required="false" android:version="1"/>
+  <application android:name="testname">
+    <activity  # DIFF-ANCHOR: {activity_diff_anchor}
+        {extra_activity_attr}android:icon="@drawable/ic_devices_48dp"
+        android:label="label with spaces"
+        android:name="to be hashed"
+        android:theme="@style/Theme.Chromium.Activity.TranslucentNoAnimations">
+      <intent-filter>  # DIFF-ANCHOR: {intent_filter_diff_anchor}
+        {extra_intent_filter_elem}\
+<action android:name="android.intent.action.SEND"/>
+        <category android:name="android.intent.category.DEFAULT"/>
+        <data android:mimeType="text/plain"/>
+      </intent-filter>  # DIFF-ANCHOR: {intent_filter_diff_anchor}
+    </activity>  # DIFF-ANCHOR: {activity_diff_anchor}
+    <receiver  # DIFF-ANCHOR: 355000d2
+        android:exported="false"
+        android:name=\
+"org.chromium.chrome.browser.announcement.AnnouncementNotificationManager$Rcvr">
+    </receiver>  # DIFF-ANCHOR: 355000d2
+  </application>
+</manifest>
+"""
+
+_ACTIVITY_DIFF_ANCHOR = '32b3a641'
+_INTENT_FILTER_DIFF_ANCHOR = '4ee601b7'
+
+
+def _CreateTestData(intent_filter_diff_anchor=_INTENT_FILTER_DIFF_ANCHOR,
+                    extra_activity_attr='',
+                    extra_intent_filter_elem=''):
+  if extra_activity_attr:
+    extra_activity_attr += '\n        '
+  if extra_intent_filter_elem:
+    extra_intent_filter_elem += '\n        '
+  test_manifest = _TEST_MANIFEST.format(
+      extra_activity_attr=extra_activity_attr,
+      extra_intent_filter_elem=extra_intent_filter_elem)
+  expected = _TEST_MANIFEST_NORMALIZED.format(
+      activity_diff_anchor=_ACTIVITY_DIFF_ANCHOR,
+      intent_filter_diff_anchor=intent_filter_diff_anchor,
+      extra_activity_attr=extra_activity_attr,
+      extra_intent_filter_elem=extra_intent_filter_elem)
+  return test_manifest, expected
+
+
+class ManifestUtilsTest(unittest.TestCase):
+  # Enable diff output.
+  maxDiff = None
+
+  def testNormalizeManifest_golden(self):
+    test_manifest, expected = _CreateTestData()
+    actual = manifest_utils.NormalizeManifest(test_manifest)
+    self.assertMultiLineEqual(expected, actual)
+
+  def testNormalizeManifest_nameUsedForActivity(self):
+    test_manifest, expected = _CreateTestData(extra_activity_attr='a="b"')
+    actual = manifest_utils.NormalizeManifest(test_manifest)
+    # Checks that the DIFF-ANCHOR does not change with the added attribute.
+    self.assertMultiLineEqual(expected, actual)
+
+  def testNormalizeManifest_nameNotUsedForIntentFilter(self):
+    test_manifest, expected = _CreateTestData(
+        extra_intent_filter_elem='<a/>', intent_filter_diff_anchor='5f5c8a70')
+    actual = manifest_utils.NormalizeManifest(test_manifest)
+    # Checks that the DIFF-ANCHOR does change with the added element despite
+    # having a nested element with an android:name set.
+    self.assertMultiLineEqual(expected, actual)
+
+
+if __name__ == '__main__':
+  unittest.main()

chrome/android/expectations/trichrome_library_apk.AndroidManifest.expected

@@ -10,11 +10,11 @@
   <uses-feature android:name="android.software.leanback" android:required="false"/>
   <uses-feature android:name="android.hardware.touchscreen" android:required="false"/>
   <uses-feature android:glEsVersion="0x00020000"/>
-  <application  # DIFF-ANCHOR: 9d5595e9
+  <application
       android:extractNativeLibs="false"
       android:icon="@drawable/icon_webview"
       android:label="Trichrome Library"
       android:multiArch="true">
     <static-library android:name="$PACKAGE" android:version="$VERSION_NUMBER"/>
-  </application>  # DIFF-ANCHOR: 9d5595e9
+  </application>
 </manifest>