Fix loaded apk string pool order

Loading in an APk changed the ordering of strings in the string pool.
When loading an apk, assign the strings to the same index as they
are in the ResStringPool.

Bug: 118831219
Test: "aapt2 dump strings left.apk" prints in the correct order,
"aapt2 convert left.apk --output-format binary -o left_binary.apk" has
entries in the correct order, and aapt2_tests

Change-Id: I00014c02195f39c1152a110e90083d9b14e9216e

9 files changed, 158 insertions, 107 deletions

diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp
index 0bc5221245ca..583f14ac0cbd 100644
--- a/tools/aapt2/Debug.cpp
+++ b/tools/aapt2/Debug.cpp
@@ -431,7 +431,7 @@ void Debug::DumpHex(const void* data, size_t len) {
 
 void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer* printer) {
   using namespace android;
-  
+
   if (pool->getError() == NO_INIT) {
     printer->Print("String pool is unitialized.\n");
     return;
@@ -460,7 +460,7 @@ void Debug::DumpResStringPool(const android::ResStringPool* pool, text::Printer*
   const size_t NS = pool->size();
   for (size_t s=0; s<NS; s++) {
     String8 str = pool->string8ObjectAt(s);
-    printer->Print(StringPrintf("String #%zd : %s\n", s, str.string()));
+    printer->Print(StringPrintf("String #%zd: %s\n", s, str.string()));
   }
 }
 
diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp
index dbe5ac5adb98..da22e885b917 100644
--- a/tools/aapt2/ResourceUtils.cpp
+++ b/tools/aapt2/ResourceUtils.cpp
@@ -718,7 +718,7 @@ std::unique_ptr<Item> ParseBinaryResValue(const ResourceType& type, const Config
           // This must be a FileReference.
           std::unique_ptr<FileReference> file_ref =
               util::make_unique<FileReference>(dst_pool->MakeRef(
-                  str, StringPool::Context(StringPool::Context::kHighPriority, config)));
+                  str, StringPool::Context(StringPool::Context::kHighPriority, config), data));
           if (type == ResourceType::kRaw) {
             file_ref->type = ResourceFile::Type::kUnknown;
           } else if (util::EndsWith(*file_ref->path, ".xml")) {
@@ -730,7 +730,7 @@ std::unique_ptr<Item> ParseBinaryResValue(const ResourceType& type, const Config
         }
 
         // There are no styles associated with this string, so treat it as a simple string.
-        return util::make_unique<String>(dst_pool->MakeRef(str, StringPool::Context(config)));
+        return util::make_unique<String>(dst_pool->MakeRef(str, StringPool::Context(config), data));
       }
     } break;
 
diff --git a/tools/aapt2/StringPool.cpp b/tools/aapt2/StringPool.cpp
index 8eabd3225d87..a8c26668b3a5 100644
--- a/tools/aapt2/StringPool.cpp
+++ b/tools/aapt2/StringPool.cpp
@@ -165,12 +165,13 @@ StringPool::Ref StringPool::MakeRef(const StringPiece& str) {
   return MakeRefImpl(str, Context{}, true);
 }
 
-StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context) {
-  return MakeRefImpl(str, context, true);
+StringPool::Ref StringPool::MakeRef(const StringPiece& str, const Context& context,
+                                    Maybe<size_t> index) {
+  return MakeRefImpl(str, context, true, index);
 }
 
 StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& context,
-                                        bool unique) {
+                                        bool unique, Maybe<size_t> index) {
   if (unique) {
     auto range = indexed_strings_.equal_range(str);
     for (auto iter = range.first; iter != range.second; ++iter) {
@@ -180,15 +181,26 @@ StringPool::Ref StringPool::MakeRefImpl(const StringPiece& str, const Context& c
     }
   }
 
+  const size_t size = strings_.size();
+  // Insert the string at the end of the string vector if no index is specified
+  const size_t insertion_index = index ? index.value() : size;
+
   std::unique_ptr<Entry> entry(new Entry());
   entry->value = str.to_string();
   entry->context = context;
-  entry->index_ = strings_.size();
+  entry->index_ = insertion_index;
   entry->ref_ = 0;
   entry->pool_ = this;
 
   Entry* borrow = entry.get();
-  strings_.emplace_back(std::move(entry));
+  if (insertion_index == size) {
+    strings_.emplace_back(std::move(entry));
+  } else {
+    // Allocate enough space for the string at the index
+    strings_.resize(std::max(insertion_index + 1, size));
+    strings_[insertion_index] = std::move(entry);
+  }
+
   indexed_strings_.insert(std::make_pair(StringPiece(borrow->value), borrow));
   return Ref(borrow);
 }
diff --git a/tools/aapt2/StringPool.h b/tools/aapt2/StringPool.h
index 1006ca970dc5..115d5d315b8f 100644
--- a/tools/aapt2/StringPool.h
+++ b/tools/aapt2/StringPool.h
@@ -166,7 +166,8 @@ class StringPool {
 
   // Adds a string to the pool, unless it already exists, with a context object that can be used
   // when sorting the string pool. Returns a reference to the string in the pool.
-  Ref MakeRef(const android::StringPiece& str, const Context& context);
+  Ref MakeRef(const android::StringPiece& str, const Context& context,
+              Maybe<size_t> index = {});
 
   // Adds a string from another string pool. Returns a reference to the string in the string pool.
   Ref MakeRef(const Ref& ref);
@@ -210,7 +211,8 @@ class StringPool {
 
   static bool Flatten(BigBuffer* out, const StringPool& pool, bool utf8, IDiagnostics* diag);
 
-  Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique);
+  Ref MakeRefImpl(const android::StringPiece& str, const Context& context, bool unique,
+                  Maybe<size_t> index = {});
   void ReAssignIndices();
 
   std::vector<std::unique_ptr<Entry>> strings_;
diff --git a/tools/aapt2/StringPool_test.cpp b/tools/aapt2/StringPool_test.cpp
index 9a7238b584ba..648be7d33754 100644
--- a/tools/aapt2/StringPool_test.cpp
+++ b/tools/aapt2/StringPool_test.cpp
@@ -84,6 +84,24 @@ TEST(StringPoolTest, MaintainInsertionOrderIndex) {
   EXPECT_THAT(ref_c.index(), Eq(2u));
 }
 
+TEST(StringPoolTest, AssignStringIndex) {
+  StringPool pool;
+
+  StringPool::Ref ref_a = pool.MakeRef("0", StringPool::Context{}, 0u);
+  StringPool::Ref ref_b = pool.MakeRef("1", StringPool::Context{}, 1u);
+  StringPool::Ref ref_c = pool.MakeRef("5", StringPool::Context{}, 5u);
+  StringPool::Ref ref_d = pool.MakeRef("2", StringPool::Context{}, 2u);
+  StringPool::Ref ref_e = pool.MakeRef("4", StringPool::Context{}, 4u);
+  StringPool::Ref ref_f = pool.MakeRef("3", StringPool::Context{}, 3u);
+
+  EXPECT_THAT(ref_a.index(), Eq(0u));
+  EXPECT_THAT(ref_b.index(), Eq(1u));
+  EXPECT_THAT(ref_d.index(), Eq(2u));
+  EXPECT_THAT(ref_f.index(), Eq(3u));
+  EXPECT_THAT(ref_e.index(), Eq(4u));
+  EXPECT_THAT(ref_c.index(), Eq(5u));
+}
+
 TEST(StringPoolTest, PruneStringsWithNoReferences) {
   StringPool pool;
 
diff --git a/tools/aapt2/cmd/Convert.cpp b/tools/aapt2/cmd/Convert.cpp
index 3ea17552ea7c..4492f6b49cf0 100644
--- a/tools/aapt2/cmd/Convert.cpp
+++ b/tools/aapt2/cmd/Convert.cpp
@@ -57,78 +57,6 @@ class IApkSerializer {
   Source source_;
 };
 
-bool ConvertApk(IAaptContext* context, unique_ptr<LoadedApk> apk, IApkSerializer* serializer,
-                IArchiveWriter* writer) {
-  io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath);
-  if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/, writer,
-                                (manifest != nullptr && manifest->WasCompressed())
-                                    ? ArchiveEntry::kCompress : 0u)) {
-    context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
-                                     << "failed to serialize AndroidManifest.xml");
-    return false;
-  }
-
-  if (apk->GetResourceTable() != nullptr) {
-    // The table might be modified by below code.
-    auto converted_table = apk->GetResourceTable();
-
-    // Resources
-    for (const auto& package : converted_table->packages) {
-      for (const auto& type : package->types) {
-        for (const auto& entry : type->entries) {
-          for (const auto& config_value : entry->values) {
-            FileReference* file = ValueCast<FileReference>(config_value->value.get());
-            if (file != nullptr) {
-              if (file->file == nullptr) {
-                context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
-                                                 << "no file associated with " << *file);
-                return false;
-              }
-
-              if (!serializer->SerializeFile(file, writer)) {
-                context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
-                                                 << "failed to serialize file " << *file->path);
-                return false;
-              }
-            } // file
-          } // config_value
-        } // entry
-      } // type
-    } // package
-
-    // Converted resource table
-    if (!serializer->SerializeTable(converted_table, writer)) {
-      context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
-                                       << "failed to serialize the resource table");
-      return false;
-    }
-  }
-
-  // Other files
-  std::unique_ptr<io::IFileCollectionIterator> iterator = apk->GetFileCollection()->Iterator();
-  while (iterator->HasNext()) {
-    io::IFile* file = iterator->Next();
-    std::string path = file->GetSource().path;
-
-    // Manifest, resource table and resources have already been taken care of.
-    if (path == kAndroidManifestPath ||
-        path == kApkResourceTablePath ||
-        path == kProtoResourceTablePath ||
-        path.find("res/") == 0) {
-      continue;
-    }
-
-    if (!io::CopyFileToArchivePreserveCompression(context, file, path, writer)) {
-      context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
-                                       << "failed to copy file " << path);
-      return false;
-    }
-  }
-
-  return true;
-}
-
-
 class BinaryApkSerializer : public IApkSerializer {
  public:
   BinaryApkSerializer(IAaptContext* context, const Source& source,
@@ -323,12 +251,97 @@ class Context : public IAaptContext {
   StdErrDiagnostics diag_;
 };
 
+int Convert(IAaptContext* context, LoadedApk* apk, IArchiveWriter* output_writer,
+            ApkFormat output_format, TableFlattenerOptions& options) {
+  // Do not change the ordering of strings in the values string pool
+  options.sort_stringpool_entries = false;
+
+  unique_ptr<IApkSerializer> serializer;
+  if (output_format == ApkFormat::kBinary) {
+    serializer.reset(new BinaryApkSerializer(context, apk->GetSource(), options));
+  } else if (output_format == ApkFormat::kProto) {
+    serializer.reset(new ProtoApkSerializer(context, apk->GetSource()));
+  } else {
+    context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+                                     << "Cannot convert APK to unknown format");
+    return 1;
+  }
+
+  io::IFile* manifest = apk->GetFileCollection()->FindFile(kAndroidManifestPath);
+  if (!serializer->SerializeXml(apk->GetManifest(), kAndroidManifestPath, true /*utf16*/,
+                                output_writer, (manifest != nullptr && manifest->WasCompressed())
+                                ? ArchiveEntry::kCompress : 0u)) {
+    context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+                                     << "failed to serialize AndroidManifest.xml");
+    return 1;
+  }
+
+  if (apk->GetResourceTable() != nullptr) {
+    // The table might be modified by below code.
+    auto converted_table = apk->GetResourceTable();
+
+    // Resources
+    for (const auto& package : converted_table->packages) {
+      for (const auto& type : package->types) {
+        for (const auto& entry : type->entries) {
+          for (const auto& config_value : entry->values) {
+            FileReference* file = ValueCast<FileReference>(config_value->value.get());
+            if (file != nullptr) {
+              if (file->file == nullptr) {
+                context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+                                                 << "no file associated with " << *file);
+                return 1;
+              }
+
+              if (!serializer->SerializeFile(file, output_writer)) {
+                context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+                                                 << "failed to serialize file " << *file->path);
+                return 1;
+              }
+            } // file
+          } // config_value
+        } // entry
+      } // type
+    } // package
+
+    // Converted resource table
+    if (!serializer->SerializeTable(converted_table, output_writer)) {
+      context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+                                       << "failed to serialize the resource table");
+      return 1;
+    }
+  }
+
+  // Other files
+  std::unique_ptr<io::IFileCollectionIterator> iterator = apk->GetFileCollection()->Iterator();
+  while (iterator->HasNext()) {
+    io::IFile* file = iterator->Next();
+    std::string path = file->GetSource().path;
+
+    // Manifest, resource table and resources have already been taken care of.
+    if (path == kAndroidManifestPath ||
+        path == kApkResourceTablePath ||
+        path == kProtoResourceTablePath ||
+        path.find("res/") == 0) {
+      continue;
+    }
+
+    if (!io::CopyFileToArchivePreserveCompression(context, file, path, output_writer)) {
+      context->GetDiagnostics()->Error(DiagMessage(apk->GetSource())
+                                       << "failed to copy file " << path);
+      return 1;
+    }
+  }
+
+  return 0;
+}
+
 const char* ConvertCommand::kOutputFormatProto = "proto";
 const char* ConvertCommand::kOutputFormatBinary = "binary";
 
 int ConvertCommand::Action(const std::vector<std::string>& args) {
   if (args.size() != 1) {
-    std::cerr << "must supply a single proto APK\n";
+    std::cerr << "must supply a single APK\n";
     Usage(&std::cerr);
     return 1;
   }
@@ -341,34 +354,31 @@ int ConvertCommand::Action(const std::vector<std::string>& args) {
     return 1;
   }
 
-  Maybe<AppInfo> app_info =
-      ExtractAppInfoFromBinaryManifest(*apk->GetManifest(), context.GetDiagnostics());
+  Maybe<AppInfo> app_info = ExtractAppInfoFromBinaryManifest(*apk->GetManifest(),
+                                                             context.GetDiagnostics());
   if (!app_info) {
     return 1;
   }
 
   context.package_ = app_info.value().package;
-
-  unique_ptr<IArchiveWriter> writer =
-      CreateZipFileArchiveWriter(context.GetDiagnostics(), output_path_);
+  unique_ptr<IArchiveWriter> writer = CreateZipFileArchiveWriter(context.GetDiagnostics(),
+                                                                 output_path_);
   if (writer == nullptr) {
     return 1;
   }
 
-  unique_ptr<IApkSerializer> serializer;
+  ApkFormat format;
   if (!output_format_ || output_format_.value() == ConvertCommand::kOutputFormatBinary) {
-
-    serializer.reset(new BinaryApkSerializer(&context, apk->GetSource(), options_));
+    format = ApkFormat::kBinary;
   } else if (output_format_.value() == ConvertCommand::kOutputFormatProto) {
-    serializer.reset(new ProtoApkSerializer(&context, apk->GetSource()));
+    format = ApkFormat::kProto;
   } else {
-    context.GetDiagnostics()->Error(DiagMessage(path)
-        << "Invalid value for flag --output-format: "
-        << output_format_.value());
+    context.GetDiagnostics()->Error(DiagMessage(path) << "Invalid value for flag --output-format: "
+                                                      << output_format_.value());
     return 1;
   }
 
-  return ConvertApk(&context, std::move(apk), serializer.get(), writer.get()) ? 0 : 1;
+  return Convert(&context, apk.get(), writer.get(), format, options_);
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/cmd/Convert.h b/tools/aapt2/cmd/Convert.h
index fcec23d16f78..6a6719c91b58 100644
--- a/tools/aapt2/cmd/Convert.h
+++ b/tools/aapt2/cmd/Convert.h
@@ -18,6 +18,7 @@
 #define AAPT2_CONVERT_H
 
 #include "Command.h"
+#include "LoadedApk.h"
 #include "format/binary/TableFlattener.h"
 
 namespace aapt {
@@ -49,6 +50,9 @@ class ConvertCommand : public Command {
   bool verbose_ = false;
 };
 
-}// namespace aapt
+int Convert(IAaptContext* context, LoadedApk* input, IArchiveWriter* output_writer,
+            ApkFormat output_format, TableFlattenerOptions& options);
+
+}  // namespace aapt
 
 #endif //AAPT2_CONVERT_H
