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

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();
     }

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;

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

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 {};
-
-  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);
-    }
-
-    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);
-
-    const Value* 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);
-
-    const Value* expected_;
-  };
-
-  ValueEq(const Value& expected) : expected_(&expected) {
+  explicit ValueEqImpl(const Value* expected) : expected_(expected) {
   }
-  ValueEq(const Value* 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_));
+  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(ValueEqImpl);
+
   const Value* expected_;
 };
 
-// MATCHER_P(ValueEq, a,
-//          std::string(negation ? "isn't" : "is") + " equal to " + ::testing::PrintToString(a)) {
-//  return arg.Equals(&a);
-//}
+template <typename TValue>
+class ValueEqMatcher {
+ public:
+  ValueEqMatcher(TValue expected) : expected_(std::move(expected)) {
+  }
+
+  template <typename T>
+  operator ::testing::Matcher<T>() const {
+    return ::testing::Matcher<T>(new ValueEqImpl<T>(&expected_));
+  }
+
+ private:
+  TValue expected_;
+};
+
+template <typename TValue>
+class ValueEqPointerMatcher {
+ public:
+  ValueEqPointerMatcher(const TValue* expected) : expected_(expected) {
+  }
+
+  template <typename T>
+  operator ::testing::Matcher<T>() const {
+    return ::testing::Matcher<T>(new ValueEqImpl<T>(expected_));
+  }
+
+ private:
+  const TValue* expected_;
+};
+
+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)) {

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));
     }

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"));