Merge changes from topic "aapt2-warn-manifest-validation"

* changes:
  AAPT2: treat manifest validation errors as warnings when asked
  AAPT2: Better error messages for ManifestFixer
  AAPT2: Differentiate between Android and Java package names

tools/aapt2/cmd/Link.cpp

@@ -1968,6 +1968,9 @@ int Link(const std::vector<StringPiece>& args, IDiagnostics* diagnostics) {
                         &options.manifest_fixer_options.rename_instrumentation_target_package)
           .OptionalFlagList("-0", "File extensions not to compress.",
                             &options.extensions_to_not_compress)
+          .OptionalSwitch("--warn-manifest-validation",
+                          "Treat manifest validation errors as warnings.",
+                          &options.manifest_fixer_options.warn_validation)
           .OptionalFlagList("--split",
                             "Split resources matching a set of configs out to a Split APK.\n"
                             "Syntax: path/to/output.apk:<config>[,<config>[...]].\n"

tools/aapt2/java/ManifestClassGenerator.cpp

@@ -22,9 +22,11 @@
 #include "java/AnnotationProcessor.h"
 #include "java/ClassDefinition.h"
 #include "util/Maybe.h"
+#include "text/Unicode.h"
 #include "xml/XmlDom.h"
 
-using android::StringPiece;
+using ::android::StringPiece;
+using ::aapt::text::IsJavaIdentifier;
 
 namespace aapt {
 
@@ -46,11 +48,8 @@ static Maybe<StringPiece> ExtractJavaIdentifier(IDiagnostics* diag,
     return {};
   }
 
-  iter = util::FindNonAlphaNumericAndNotInSet(result, "_");
+  if (!IsJavaIdentifier(result)) {
-  if (iter != result.end()) {
+    diag->Error(DiagMessage(source) << "invalid Java identifier '" << result << "'");
-    diag->Error(DiagMessage(source) << "invalid character '"
-                                    << StringPiece(iter, 1) << "' in '"
-                                    << result << "'");
     return {};
   }
 

tools/aapt2/link/ManifestFixer.cpp

@@ -127,9 +127,9 @@ static bool VerifyManifest(xml::Element* el, SourcePathDiagnostics* diag) {
     diag->Error(DiagMessage(el->line_number)
                 << "attribute 'package' in <manifest> tag must not be a reference");
     return false;
-  } else if (!util::IsJavaPackageName(attr->value)) {
+  } else if (!util::IsAndroidPackageName(attr->value)) {
     diag->Error(DiagMessage(el->line_number)
-                << "attribute 'package' in <manifest> tag is not a valid Java package name: '"
+                << "attribute 'package' in <manifest> tag is not a valid Android package name: '"
                 << attr->value << "'");
     return false;
   }
@@ -409,7 +409,10 @@ bool ManifestFixer::Consume(IAaptContext* context, xml::XmlResource* doc) {
     return false;
   }
 
-  if (!executor.Execute(xml::XmlActionExecutorPolicy::kWhitelist, context->GetDiagnostics(), doc)) {
+  xml::XmlActionExecutorPolicy policy = options_.warn_validation
+                                            ? xml::XmlActionExecutorPolicy::kWhitelistWarning
+                                            : xml::XmlActionExecutorPolicy::kWhitelist;
+  if (!executor.Execute(policy, context->GetDiagnostics(), doc)) {
     return false;
   }
 

tools/aapt2/link/ManifestFixer.h

@@ -35,6 +35,11 @@ struct ManifestFixerOptions {
   Maybe<std::string> rename_instrumentation_target_package;
   Maybe<std::string> version_name_default;
   Maybe<std::string> version_code_default;
+
+  // Wether validation errors should be treated only as warnings. If this is 'true', then an
+  // incorrect node will not result in an error, but only as a warning, and the parsing will
+  // continue.
+  bool warn_validation = false;
 };
 
 /**

tools/aapt2/link/ManifestFixer_test.cpp

@@ -19,6 +19,7 @@
 #include "test/Test.h"
 
 using ::android::StringPiece;
+using ::testing::IsNull;
 using ::testing::NotNull;
 
 namespace aapt {
@@ -109,7 +110,9 @@ TEST_F(ManifestFixerTest, AllowMetaData) {
 }
 
 TEST_F(ManifestFixerTest, UseDefaultSdkVersionsIfNonePresent) {
-  ManifestFixerOptions options = {std::string("8"), std::string("22")};
+  ManifestFixerOptions options;
+  options.min_sdk_version_default = std::string("8");
+  options.target_sdk_version_default = std::string("22");
 
   std::unique_ptr<xml::XmlResource> doc = VerifyWithOptions(R"EOF(
       <manifest xmlns:android="http://schemas.android.com/apk/res/android"
@@ -190,7 +193,9 @@ TEST_F(ManifestFixerTest, UseDefaultSdkVersionsIfNonePresent) {
 }
 
 TEST_F(ManifestFixerTest, UsesSdkMustComeBeforeApplication) {
-  ManifestFixerOptions options = {std::string("8"), std::string("22")};
+  ManifestFixerOptions options;
+  options.min_sdk_version_default = std::string("8");
+  options.target_sdk_version_default = std::string("22");
   std::unique_ptr<xml::XmlResource> doc = VerifyWithOptions(R"EOF(
           <manifest xmlns:android="http://schemas.android.com/apk/res/android"
                     package="android">
@@ -439,4 +444,27 @@ TEST_F(ManifestFixerTest, SupportKeySets) {
   EXPECT_THAT(Verify(input), NotNull());
 }
 
+TEST_F(ManifestFixerTest, UnexpectedElementsInManifest) {
+  std::string input = R"(
+      <manifest xmlns:android="http://schemas.android.com/apk/res/android"
+          package="android">
+        <beep/>
+      </manifest>)";
+  ManifestFixerOptions options;
+  options.warn_validation = true;
+
+  // Unexpected element should result in a warning if the flag is set to 'true'.
+  std::unique_ptr<xml::XmlResource> manifest = VerifyWithOptions(input, options);
+  ASSERT_THAT(manifest, NotNull());
+
+  // Unexpected element should result in an error if the flag is set to 'false'.
+  options.warn_validation = false;
+  manifest = VerifyWithOptions(input, options);
+  ASSERT_THAT(manifest, IsNull());
+
+  // By default the flag should be set to 'false'.
+  manifest = Verify(input);
+  ASSERT_THAT(manifest, IsNull());
+}
+
 }  // namespace aapt

tools/aapt2/text/Unicode.cpp

@@ -85,7 +85,8 @@ bool IsJavaIdentifier(const StringPiece& str) {
     return false;
   }
 
-  if (!IsXidStart(iter.Next())) {
+  const char32_t first_codepoint = iter.Next();
+  if (!IsXidStart(first_codepoint) && first_codepoint != U'_' && first_codepoint != U'$') {
     return false;
   }
 

tools/aapt2/text/Unicode_test.cpp

@@ -44,10 +44,11 @@ TEST(UnicodeTest, IsXidContinue) {
 TEST(UnicodeTest, IsJavaIdentifier) {
   EXPECT_TRUE(IsJavaIdentifier("FøøBar_12"));
   EXPECT_TRUE(IsJavaIdentifier("Føø$Bar"));
+  EXPECT_TRUE(IsJavaIdentifier("_FøøBar"));
+  EXPECT_TRUE(IsJavaIdentifier("$Føø$Bar"));
 
   EXPECT_FALSE(IsJavaIdentifier("12FøøBar"));
-  EXPECT_FALSE(IsJavaIdentifier("_FøøBar"));
+  EXPECT_FALSE(IsJavaIdentifier(".Hello"));
-  EXPECT_FALSE(IsJavaIdentifier("$Føø$Bar"));
 }
 
 TEST(UnicodeTest, IsValidResourceEntryName) {

tools/aapt2/util/Util.cpp

@@ -24,6 +24,7 @@
 #include "androidfw/StringPiece.h"
 #include "utils/Unicode.h"
 
+#include "text/Unicode.h"
 #include "text/Utf8Iterator.h"
 #include "util/BigBuffer.h"
 #include "util/Maybe.h"
@@ -94,72 +95,55 @@ StringPiece TrimWhitespace(const StringPiece& str) {
   return StringPiece(start, end - start);
 }
 
-StringPiece::const_iterator FindNonAlphaNumericAndNotInSet(
+static int IsJavaNameImpl(const StringPiece& str) {
-    const StringPiece& str, const StringPiece& allowed_chars) {
+  int pieces = 0;
-  const auto end_iter = str.end();
+  for (const StringPiece& piece : Tokenize(str, '.')) {
-  for (auto iter = str.begin(); iter != end_iter; ++iter) {
+    pieces++;
-    char c = *iter;
+    if (!text::IsJavaIdentifier(piece)) {
-    if ((c >= u'a' && c <= u'z') || (c >= u'A' && c <= u'Z') ||
+      return -1;
-        (c >= u'0' && c <= u'9')) {
-      continue;
-    }
-
-    bool match = false;
-    for (char i : allowed_chars) {
-      if (c == i) {
-        match = true;
-        break;
-      }
-    }
-
-    if (!match) {
-      return iter;
     }
   }
-  return end_iter;
+  return pieces;
 }
 
 bool IsJavaClassName(const StringPiece& str) {
-  size_t pieces = 0;
+  return IsJavaNameImpl(str) >= 2;
-  for (const StringPiece& piece : Tokenize(str, '.')) {
-    pieces++;
-    if (piece.empty()) {
-      return false;
-    }
-
-    // Can't have starting or trailing $ character.
-    if (piece.data()[0] == '$' || piece.data()[piece.size() - 1] == '$') {
-      return false;
-    }
-
-    if (FindNonAlphaNumericAndNotInSet(piece, "$_") != piece.end()) {
-      return false;
-    }
-  }
-  return pieces >= 2;
 }
 
 bool IsJavaPackageName(const StringPiece& str) {
-  if (str.empty()) {
+  return IsJavaNameImpl(str) >= 1;
-    return false;
+}
-  }
 
-  size_t pieces = 0;
+static int IsAndroidNameImpl(const StringPiece& str) {
+  int pieces = 0;
   for (const StringPiece& piece : Tokenize(str, '.')) {
-    pieces++;
     if (piece.empty()) {
-      return false;
+      return -1;
     }
 
-    if (piece.data()[0] == '_' || piece.data()[piece.size() - 1] == '_') {
+    const char first_character = piece.data()[0];
-      return false;
+    if (!::isalpha(first_character)) {
+      return -1;
     }
 
-    if (FindNonAlphaNumericAndNotInSet(piece, "_") != piece.end()) {
+    bool valid = std::all_of(piece.begin() + 1, piece.end(), [](const char c) -> bool {
-      return false;
+      return ::isalnum(c) || c == '_';
+    });
+
+    if (!valid) {
+      return -1;
     }
+    pieces++;
   }
-  return pieces >= 1;
+  return pieces;
+}
+
+bool IsAndroidPackageName(const StringPiece& str) {
+  return IsAndroidNameImpl(str) > 1 || str == "android";
+}
+
+bool IsAndroidSplitName(const StringPiece& str) {
+  return IsAndroidNameImpl(str) > 0;
 }
 
 Maybe<std::string> GetFullyQualifiedClassName(const StringPiece& package,
@@ -176,7 +160,7 @@ Maybe<std::string> GetFullyQualifiedClassName(const StringPiece& package,
     return {};
   }
 
-  std::string result(package.data(), package.size());
+  std::string result = package.to_string();
   if (classname.data()[0] != '.') {
     result += '.';
   }

tools/aapt2/util/Util.h

@@ -53,48 +53,40 @@ struct Range {
 std::vector<std::string> Split(const android::StringPiece& str, char sep);
 std::vector<std::string> SplitAndLowercase(const android::StringPiece& str, char sep);
 
-/**
+// Returns true if the string starts with prefix.
- * Returns true if the string starts with prefix.
- */
 bool StartsWith(const android::StringPiece& str, const android::StringPiece& prefix);
 
-/**
+// Returns true if the string ends with suffix.
- * Returns true if the string ends with suffix.
- */
 bool EndsWith(const android::StringPiece& str, const android::StringPiece& suffix);
 
-/**
+// Creates a new StringPiece16 that points to a substring of the original string without leading or
- * Creates a new StringPiece16 that points to a substring
+// trailing whitespace.
- * of the original string without leading or trailing whitespace.
- */
 android::StringPiece TrimWhitespace(const android::StringPiece& str);
 
-/**
+// Tests that the string is a valid Java class name.
- * Returns an iterator to the first character that is not alpha-numeric and that
- * is not in the allowedChars set.
- */
-android::StringPiece::const_iterator FindNonAlphaNumericAndNotInSet(
-    const android::StringPiece& str, const android::StringPiece& allowed_chars);
-
-/**
- * Tests that the string is a valid Java class name.
- */
 bool IsJavaClassName(const android::StringPiece& str);
 
-/**
+// Tests that the string is a valid Java package name.
- * Tests that the string is a valid Java package name.
- */
 bool IsJavaPackageName(const android::StringPiece& str);
 
-/**
+// Tests that the string is a valid Android package name. More strict than a Java package name.
- * Converts the class name to a fully qualified class name from the given
+// - First character of each component (separated by '.') must be an ASCII letter.
- * `package`. Ex:
+// - Subsequent characters of a component can be ASCII alphanumeric or an underscore.
- *
+// - Package must contain at least two components, unless it is 'android'.
- * asdf         --> package.asdf
+bool IsAndroidPackageName(const android::StringPiece& str);
- * .asdf        --> package.asdf
+
- * .a.b         --> package.a.b
+// Tests that the string is a valid Android split name.
- * asdf.adsf    --> asdf.adsf
+// - First character of each component (separated by '.') must be an ASCII letter.
- */
+// - Subsequent characters of a component can be ASCII alphanumeric or an underscore.
+bool IsAndroidSplitName(const android::StringPiece& str);
+
+// Converts the class name to a fully qualified class name from the given
+// `package`. Ex:
+//
+// asdf         --> package.asdf
+// .asdf        --> package.asdf
+// .a.b         --> package.a.b
+// asdf.adsf    --> asdf.adsf
 Maybe<std::string> GetFullyQualifiedClassName(const android::StringPiece& package,
                                               const android::StringPiece& class_name);
 
@@ -108,23 +100,17 @@ typename std::enable_if<std::is_arithmetic<T>::value, int>::type compare(const T
   return 0;
 }
 
-/**
+// Makes a std::unique_ptr<> with the template parameter inferred by the compiler.
- * Makes a std::unique_ptr<> with the template parameter inferred by the compiler.
+// This will be present in C++14 and can be removed then.
- * This will be present in C++14 and can be removed then.
- */
 template <typename T, class... Args>
 std::unique_ptr<T> make_unique(Args&&... args) {
   return std::unique_ptr<T>(new T{std::forward<Args>(args)...});
 }
 
-/**
+// Writes a set of items to the std::ostream, joining the times with the provided separator.
- * Writes a set of items to the std::ostream, joining the times with the
- * provided
- * separator.
- */
 template <typename Container>
-::std::function<::std::ostream&(::std::ostream&)> Joiner(
+::std::function<::std::ostream&(::std::ostream&)> Joiner(const Container& container,
-    const Container& container, const char* sep) {
+                                                         const char* sep) {
   using std::begin;
   using std::end;
   const auto begin_iter = begin(container);
@@ -140,32 +126,19 @@ template <typename Container>
   };
 }
 
-/**
+// Helper method to extract a UTF-16 string from a StringPool. If the string is stored as UTF-8,
- * Helper method to extract a UTF-16 string from a StringPool. If the string is
+// the conversion to UTF-16 happens within ResStringPool.
- * stored as UTF-8,
- * the conversion to UTF-16 happens within ResStringPool.
- */
 android::StringPiece16 GetString16(const android::ResStringPool& pool, size_t idx);
 
-/**
+// Helper method to extract a UTF-8 string from a StringPool. If the string is stored as UTF-16,
- * Helper method to extract a UTF-8 string from a StringPool. If the string is
+// the conversion from UTF-16 to UTF-8 does not happen in ResStringPool and is done by this method,
- * stored as UTF-16,
+// which maintains no state or cache. This means we must return an std::string copy.
- * the conversion from UTF-16 to UTF-8 does not happen in ResStringPool and is
- * done by this method,
- * which maintains no state or cache. This means we must return an std::string
- * copy.
- */
 std::string GetString(const android::ResStringPool& pool, size_t idx);
 
-/**
+// Checks that the Java string format contains no non-positional arguments (arguments without
- * Checks that the Java string format contains no non-positional arguments
+// explicitly specifying an index) when there are more than one argument. This is an error
- * (arguments without
+// because translations may rearrange the order of the arguments in the string, which will
- * explicitly specifying an index) when there are more than one argument. This
+// break the string interpolation.
- * is an error
- * because translations may rearrange the order of the arguments in the string,
- * which will
- * break the string interpolation.
- */
 bool VerifyJavaStringFormat(const android::StringPiece& str);
 
 class StringBuilder {
@@ -194,36 +167,38 @@ class StringBuilder {
   std::string error_;
 };
 
-inline const std::string& StringBuilder::ToString() const { return str_; }
+inline const std::string& StringBuilder::ToString() const {
+  return str_;
+}
 
-inline const std::string& StringBuilder::Error() const { return error_; }
+inline const std::string& StringBuilder::Error() const {
+  return error_;
+}
 
-inline bool StringBuilder::IsEmpty() const { return str_.empty(); }
+inline bool StringBuilder::IsEmpty() const {
+  return str_.empty();
+}
 
-inline size_t StringBuilder::Utf16Len() const { return utf16_len_; }
+inline size_t StringBuilder::Utf16Len() const {
+  return utf16_len_;
+}
 
-inline StringBuilder::operator bool() const { return error_.empty(); }
+inline StringBuilder::operator bool() const {
+  return error_.empty();
+}
 
-/**
+// Converts a UTF8 string to a UTF16 string.
- * Converts a UTF8 string to a UTF16 string.
- */
 std::u16string Utf8ToUtf16(const android::StringPiece& utf8);
 std::string Utf16ToUtf8(const android::StringPiece16& utf16);
 
-/**
+// Writes the entire BigBuffer to the output stream.
- * Writes the entire BigBuffer to the output stream.
- */
 bool WriteAll(std::ostream& out, const BigBuffer& buffer);
 
-/*
+// Copies the entire BigBuffer into a single buffer.
- * Copies the entire BigBuffer into a single buffer.
- */
 std::unique_ptr<uint8_t[]> Copy(const BigBuffer& buffer);
 
-/**
+// A Tokenizer implemented as an iterable collection. It does not allocate any memory on the heap
- * A Tokenizer implemented as an iterable collection. It does not allocate
+// nor use standard containers.
- * any memory on the heap nor use standard containers.
- */
 class Tokenizer {
  public:
   class iterator {
@@ -269,38 +244,42 @@ class Tokenizer {
   const iterator end_;
 };
 
-inline Tokenizer Tokenize(const android::StringPiece& str, char sep) { return Tokenizer(str, sep); }
+inline Tokenizer Tokenize(const android::StringPiece& str, char sep) {
+  return Tokenizer(str, sep);
+}
 
-inline uint16_t HostToDevice16(uint16_t value) { return htods(value); }
+inline uint16_t HostToDevice16(uint16_t value) {
+  return htods(value);
+}
 
-inline uint32_t HostToDevice32(uint32_t value) { return htodl(value); }
+inline uint32_t HostToDevice32(uint32_t value) {
+  return htodl(value);
+}
 
-inline uint16_t DeviceToHost16(uint16_t value) { return dtohs(value); }
+inline uint16_t DeviceToHost16(uint16_t value) {
+  return dtohs(value);
+}
 
-inline uint32_t DeviceToHost32(uint32_t value) { return dtohl(value); }
+inline uint32_t DeviceToHost32(uint32_t value) {
+  return dtohl(value);
+}
 
-/**
+// Given a path like: res/xml-sw600dp/foo.xml
- * Given a path like: res/xml-sw600dp/foo.xml
+//
- *
+// Extracts "res/xml-sw600dp/" into outPrefix.
- * Extracts "res/xml-sw600dp/" into outPrefix.
+// Extracts "foo" into outEntry.
- * Extracts "foo" into outEntry.
+// Extracts ".xml" into outSuffix.
- * Extracts ".xml" into outSuffix.
+//
- *
+// Returns true if successful.
- * Returns true if successful.
- */
 bool ExtractResFilePathParts(const android::StringPiece& path, android::StringPiece* out_prefix,
                              android::StringPiece* out_entry, android::StringPiece* out_suffix);
 
 }  // namespace util
 
-/**
+// Stream operator for functions. Calls the function with the stream as an argument.
- * Stream operator for functions. Calls the function with the stream as an
+// In the aapt namespace for lookup.
- * argument.
+inline ::std::ostream& operator<<(::std::ostream& out,
- * In the aapt namespace for lookup.
+                                  const ::std::function<::std::ostream&(::std::ostream&)>& f) {
- */
-inline ::std::ostream& operator<<(
-    ::std::ostream& out,
-    const ::std::function<::std::ostream&(::std::ostream&)>& f) {
   return f(out);
 }
 

tools/aapt2/util/Util_test.cpp

@@ -117,24 +117,46 @@ TEST(UtilTest, IsJavaClassName) {
   EXPECT_TRUE(util::IsJavaClassName("android.test.Class$Inner"));
   EXPECT_TRUE(util::IsJavaClassName("android_test.test.Class"));
   EXPECT_TRUE(util::IsJavaClassName("_android_.test._Class_"));
-  EXPECT_FALSE(util::IsJavaClassName("android.test.$Inner"));
+  EXPECT_TRUE(util::IsJavaClassName("android.test.$Inner"));
-  EXPECT_FALSE(util::IsJavaClassName("android.test.Inner$"));
+  EXPECT_TRUE(util::IsJavaClassName("android.test.Inner$"));
+  EXPECT_TRUE(util::IsJavaClassName("com.foo.FøøBar"));
+
   EXPECT_FALSE(util::IsJavaClassName(".test.Class"));
   EXPECT_FALSE(util::IsJavaClassName("android"));
+  EXPECT_FALSE(util::IsJavaClassName("FooBar"));
 }
 
 TEST(UtilTest, IsJavaPackageName) {
   EXPECT_TRUE(util::IsJavaPackageName("android"));
   EXPECT_TRUE(util::IsJavaPackageName("android.test"));
   EXPECT_TRUE(util::IsJavaPackageName("android.test_thing"));
-  EXPECT_FALSE(util::IsJavaPackageName("_android"));
+  EXPECT_TRUE(util::IsJavaPackageName("_android"));
-  EXPECT_FALSE(util::IsJavaPackageName("android_"));
+  EXPECT_TRUE(util::IsJavaPackageName("android_"));
+  EXPECT_TRUE(util::IsJavaPackageName("android._test"));
+  EXPECT_TRUE(util::IsJavaPackageName("cøm.foo"));
+
   EXPECT_FALSE(util::IsJavaPackageName("android."));
   EXPECT_FALSE(util::IsJavaPackageName(".android"));
-  EXPECT_FALSE(util::IsJavaPackageName("android._test"));
   EXPECT_FALSE(util::IsJavaPackageName(".."));
 }
 
+TEST(UtilTest, IsAndroidPackageName) {
+  EXPECT_TRUE(util::IsAndroidPackageName("android"));
+  EXPECT_TRUE(util::IsAndroidPackageName("android.test"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo.test_thing"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo.testing_thing_"));
+  EXPECT_TRUE(util::IsAndroidPackageName("com.foo.test_99_"));
+
+  EXPECT_FALSE(util::IsAndroidPackageName("android._test"));
+  EXPECT_FALSE(util::IsAndroidPackageName("com"));
+  EXPECT_FALSE(util::IsAndroidPackageName("_android"));
+  EXPECT_FALSE(util::IsAndroidPackageName("android."));
+  EXPECT_FALSE(util::IsAndroidPackageName(".android"));
+  EXPECT_FALSE(util::IsAndroidPackageName(".."));
+  EXPECT_FALSE(util::IsAndroidPackageName("cøm.foo"));
+}
+
 TEST(UtilTest, FullyQualifiedClassName) {
   EXPECT_THAT(util::GetFullyQualifiedClassName("android", ".asdf"), Eq("android.asdf"));
   EXPECT_THAT(util::GetFullyQualifiedClassName("android", ".a.b"), Eq("android.a.b"));

tools/aapt2/xml/XmlActionExecutor.cpp

@@ -16,6 +16,8 @@
 
 #include "xml/XmlActionExecutor.h"
 
+using ::android::StringPiece;
+
 namespace aapt {
 namespace xml {
 
@@ -46,8 +48,8 @@ static void PrintElementToDiagMessage(const Element* el, DiagMessage* msg) {
   *msg << el->name << ">";
 }
 
-bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostics* diag,
+bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, std::vector<StringPiece>* bread_crumb,
-                            Element* el) const {
+                            SourcePathDiagnostics* diag, Element* el) const {
   bool error = false;
   for (const ActionFuncWithDiag& action : actions_) {
     error |= !action(el, diag);
@@ -57,17 +59,29 @@ bool XmlNodeAction::Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostic
     if (child_el->namespace_uri.empty()) {
       std::map<std::string, XmlNodeAction>::const_iterator iter = map_.find(child_el->name);
       if (iter != map_.end()) {
-        error |= !iter->second.Execute(policy, diag, child_el);
+        // Use the iterator's copy of the element name, because the element may be modified.
+        bread_crumb->push_back(iter->first);
+        error |= !iter->second.Execute(policy, bread_crumb, diag, child_el);
+        bread_crumb->pop_back();
         continue;
       }
 
-      if (policy == XmlActionExecutorPolicy::kWhitelist) {
+      if (policy != XmlActionExecutorPolicy::kNone) {
         DiagMessage error_msg(child_el->line_number);
-        error_msg << "unknown element ";
+        error_msg << "unexpected element ";
         PrintElementToDiagMessage(child_el, &error_msg);
-        error_msg << " found";
+        error_msg << " found in ";
-        diag->Error(error_msg);
+        for (const StringPiece& element : *bread_crumb) {
-        error = true;
+          error_msg << "<" << element << ">";
+        }
+        if (policy == XmlActionExecutorPolicy::kWhitelistWarning) {
+          // Treat the error only as a warning.
+          diag->Warn(error_msg);
+        } else {
+          // Policy is XmlActionExecutorPolicy::kWhitelist, we should fail.
+          diag->Error(error_msg);
+          error = true;
+        }
       }
     }
   }
@@ -90,14 +104,15 @@ bool XmlActionExecutor::Execute(XmlActionExecutorPolicy policy, IDiagnostics* di
   if (el->namespace_uri.empty()) {
     std::map<std::string, XmlNodeAction>::const_iterator iter = map_.find(el->name);
     if (iter != map_.end()) {
-      return iter->second.Execute(policy, &source_diag, el);
+      std::vector<StringPiece> bread_crumb;
+      bread_crumb.push_back(iter->first);
+      return iter->second.Execute(policy, &bread_crumb, &source_diag, el);
     }
 
     if (policy == XmlActionExecutorPolicy::kWhitelist) {
       DiagMessage error_msg(el->line_number);
-      error_msg << "unknown element ";
+      error_msg << "unexpected root element ";
       PrintElementToDiagMessage(el, &error_msg);
-      error_msg << " found";
       source_diag.Error(error_msg);
       return false;
     }

tools/aapt2/xml/XmlActionExecutor.h

@@ -34,62 +34,57 @@ enum class XmlActionExecutorPolicy {
   // Actions are run if elements are matched, errors occur only when actions return false.
   kNone,
 
-  // The actions defined must match and run. If an element is found that does
+  // The actions defined must match and run. If an element is found that does not match an action,
-  // not match an action, an error occurs.
+  // an error occurs.
   // Note: namespaced elements are always ignored.
   kWhitelist,
+
+  // The actions defined should match and run. if an element is found that does not match an
+  // action, a warning is printed.
+  // Note: namespaced elements are always ignored.
+  kWhitelistWarning,
 };
 
-/**
+// Contains the actions to perform at this XML node. This is a recursive data structure that
- * Contains the actions to perform at this XML node. This is a recursive data
+// holds XmlNodeActions for child XML nodes.
- * structure that
- * holds XmlNodeActions for child XML nodes.
- */
 class XmlNodeAction {
  public:
   using ActionFuncWithDiag = std::function<bool(Element*, SourcePathDiagnostics*)>;
   using ActionFunc = std::function<bool(Element*)>;
 
-  /**
+  // Find or create a child XmlNodeAction that will be performed for the child element with the
-   * Find or create a child XmlNodeAction that will be performed for the child
+  // name `name`.
-   * element with the name `name`.
+  XmlNodeAction& operator[](const std::string& name) {
-   */
+    return map_[name];
-  XmlNodeAction& operator[](const std::string& name) { return map_[name]; }
+  }
 
-  /**
+  // Add an action to be performed at this XmlNodeAction.
-   * Add an action to be performed at this XmlNodeAction.
-   */
   void Action(ActionFunc f);
   void Action(ActionFuncWithDiag);
 
  private:
   friend class XmlActionExecutor;
 
-  bool Execute(XmlActionExecutorPolicy policy, SourcePathDiagnostics* diag, Element* el) const;
+  bool Execute(XmlActionExecutorPolicy policy, std::vector<::android::StringPiece>* bread_crumb,
+               SourcePathDiagnostics* diag, Element* el) const;
 
   std::map<std::string, XmlNodeAction> map_;
   std::vector<ActionFuncWithDiag> actions_;
 };
 
-/**
+// Allows the definition of actions to execute at specific XML elements defined by their hierarchy.
- * Allows the definition of actions to execute at specific XML elements defined
- * by their
- * hierarchy.
- */
 class XmlActionExecutor {
  public:
   XmlActionExecutor() = default;
 
-  /**
+  // Find or create a root XmlNodeAction that will be performed for the root XML element with the
-   * Find or create a root XmlNodeAction that will be performed for the root XML
+  // name `name`.
-   * element with the name `name`.
+  XmlNodeAction& operator[](const std::string& name) {
-   */
+    return map_[name];
-  XmlNodeAction& operator[](const std::string& name) { return map_[name]; }
+  }
 
-  /**
+  // Execute the defined actions for this XmlResource.
-   * Execute the defined actions for this XmlResource.
+  // Returns true if all actions return true, otherwise returns false.
-   * Returns true if all actions return true, otherwise returns false.
-   */
   bool Execute(XmlActionExecutorPolicy policy, IDiagnostics* diag, XmlResource* doc) const;
 
  private:

tools/aapt2/xml/XmlActionExecutor_test.cpp

@@ -56,9 +56,13 @@ TEST(XmlActionExecutorTest, FailsWhenUndefinedHierarchyExists) {
   XmlActionExecutor executor;
   executor["manifest"]["application"];
 
-  std::unique_ptr<XmlResource> doc =
+  std::unique_ptr<XmlResource> doc;
-      test::BuildXmlDom("<manifest><application /><activity /></manifest>");
   StdErrDiagnostics diag;
+
+  doc = test::BuildXmlDom("<manifest><application /><activity /></manifest>");
+  ASSERT_FALSE(executor.Execute(XmlActionExecutorPolicy::kWhitelist, &diag, doc.get()));
+
+  doc = test::BuildXmlDom("<manifest><application><activity /></application></manifest>");
   ASSERT_FALSE(executor.Execute(XmlActionExecutorPolicy::kWhitelist, &diag, doc.get()));
 }