diff --git a/tools/aapt2/format/binary/TableFlattener.cpp b/tools/aapt2/format/binary/TableFlattener.cpp
index 8a86f63a30c1..6c1a9ba2cbad 100644
--- a/tools/aapt2/format/binary/TableFlattener.cpp
+++ b/tools/aapt2/format/binary/TableFlattener.cpp
@@ -573,15 +573,17 @@ class PackageFlattener {
 }  // namespace
 
 bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) {
-  // We must do this before writing the resources, since the string pool IDs may change.
-  table->string_pool.Prune();
-  table->string_pool.Sort([](const StringPool::Context& a, const StringPool::Context& b) -> int {
-    int diff = util::compare(a.priority, b.priority);
-    if (diff == 0) {
-      diff = a.config.compare(b.config);
-    }
-    return diff;
-  });
+  if (options_.sort_stringpool_entries) {
+    // We must do this before writing the resources, since the string pool IDs may change.
+    table->string_pool.Prune();
+    table->string_pool.Sort([](const StringPool::Context &a, const StringPool::Context &b) -> int {
+      int diff = util::compare(a.priority, b.priority);
+      if (diff == 0) {
+        diff = a.config.compare(b.config);
+      }
+      return diff;
+    });
+  }
 
   // Write the ResTable header.
   ChunkWriter table_writer(buffer_);
diff --git a/tools/aapt2/format/binary/TableFlattener.h b/tools/aapt2/format/binary/TableFlattener.h
index c2e1d4b4ce8c..635cb21f514c 100644
--- a/tools/aapt2/format/binary/TableFlattener.h
+++ b/tools/aapt2/format/binary/TableFlattener.h
@@ -43,6 +43,9 @@ struct TableFlattenerOptions {
 
   // Set of whitelisted resource names to avoid altering in key stringpool
   std::set<std::string> whitelisted_resources;
+
+  // When true, sort the entries in the values string pool by priority and configuration.
+  bool sort_stringpool_entries = true;
 };
 
 class TableFlattener : public IResourceTableConsumer {