Use -identifiernamestring for split compat implementation classes

This change adds a @IdentifierNameString annotation to use the
-identifiernamestring R8 rule. There are a few limitations to this:
  - -identifiernamestring only obfuscates the string, it does not
    actually check that the class exists.
  - If a field has the annotation, it must be non-final, otherwise javac
    will inline the constant and R8 won't obfuscate it.
  - Any field/method must be assigned/called with a String literal,
    otherwise R8 won't obfuscate the string and there will be issues at
    runtime.

Due to these limitations, I ended up adding a getIdentifierName() method
to SplitCompatUtils which returns the obfuscated name of implementation
classes for split compat. I also added a PRESUBMIT check to enforce that
only string literals are passed to that method. There can be cases where
R8 will not fail, but a string variable is silently ignored if it is not
a String literal.

Bug: 1126301
Change-Id: Ie5216e6a81cdbca71cc117460704ea4d9122fc78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2451388
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814212}

base/BUILD.gn

@@ -3603,6 +3603,7 @@ if (is_android) {
       "android/java/src/org/chromium/base/annotations/CalledByNativeUnchecked.java",
       "android/java/src/org/chromium/base/annotations/CheckDiscard.java",
       "android/java/src/org/chromium/base/annotations/DoNotInline.java",
+      "android/java/src/org/chromium/base/annotations/IdentifierNameString.java",
       "android/java/src/org/chromium/base/annotations/JNIAdditionalImport.java",
       "android/java/src/org/chromium/base/annotations/JNINamespace.java",
       "android/java/src/org/chromium/base/annotations/JniIgnoreNatives.java",

base/android/java/src/org/chromium/base/annotations/IdentifierNameString.java

@@ -0,0 +1,35 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base.annotations;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Annotation used to mark field that may contain Strings referring to fully qualified class names
+ * and methods whose arguments may be fully qualified class names. These classes may then be
+ * obfuscated by R8. A couple caveats when using this:
+ * - This only obfuscates the string, it does not actually check that the class exists.
+ * - If a field has this annotation, it must be non-final, otherwise javac will inline the constant
+ *   and R8 won't obfuscate it.
+ * - Any field/method must be assigned/called with a String literal or a variable R8 can easily
+ *   trace to a String literal.
+ *
+ * <p>Usage example:<br>
+ *   {@code
+ *   @IdentifierNameString
+ *   public static final String LOGGING_TAG = "com.google.android.apps.foo.FooActivity";
+ *
+ *   // In this example, both className and message are treated as identifier name strings, but will
+ *   // only be obfuscated if the string points to a real class.
+ *   @IdentifierNameString
+ *   public void doSomeLogging(String className, String message) { ... }
+ *   }
+ */
+@Target({ElementType.FIELD, ElementType.METHOD})
+@Retention(RetentionPolicy.CLASS)
+public @interface IdentifierNameString {}

base/android/proguard/chromium_code.flags

@@ -18,6 +18,7 @@
 -keep @interface org.chromium.base.annotations.DoNotInline
 -keep @interface org.chromium.base.annotations.RemovableInRelease
 -keep @interface org.chromium.base.annotations.UsedByReflection
+-keep @interface org.chromium.base.annotations.IdentifierNameString
 
 # Android support library annotations will get converted to androidx ones
 # which we want to keep.
@@ -97,3 +98,8 @@
 -keepclassmembers enum org.chromium.** {
     public static **[] values();
 }
+
+# Mark members annotated with IdentifierNameString as identifier name strings
+-identifiernamestring class * {
+    @org.chromium.base.annotations.IdentifierNameString *;
+}

chrome/android/expectations/monochrome_public_bundle.proguard_flags.expected

@@ -118,6 +118,7 @@
 -keep @interface org.chromium.base.annotations.DoNotInline
 -keep @interface org.chromium.base.annotations.RemovableInRelease
 -keep @interface org.chromium.base.annotations.UsedByReflection
+-keep @interface org.chromium.base.annotations.IdentifierNameString
 
 # Android support library annotations will get converted to androidx ones
 # which we want to keep.
@@ -198,6 +199,11 @@
     public static **[] values();
 }
 
+# Mark members annotated with IdentifierNameString as identifier name strings
+-identifiernamestring class * {
+    @org.chromium.base.annotations.IdentifierNameString *;
+}
+
 # File: ../../build/android/dcheck_is_off.flags
 # Copyright 2019 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
@@ -287,6 +293,12 @@
   *** build() return null;
 }
 
+# Keep implementation classes needed for split compat. These will be accessed by
+# reflection.
+-keep,allowobfuscation public class ** extends org.chromium.chrome.browser.base.SplitCompat*$Impl {
+  public <init>();
+}
+
 # File: ../../third_party/gvr-android-sdk/proguard-gvr-chromium.txt
 -dontwarn com.google.common.logging.nano.Vr$**
 -dontwarn com.google.vr.**

