From b2bac12ce882fe1402aa06f395cfc867367c07be Mon Sep 17 00:00:00 2001
From: Mohamed Heikal <mheikal@google.com>
Date: Wed, 17 Jul 2019 17:47:17 -0400
Subject: [PATCH] ResourcePathShortener deterministically handles hash
 collisions

ResourcePathShortener appends one digit at the end of colliding short
file names to disambiguate them. This cl sorts the list of file paths so
that colliding files always get the same disambiguating digit from one
run to the next rather than possibly alternating.

Test: make aapt2_tests

Change-Id: I983a1448c21f2c79d7cdb1de232e7758c04fc256
---
 .../aapt2/optimize/ResourcePathShortener.cpp  | 12 ++++-
 .../optimize/ResourcePathShortener_test.cpp   | 49 +++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/tools/aapt2/optimize/ResourcePathShortener.cpp b/tools/aapt2/optimize/ResourcePathShortener.cpp
index 7f5d10475f93a..7ff9bf5aa8df1 100644
--- a/tools/aapt2/optimize/ResourcePathShortener.cpp
+++ b/tools/aapt2/optimize/ResourcePathShortener.cpp
@@ -16,6 +16,7 @@
 
 #include "optimize/ResourcePathShortener.h"
 
+#include <set>
 #include <unordered_set>
 
 #include "androidfw/StringPiece.h"
@@ -71,10 +72,19 @@ std::string GetShortenedPath(const android::StringPiece& shortened_filename,
   return shortened_path;
 }
 
+// implement custom comparator of FileReference pointers so as to use the
+// underlying filepath as key rather than the integer address. This is to ensure
+// determinism of output for colliding files.
+struct PathComparator {
+    bool operator() (const FileReference* lhs, const FileReference* rhs) const {
+        return lhs->path->compare(*rhs->path);
+    }
+};
+
 bool ResourcePathShortener::Consume(IAaptContext* context, ResourceTable* table) {
   // used to detect collisions
   std::unordered_set<std::string> shortened_paths;
-  std::unordered_set<FileReference*> file_refs;
+  std::set<FileReference*, PathComparator> file_refs;
   for (auto& package : table->packages) {
     for (auto& type : package->types) {
       for (auto& entry : type->entries) {
diff --git a/tools/aapt2/optimize/ResourcePathShortener_test.cpp b/tools/aapt2/optimize/ResourcePathShortener_test.cpp
index 1f45694951868..f5a02be0ea5ef 100644
--- a/tools/aapt2/optimize/ResourcePathShortener_test.cpp
+++ b/tools/aapt2/optimize/ResourcePathShortener_test.cpp
@@ -29,6 +29,14 @@ android::StringPiece GetExtension(android::StringPiece path) {
   return android::StringPiece(iter, path.end() - iter);
 }
 
+void FillTable(aapt::test::ResourceTableBuilder& builder, int start, int end) {
+  for (int i=start; i<end; i++) {
+    builder.AddFileReference(
+        "android:drawable/xmlfile" + std::to_string(i),
+        "res/drawable/xmlfile" + std::to_string(i) + ".xml");
+  }
+}
+
 namespace aapt {
 
 TEST(ResourcePathShortenerTest, FileRefPathsChangedInResourceTable) {
@@ -114,4 +122,45 @@ TEST(ResourcePathShortenerTest, KeepExtensions) {
   EXPECT_THAT(GetExtension(path_map[original_png_path]), Eq(android::StringPiece(".png")));
 }
 
+TEST(ResourcePathShortenerTest, DeterministicallyHandleCollisions) {
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+
+  // 4000 resources is the limit at which the hash space is expanded to 3
+  // letters to reduce collisions, we want as many collisions as possible thus
+  // N-1.
+  const auto kNumResources = 3999;
+  const auto kNumTries = 5;
+
+  test::ResourceTableBuilder builder1;
+  FillTable(builder1, 0, kNumResources);
+  std::unique_ptr<ResourceTable> table1 = builder1.Build();
+  std::map<std::string, std::string> expected_mapping;
+  ASSERT_TRUE(ResourcePathShortener(expected_mapping).Consume(context.get(), table1.get()));
+
+  // We are trying to ensure lack of non-determinism, it is not simple to prove
+  // a negative, thus we must try the test a few times so that the test itself
+  // is non-flaky. Basically create the pathmap 5 times from the same set of
+  // resources but a different order of addition and then ensure they are always
+  // mapped to the same short path.
+  for (int i=0; i<kNumTries; i++) {
+    test::ResourceTableBuilder builder2;
+    // This loop adds resources to the resource table in the range of
+    // [0:kNumResources).  Adding the file references in different order makes
+    // non-determinism more likely to surface. Thus we add resources
+    // [start_index:kNumResources) first then [0:start_index). We also use a
+    // different start_index each run.
+    int start_index = (kNumResources/kNumTries)*i;
+    FillTable(builder2, start_index, kNumResources);
+    FillTable(builder2, 0, start_index);
+    std::unique_ptr<ResourceTable> table2 = builder2.Build();
+
+    std::map<std::string, std::string> actual_mapping;
+    ASSERT_TRUE(ResourcePathShortener(actual_mapping).Consume(context.get(), table2.get()));
+
+    for (auto& item : actual_mapping) {
+      ASSERT_THAT(expected_mapping[item.first], Eq(item.second));
+    }
+  }
+}
+
 }   // namespace aapt