AAPT2: Fix issue with deserializing binary XML

We assumed that a raw text value set for an attribute meant there
were no compiled values set either.

This would only really happen for attributes that did not belong to any
namespace (no prefix:), since we always kept their raw string values
in case some code relied on it.

Bug: 72700446
Test: make aapt2_tests
Change-Id: Icba40a1d4b181bfe7cad73131c4dbe5ba7f8b085

6 files changed, 94 insertions, 84 deletions

diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp
index 249557af921e..f064cb14248f 100644
--- a/tools/aapt2/Debug.cpp
+++ b/tools/aapt2/Debug.cpp
@@ -450,7 +450,15 @@ class XmlPrinter : public xml::ConstVisitor {
       if (attr.compiled_value != nullptr) {
         attr.compiled_value->PrettyPrint(printer_);
       } else {
+        printer_->Print("\"");
         printer_->Print(attr.value);
+        printer_->Print("\"");
+      }
+
+      if (!attr.value.empty()) {
+        printer_->Print(" (Raw: \"");
+        printer_->Print(attr.value);
+        printer_->Print("\")");
       }
       printer_->Println();
     }
diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp
index d80307cd154a..7f956c525bed 100644
--- a/tools/aapt2/cmd/Convert.cpp
+++ b/tools/aapt2/cmd/Convert.cpp
@@ -139,6 +139,7 @@ class BinaryApkSerializer : public IApkSerializer {
     BigBuffer buffer(4096);
     XmlFlattenerOptions options = {};
     options.use_utf16 = utf16;
+    options.keep_raw_values = true;
     XmlFlattener flattener(&buffer, options);
     if (!flattener.Consume(context_, xml)) {
       return false;
diff --git a/tools/aapt2/link/XmlCompatVersioner_test.cpp b/tools/aapt2/link/XmlCompatVersioner_test.cpp
index 1ed4536c4566..a98ab0f76de4 100644
--- a/tools/aapt2/link/XmlCompatVersioner_test.cpp
+++ b/tools/aapt2/link/XmlCompatVersioner_test.cpp
@@ -23,6 +23,7 @@ using ::aapt::test::ValueEq;
 using ::testing::Eq;
 using ::testing::IsNull;
 using ::testing::NotNull;
+using ::testing::Pointee;
 using ::testing::SizeIs;
 
 namespace aapt {
@@ -287,13 +288,13 @@ TEST_F(XmlCompatVersionerTest, DegradeRuleOverridesExistingAttribute) {
   ASSERT_THAT(attr, NotNull());
   ASSERT_THAT(attr->compiled_value, NotNull());
   ASSERT_TRUE(attr->compiled_attribute);
-  ASSERT_THAT(*attr->compiled_value, ValueEq(padding_horizontal_value));
+  ASSERT_THAT(attr->compiled_value, Pointee(ValueEq(padding_horizontal_value)));
 
   attr = el->FindAttribute(xml::kSchemaAndroid, "paddingRight");
   ASSERT_THAT(attr, NotNull());
   ASSERT_THAT(attr->compiled_value, NotNull());
   ASSERT_TRUE(attr->compiled_attribute);
-  ASSERT_THAT(*attr->compiled_value, ValueEq(padding_horizontal_value));
+  ASSERT_THAT(attr->compiled_value, Pointee(ValueEq(padding_horizontal_value)));
 
   EXPECT_THAT(versioned_docs[1]->file.config.sdkVersion, Eq(SDK_LOLLIPOP_MR1));
   el = versioned_docs[1]->root.get();
@@ -302,21 +303,20 @@ TEST_F(XmlCompatVersionerTest, DegradeRuleOverridesExistingAttribute) {
 
   attr = el->FindAttribute(xml::kSchemaAndroid, "paddingHorizontal");
   ASSERT_THAT(attr, NotNull());
-  ASSERT_THAT(attr->compiled_value, NotNull());
   ASSERT_TRUE(attr->compiled_attribute);
-  ASSERT_THAT(*attr->compiled_value, ValueEq(padding_horizontal_value));
+  ASSERT_THAT(attr->compiled_value, Pointee(ValueEq(padding_horizontal_value)));
 
   attr = el->FindAttribute(xml::kSchemaAndroid, "paddingLeft");
   ASSERT_THAT(attr, NotNull());
   ASSERT_THAT(attr->compiled_value, NotNull());
   ASSERT_TRUE(attr->compiled_attribute);
-  ASSERT_THAT(*attr->compiled_value, ValueEq(padding_horizontal_value));
+  ASSERT_THAT(attr->compiled_value, Pointee(ValueEq(padding_horizontal_value)));
 
   attr = el->FindAttribute(xml::kSchemaAndroid, "paddingRight");
   ASSERT_THAT(attr, NotNull());
   ASSERT_THAT(attr->compiled_value, NotNull());
   ASSERT_TRUE(attr->compiled_attribute);
-  ASSERT_THAT(*attr->compiled_value, ValueEq(padding_horizontal_value));
+  ASSERT_THAT(attr->compiled_value, Pointee(ValueEq(padding_horizontal_value)));
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/test/Common.h b/tools/aapt2/test/Common.h
index 4e318a92f3fa..aca161a5189d 100644
--- a/tools/aapt2/test/Common.h
+++ b/tools/aapt2/test/Common.h
@@ -146,97 +146,70 @@ MATCHER_P(StrEq, a,
   return android::StringPiece16(arg) == a;
 }
 
-class ValueEq {
+template <typename T>
+class ValueEqImpl : public ::testing::MatcherInterface<T> {
  public:
-  template <typename arg_type>
-  class BaseImpl : public ::testing::MatcherInterface<arg_type> {
-    BaseImpl(const BaseImpl&) = default;
-
-    void DescribeTo(::std::ostream* os) const override {
-      *os << "is equal to " << *expected_;
-    }
-
-    void DescribeNegationTo(::std::ostream* os) const override {
-      *os << "is not equal to " << *expected_;
-    }
-
-   protected:
-    BaseImpl(const Value* expected) : expected_(expected) {
-    }
-
-    const Value* expected_;
-  };
-
-  template <typename T, bool>
-  class Impl {};
+  explicit ValueEqImpl(const Value* expected) : expected_(expected) {
+  }
 
-  template <typename T>
-  class Impl<T, false> : public ::testing::MatcherInterface<T> {
-   public:
-    explicit Impl(const Value* expected) : expected_(expected) {
-    }
+  bool MatchAndExplain(T x, ::testing::MatchResultListener* listener) const override {
+    return expected_->Equals(&x);
+  }
 
-    bool MatchAndExplain(T x, ::testing::MatchResultListener* listener) const override {
-      return expected_->Equals(&x);
-    }
+  void DescribeTo(::std::ostream* os) const override {
+    *os << "is equal to " << *expected_;
+  }
 
-    void DescribeTo(::std::ostream* os) const override {
-      *os << "is equal to " << *expected_;
-    }
+  void DescribeNegationTo(::std::ostream* os) const override {
+    *os << "is not equal to " << *expected_;
+  }
 
-    void DescribeNegationTo(::std::ostream* os) const override {
-      *os << "is not equal to " << *expected_;
-    }
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ValueEqImpl);
 
-   private:
-    DISALLOW_COPY_AND_ASSIGN(Impl);
+  const Value* expected_;
+};
 
-    const Value* expected_;
-  };
+template <typename TValue>
+class ValueEqMatcher {
+ public:
+  ValueEqMatcher(TValue expected) : expected_(std::move(expected)) {
+  }
 
   template <typename T>
-  class Impl<T, true> : public ::testing::MatcherInterface<T> {
-   public:
-    explicit Impl(const Value* expected) : expected_(expected) {
-    }
-
-    bool MatchAndExplain(T x, ::testing::MatchResultListener* listener) const override {
-      return expected_->Equals(x);
-    }
-
-    void DescribeTo(::std::ostream* os) const override {
-      *os << "is equal to " << *expected_;
-    }
-
-    void DescribeNegationTo(::std::ostream* os) const override {
-      *os << "is not equal to " << *expected_;
-    }
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(Impl);
+  operator ::testing::Matcher<T>() const {
+    return ::testing::Matcher<T>(new ValueEqImpl<T>(&expected_));
+  }
 
-    const Value* expected_;
-  };
+ private:
+  TValue expected_;
+};
 
-  ValueEq(const Value& expected) : expected_(&expected) {
-  }
-  ValueEq(const Value* expected) : expected_(expected) {
+template <typename TValue>
+class ValueEqPointerMatcher {
+ public:
+  ValueEqPointerMatcher(const TValue* expected) : expected_(expected) {
   }
-  ValueEq(const ValueEq&) = default;
 
   template <typename T>
   operator ::testing::Matcher<T>() const {
-    return ::testing::Matcher<T>(new Impl<T, std::is_pointer<T>::value>(expected_));
+    return ::testing::Matcher<T>(new ValueEqImpl<T>(expected_));
   }
 
  private:
-  const Value* expected_;
+  const TValue* expected_;
 };
 
-// MATCHER_P(ValueEq, a,
-//          std::string(negation ? "isn't" : "is") + " equal to " + ::testing::PrintToString(a)) {
-//  return arg.Equals(&a);
-//}
+template <typename TValue,
+          typename = typename std::enable_if<!std::is_pointer<TValue>::value, void>::type>
+inline ValueEqMatcher<TValue> ValueEq(TValue value) {
+  return ValueEqMatcher<TValue>(std::move(value));
+}
+
+template <typename TValue>
+inline ValueEqPointerMatcher<TValue> ValueEq(const TValue* value) {
+  return ValueEqPointerMatcher<TValue>(value);
+}
 
 MATCHER_P(StrValueEq, a,
           std::string(negation ? "isn't" : "is") + " equal to " + ::testing::PrintToString(a)) {
diff --git a/tools/aapt2/xml/XmlDom.cpp b/tools/aapt2/xml/XmlDom.cpp
index fddb6b8c5d87..7b748ce78cbc 100644
--- a/tools/aapt2/xml/XmlDom.cpp
+++ b/tools/aapt2/xml/XmlDom.cpp
@@ -244,14 +244,13 @@ static void CopyAttributes(Element* el, android::ResXMLParser* parser, StringPoo
       str16 = parser->getAttributeStringValue(i, &len);
       if (str16) {
         attr.value = util::Utf16ToUtf8(StringPiece16(str16, len));
-      } else {
-        android::Res_value res_value;
-        if (parser->getAttributeValue(i, &res_value) > 0) {
-          attr.compiled_value = ResourceUtils::ParseBinaryResValue(
-              ResourceType::kAnim, {}, parser->getStrings(), res_value, out_pool);
-        }
       }
 
+      android::Res_value res_value;
+      if (parser->getAttributeValue(i, &res_value) > 0) {
+        attr.compiled_value = ResourceUtils::ParseBinaryResValue(
+            ResourceType::kAnim, {}, parser->getStrings(), res_value, out_pool);
+      }
 
       el->attributes.push_back(std::move(attr));
     }
diff --git a/tools/aapt2/xml/XmlDom_test.cpp b/tools/aapt2/xml/XmlDom_test.cpp
index e5012d67163d..486b53ada6bb 100644
--- a/tools/aapt2/xml/XmlDom_test.cpp
+++ b/tools/aapt2/xml/XmlDom_test.cpp
@@ -23,8 +23,10 @@
 #include "test/Test.h"
 
 using ::aapt::io::StringInputStream;
+using ::aapt::test::ValueEq;
 using ::testing::Eq;
 using ::testing::NotNull;
+using ::testing::Pointee;
 using ::testing::SizeIs;
 using ::testing::StrEq;
 
@@ -59,6 +61,16 @@ TEST(XmlDomTest, BinaryInflate) {
   doc->root->name = "Layout";
   doc->root->line_number = 2u;
 
+  xml::Attribute attr;
+  attr.name = "text";
+  attr.namespace_uri = kSchemaAndroid;
+  attr.compiled_attribute = AaptAttribute(
+      aapt::Attribute(android::ResTable_map::TYPE_REFERENCE | android::ResTable_map::TYPE_STRING),
+      ResourceId(0x01010001u));
+  attr.value = "@string/foo";
+  attr.compiled_value = test::BuildReference("string/foo", ResourceId(0x7f010000u));
+  doc->root->attributes.push_back(std::move(attr));
+
   NamespaceDecl decl;
   decl.uri = kSchemaAndroid;
   decl.prefix = "android";
@@ -66,7 +78,9 @@ TEST(XmlDomTest, BinaryInflate) {
   doc->root->namespace_decls.push_back(decl);
 
   BigBuffer buffer(4096);
-  XmlFlattener flattener(&buffer, {});
+  XmlFlattenerOptions options;
+  options.keep_raw_values = true;
+  XmlFlattener flattener(&buffer, options);
   ASSERT_TRUE(flattener.Consume(context.get(), doc.get()));
 
   auto block = util::Copy(buffer);
@@ -75,6 +89,21 @@ TEST(XmlDomTest, BinaryInflate) {
 
   EXPECT_THAT(new_doc->root->name, StrEq("Layout"));
   EXPECT_THAT(new_doc->root->line_number, Eq(2u));
+
+  ASSERT_THAT(new_doc->root->attributes, SizeIs(1u));
+  EXPECT_THAT(new_doc->root->attributes[0].name, StrEq("text"));
+  EXPECT_THAT(new_doc->root->attributes[0].namespace_uri, StrEq(kSchemaAndroid));
+
+  // We only check that the resource ID was preserved. There is no where to encode the types that
+  // the Attribute accepts (eg: string|reference).
+  ASSERT_TRUE(new_doc->root->attributes[0].compiled_attribute);
+  EXPECT_THAT(new_doc->root->attributes[0].compiled_attribute.value().id,
+              Eq(make_value(ResourceId(0x01010001u))));
+
+  EXPECT_THAT(new_doc->root->attributes[0].value, StrEq("@string/foo"));
+  EXPECT_THAT(new_doc->root->attributes[0].compiled_value,
+              Pointee(ValueEq(Reference(ResourceId(0x7f010000u)))));
+
   ASSERT_THAT(new_doc->root->namespace_decls, SizeIs(1u));
   EXPECT_THAT(new_doc->root->namespace_decls[0].uri, StrEq(kSchemaAndroid));
   EXPECT_THAT(new_doc->root->namespace_decls[0].prefix, StrEq("android"));