chrome/android/java/src/PRESUBMIT.py

@@ -28,6 +28,9 @@
 NEW_ALERTDIALOG_BUILDER_RE = re.compile(
     r'\bnew\sAlertDialog\.Builder\b')
 
+SPLIT_COMPAT_UTILS_IMPL_NAME_RE = re.compile(
+    r'\bSplitCompatUtils\.getIdentifierName\(\s*[^\s"]')
+
 COMMENT_RE = re.compile(r'^\s*(//|/\*|\*)')
 
 BROWSER_ROOT = 'chrome/android/java/src/org/chromium/chrome/browser/'
@@ -47,6 +50,7 @@ def _CommonChecks(input_api, output_api):
   result.extend(_CheckNotificationConstructors(input_api, output_api))
   result.extend(_CheckAlertDialogBuilder(input_api, output_api))
   result.extend(_CheckCompatibleAlertDialogBuilder(input_api, output_api))
+  result.extend(_CheckSplitCompatUtilsIdentifierName(input_api, output_api))
   # Add more checks here
   return result
 
@@ -115,8 +119,9 @@ def _CheckAlertDialogBuilder(input_api, output_api):
   //src/chrome/android/java/src/org/chromium/chrome/browser/vr/VR_JAVA_OWNERS
   '''
   error_files = []
-  result = _CheckReIgnoreComment(input_api, output_api, error_msg, files_to_skip,
-                                 NEW_ALERTDIALOG_BUILDER_RE, error_files)
+  result = _CheckReIgnoreComment(input_api, output_api, error_msg,
+                                 files_to_skip, NEW_ALERTDIALOG_BUILDER_RE,
+                                 error_files)
 
   wrong_builder_errors = []
   wrong_builder_error_msg = '''
@@ -172,6 +177,16 @@ def _CheckCompatibleAlertDialogBuilder(input_api, output_api):
                                NEW_COMPATIBLE_ALERTDIALOG_BUILDER_RE)
 
 
+def _CheckSplitCompatUtilsIdentifierName(input_api, output_api):
+  error_msg = '''
+  SplitCompatUtils.getIdentifierName() not check failed:
+  SplitCompatUtils.getIdentifierName() must be called with a String literal,
+  otherwise R8 may not correctly obfuscate the class name passed in.
+  '''
+  return _CheckReIgnoreComment(input_api, output_api, error_msg, [],
+                               SPLIT_COMPAT_UTILS_IMPL_NAME_RE)
+
+
 def _CheckReIgnoreComment(input_api, output_api, error_msg, files_to_skip,
                           regular_expression, error_files=None):
 
@@ -192,7 +207,7 @@ def CheckLine(current_file, line_number, line, problems, error_files):
   for f in input_api.AffectedFiles(include_deletes=False,
                                    file_filter=sources):
     previous_line = ''
-    for line_number, line in f.ChangedContents():
+    for line_number, line in enumerate(f.NewContents(), start=1):
       if not CheckLine(f, line_number, line, problems, error_files):
         if previous_line:
           two_lines = '\n'.join([previous_line, line])

chrome/android/java/src/PRESUBMIT_test.py

@@ -107,7 +107,7 @@ def testFailure_WrongBuilderCheck(self):
     self.assertEqual(1, len(errors[1].items))
     self.assertIn('One.java', errors[1].items[0])
 
