From 3001f0311ddf4a6cc8b328ae697314feb1fd00f7 Mon Sep 17 00:00:00 2001
From: Jake Wharton <jakew@google.com>
Date: Mon, 11 Jun 2018 12:24:11 -0400
Subject: [PATCH] Only keep methods with correct signature for view/menu click

For View's onClick attribute the method must have a single argument of type android.view.View. For a MenuItem's onClick attribute the method must have a single argument of type android.view.MenuItem. Since these rules match all types and any return type, matching by signature is the only available specificity that can be added.

Bug: 37123156
Test: make aapt2_tests
Change-Id: I4b82f5ef9e62a8ecffaab424e269df627825709e
---
 tools/aapt2/java/ProguardRules.cpp      | 13 ++++++++-----
 tools/aapt2/java/ProguardRules.h        | 25 ++++++++++++++++++++++---
 tools/aapt2/java/ProguardRules_test.cpp |  6 ++++--
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/tools/aapt2/java/ProguardRules.cpp b/tools/aapt2/java/ProguardRules.cpp
index d7ebd8c1d33ee..d03cdb3d85181 100644
--- a/tools/aapt2/java/ProguardRules.cpp
+++ b/tools/aapt2/java/ProguardRules.cpp
@@ -79,8 +79,10 @@ class BaseVisitor : public xml::Visitor {
     keep_set_->AddConditionalClass({file_.name, file_.source.WithLine(line_number)}, class_name);
   }
 
-  void AddMethod(size_t line_number, const std::string& method_name) {
-    keep_set_->AddMethod({file_.name, file_.source.WithLine(line_number)}, method_name);
+  void AddMethod(size_t line_number, const std::string& method_name,
+                 const std::string& method_signature) {
+    keep_set_->AddMethod({file_.name, file_.source.WithLine(line_number)},
+        {method_name, method_signature});
   }
 
   void AddReference(size_t line_number, Reference* ref) {
@@ -125,7 +127,7 @@ class LayoutVisitor : public BaseVisitor {
         AddClass(node->line_number, attr.value);
       } else if (attr.namespace_uri == xml::kSchemaAndroid &&
                  attr.name == "onClick") {
-        AddMethod(node->line_number, attr.value);
+        AddMethod(node->line_number, attr.value, "android.view.View");
       }
     }
 
@@ -149,7 +151,7 @@ class MenuVisitor : public BaseVisitor {
               util::IsJavaClassName(attr.value)) {
             AddClass(node->line_number, attr.value);
           } else if (attr.name == "onClick") {
-            AddMethod(node->line_number, attr.value);
+            AddMethod(node->line_number, attr.value, "android.view.MenuItem");
           }
         }
       }
@@ -396,7 +398,8 @@ void WriteKeepSet(const KeepSet& keep_set, OutputStream* out) {
     for (const UsageLocation& location : entry.second) {
       printer.Print("# Referenced at ").Println(location.source.to_string());
     }
-    printer.Print("-keepclassmembers class * { *** ").Print(entry.first).Println("(...); }");
+    printer.Print("-keepclassmembers class * { *** ").Print(entry.first.name)
+        .Print("(").Print(entry.first.signature).Println("); }");
     printer.Println();
   }
 }
diff --git a/tools/aapt2/java/ProguardRules.h b/tools/aapt2/java/ProguardRules.h
index 343272e4362a8..acaceac41237c 100644
--- a/tools/aapt2/java/ProguardRules.h
+++ b/tools/aapt2/java/ProguardRules.h
@@ -40,6 +40,11 @@ struct UsageLocation {
   Source source;
 };
 
+struct NameAndSignature {
+  std::string name;
+  std::string signature;
+};
+
 class KeepSet {
  public:
   KeepSet() = default;
@@ -55,8 +60,8 @@ class KeepSet {
     conditional_class_set_[class_name].insert(file);
   }
 
-  inline void AddMethod(const UsageLocation& file, const std::string& method_name) {
-    method_set_[method_name].insert(file);
+  inline void AddMethod(const UsageLocation& file, const NameAndSignature& name_and_signature) {
+    method_set_[name_and_signature].insert(file);
   }
 
   inline void AddReference(const UsageLocation& file, const ResourceName& resource_name) {
@@ -71,7 +76,7 @@ class KeepSet {
 
   bool conditional_keep_rules_ = false;
   std::map<std::string, std::set<UsageLocation>> manifest_class_set_;
-  std::map<std::string, std::set<UsageLocation>> method_set_;
+  std::map<NameAndSignature, std::set<UsageLocation>> method_set_;
   std::map<std::string, std::set<UsageLocation>> conditional_class_set_;
   std::map<ResourceName, std::set<UsageLocation>> reference_set_;
 };
@@ -100,6 +105,20 @@ inline int operator<(const UsageLocation& lhs, const UsageLocation& rhs) {
   return lhs.name.compare(rhs.name);
 }
 
+//
+// NameAndSignature implementation.
+//
+
+inline bool operator<(const NameAndSignature& lhs, const NameAndSignature& rhs) {
+  if (lhs.name < rhs.name) {
+    return true;
+  }
+  if (lhs.name == rhs.name) {
+    return lhs.signature < rhs.signature;
+  }
+  return false;
+}
+
 }  // namespace proguard
 }  // namespace aapt
 
diff --git a/tools/aapt2/java/ProguardRules_test.cpp b/tools/aapt2/java/ProguardRules_test.cpp
index a548f4a144ec5..b5e27e0cb9529 100644
--- a/tools/aapt2/java/ProguardRules_test.cpp
+++ b/tools/aapt2/java/ProguardRules_test.cpp
@@ -239,7 +239,8 @@ TEST(ProguardRulesTest, ViewOnClickRuleIsEmitted) {
 
   std::string actual = GetKeepSetString(set);
 
-  EXPECT_THAT(actual, HasSubstr("-keepclassmembers class * { *** bar_method(...); }"));
+  EXPECT_THAT(actual, HasSubstr(
+      "-keepclassmembers class * { *** bar_method(android.view.View); }"));
 }
 
 TEST(ProguardRulesTest, MenuRulesAreEmitted) {
@@ -258,7 +259,8 @@ TEST(ProguardRulesTest, MenuRulesAreEmitted) {
 
   std::string actual = GetKeepSetString(set);
 
-  EXPECT_THAT(actual, HasSubstr("-keepclassmembers class * { *** on_click(...); }"));
+  EXPECT_THAT(actual, HasSubstr(
+      "-keepclassmembers class * { *** on_click(android.view.MenuItem); }"));
   EXPECT_THAT(actual, HasSubstr("-keep class com.foo.Bar { <init>(...); }"));
   EXPECT_THAT(actual, HasSubstr("-keep class com.foo.Baz { <init>(...); }"));
   EXPECT_THAT(actual, Not(HasSubstr("com.foo.Bat")));