-  def testSucess_WrongBuilderCheck(self):
+  def testSuccess_WrongBuilderCheck(self):
     """Use of OS-dependent AlertDialog should not be flagged."""
     mock_input = MockInputApi()
     mock_input.files = [
@@ -153,7 +153,7 @@ def testFailure(self):
     self.assertIn('Three.java', errors[0].items[2])
     self.assertIn('Four.java', errors[0].items[3])
 
-  def testSucess(self):
+  def testSuccess(self):
     """Examples of when AlertDialog.Builder should not be flagged."""
     mock_input = MockInputApi()
     mock_input.files = [
@@ -176,6 +176,56 @@ def testSucess(self):
         mock_input, MockOutputApi())
     self.assertEqual(0, len(errors))
 
+class CheckSplitCompatUtilsIdentifierName(unittest.TestCase):
+  """Test the _CheckSplitCompatUtilsIdentifierName presubmit check."""
+
+  def testFailure(self):
+    """
+    SplitCompatUtils.getIdentifierName() without a String literal is flagged.
+    """
+    mock_input = MockInputApi()
+    mock_input.files = [
+        MockFile('path/One.java',
+                 [
+                  'SplitCompatUtils.getIdentifierName(foo)',
+                  'A new line to make sure there is no duplicate error.']),
+        MockFile('path/Two.java',
+                 ['SplitCompatUtils.getIdentifierName(    foo)']),
+        MockFile('path/Three.java',
+                 ['SplitCompatUtils.getIdentifierName(',
+                  '     bar)']),
+    ]
+    errors = PRESUBMIT._CheckSplitCompatUtilsIdentifierName(
+        mock_input, MockOutputApi())
+    self.assertEqual(1, len(errors))
+    self.assertEqual(3, len(errors[0].items))
+    self.assertIn('One.java', errors[0].items[0])
+    self.assertIn('Two.java', errors[0].items[1])
+    self.assertIn('Three.java', errors[0].items[2])
+
+  def testSuccess(self):
+    """
+    Examples of when SplitCompatUtils.getIdentifierName() should not be flagged.
+    """
+    mock_input = MockInputApi()
+    mock_input.files = [
+        MockFile('path/One.java',
+                 [
+                  'SplitCompatUtils.getIdentifierName("foo")',
+                  'A new line.']),
+        MockFile('path/Two.java',
+                 ['SplitCompatUtils.getIdentifierName(    "foo")']),
+        MockFile('path/Three.java',
+                 ['SplitCompatUtils.getIdentifierName(',
+                  '    "bar")']),
+        MockFile('path/Four.java',
+                 ['  super(SplitCompatUtils.getIdentifierName(',
+                  '"bar"))']),
+    ]
+    errors = PRESUBMIT._CheckSplitCompatUtilsIdentifierName(
+        mock_input, MockOutputApi())
+    self.assertEqual(0, len(errors))
+
 
 if __name__ == '__main__':
   unittest.main()

chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

@@ -20,7 +20,6 @@
 import org.chromium.base.PathUtils;
 import org.chromium.base.TraceEvent;
 import org.chromium.base.annotations.MainDex;
-import org.chromium.base.annotations.UsedByReflection;
 import org.chromium.base.memory.MemoryPressureMonitor;
 import org.chromium.chrome.browser.background_task_scheduler.ChromeBackgroundTaskFactory;
 import org.chromium.chrome.browser.base.SplitCompatApplication;
@@ -64,9 +63,7 @@ public class ChromeApplication extends SplitCompatApplication {
     private static volatile ChromeAppComponent sComponent;
 
     /** Chrome application logic. */
-    @UsedByReflection("SplitChromeApplication.java")
     public static class ChromeApplicationImpl extends Impl {
-        @UsedByReflection("SplitChromeApplication.java")
         public ChromeApplicationImpl() {}
 
         // Called by the framework for ALL processes. Runs before ContentProviders are created.

chrome/android/java/src/org/chromium/chrome/browser/MonochromeApplication.java

@@ -9,7 +9,6 @@
 import org.chromium.android_webview.nonembedded.WebViewApkApplication;
 import org.chromium.base.ActivityState;
 import org.chromium.base.ApplicationStatus;
-import org.chromium.base.annotations.UsedByReflection;
 import org.chromium.base.library_loader.LibraryProcessType;
 import org.chromium.chrome.browser.base.SplitMonochromeApplication;
 import org.chromium.chrome.browser.version.ChromeVersionInfo;
@@ -33,9 +32,7 @@ public MonochromeApplication() {
     }
 
     /** Monochrome application logic. */
-    @UsedByReflection("SplitMonochromeApplication.java")
     public static class MonochromeApplicationImpl extends ChromeApplication.ChromeApplicationImpl {
-        @UsedByReflection("SplitMonochromeApplication.java")
         public MonochromeApplicationImpl() {}
 
         @Override

chrome/android/java/src/org/chromium/chrome/browser/base/SplitChromeApplication.java

@@ -16,7 +16,8 @@ public class SplitChromeApplication extends SplitCompatApplication {
     private String mChromeApplicationClassName;
 
     public SplitChromeApplication() {
-        this("org.chromium.chrome.browser.ChromeApplication$ChromeApplicationImpl");
+        this(SplitCompatUtils.getIdentifierName(
+                "org.chromium.chrome.browser.ChromeApplication$ChromeApplicationImpl"));
     }
 
     public SplitChromeApplication(String chromeApplicationClassName) {

chrome/android/java/src/org/chromium/chrome/browser/base/SplitCompatUtils.java

@@ -8,12 +8,22 @@
 import android.content.pm.PackageManager;
 import android.os.Build;
 
+import org.chromium.base.annotations.IdentifierNameString;
 import org.chromium.base.compat.ApiHelperForO;
 
 /** Utils for compatibility with isolated splits. */
 public class SplitCompatUtils {
     private SplitCompatUtils() {}
 
+    /**
+     * Gets the obfuscated name for the passed in class name. Important: this MUST be called with a
+     * string literal, otherwise @IdentifierNameString will not work.
+     */
+    @IdentifierNameString
+    public static String getIdentifierName(String className) {
+        return className;
+    }
+
     /** Creates a context which can be used to load code and resources in the chrome split. */
     public static Context createChromeContext(Context base) {
         // Isolated splits are only supported in O+, so just return the base context on other

chrome/android/java/src/org/chromium/chrome/browser/base/SplitMonochromeApplication.java

@@ -41,7 +41,8 @@ public boolean isWebViewProcess() {
     }
 
     public SplitMonochromeApplication() {
-        super("org.chromium.chrome.browser.MonochromeApplication$MonochromeApplicationImpl");
+        super(SplitCompatUtils.getIdentifierName(
+                "org.chromium.chrome.browser.MonochromeApplication$MonochromeApplicationImpl"));
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadForegroundService.java

@@ -5,12 +5,12 @@
 package org.chromium.chrome.browser.download;
 
 import org.chromium.chrome.browser.base.SplitCompatService;
+import org.chromium.chrome.browser.base.SplitCompatUtils;
 
 /** See {@link DownloadForegroundServiceImpl}. */
 public class DownloadForegroundService extends SplitCompatService {
-    // TODO(crbug.com/1126301):  Use R8's -identifiernamestring to verify this and other
-    // SplitCompatService subclasses.
     public DownloadForegroundService() {
-        super("org.chromium.chrome.browser.download.DownloadForegroundServiceImpl");
+        super(SplitCompatUtils.getIdentifierName(
+                "org.chromium.chrome.browser.download.DownloadForegroundServiceImpl"));
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadForegroundServiceImpl.java

@@ -22,7 +22,6 @@
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
-import org.chromium.base.annotations.UsedByReflection;
 import org.chromium.components.browser_ui.notifications.ForegroundServiceUtils;
 
 import java.lang.annotation.Retention;
@@ -31,7 +30,6 @@
 /**
  * Keep-alive foreground service for downloads.
  */
-@UsedByReflection("DownloadForegroundService.java")
 public class DownloadForegroundServiceImpl extends DownloadForegroundService.Impl {
     private static final String TAG = "DownloadFg";
     private final IBinder mBinder = new LocalBinder();
@@ -45,9 +43,6 @@ public class DownloadForegroundServiceImpl extends DownloadForegroundService.Imp
         int DETACH = 1; // Try to detach, otherwise kill and relaunch.
     }
 
-    @UsedByReflection("DownloadForegroundService.java")
-    public DownloadForegroundServiceImpl() {}
-
     @Override
     public void onCreate() {
         super.onCreate();

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobService.java

@@ -5,10 +5,12 @@
 package org.chromium.chrome.browser.notifications;
 
 import org.chromium.chrome.browser.base.SplitCompatJobService;
+import org.chromium.chrome.browser.base.SplitCompatUtils;
 
 /** See {@link NotificationJobServiceImpl}. */
 public class NotificationJobService extends SplitCompatJobService {
     public NotificationJobService() {
-        super("org.chromium.chrome.browser.notifications.NotificationJobServiceImpl");
+        super(SplitCompatUtils.getIdentifierName(
+                "org.chromium.chrome.browser.notifications.NotificationJobServiceImpl"));
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationJobServiceImpl.java

@@ -13,18 +13,13 @@
 import android.os.SystemClock;
 
 import org.chromium.base.ThreadUtils;
-import org.chromium.base.annotations.UsedByReflection;
 
 /**
  * Processes jobs scheduled when user actions are issued on web notifications.
  * We use this instead of starting the NotificationService on N+.
  */
-@UsedByReflection("NotificationService.java")
 @TargetApi(Build.VERSION_CODES.N)
 public class NotificationJobServiceImpl extends NotificationJobService.Impl {
-    @UsedByReflection("NotificationService.java")
-    public NotificationJobServiceImpl() {}
-
     static PersistableBundle getJobExtrasFromIntent(Intent intent) {
         PersistableBundle bundle = new PersistableBundle();
         bundle.putString(NotificationConstants.EXTRA_NOTIFICATION_ID,

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java

@@ -16,7 +16,9 @@ public class NotificationService extends SplitCompatIntentService {
     private static final String TAG = NotificationService.class.getSimpleName();
 
     public NotificationService() {
-        super("org.chromium.chrome.browser.notifications.NotificationServiceImpl", TAG);
+        super(SplitCompatUtils.getIdentifierName(
+                      "org.chromium.chrome.browser.notifications.NotificationServiceImpl"),
+                TAG);
     }
 
     /**