Bytecode rewriter validating direct class path

This also suppresses issues brought up by the checks that aren't easily
fixable.
Timing without change on chrome_java:
real	0m3.142s
user	0m7.278s
sys	0m0.563s

Timing with change on chrome_java:
real	0m3.429s
user	0m8.035s
sys	0m0.687s


TBR=trivial third_party changes

Bug: 898251, 874854
Change-Id: Ifcb10f260e80a542ab26a470bb53426e1404e9f0
Reviewed-on: https://chromium-review.googlesource.com/c/1297233
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603164}

build/android/bytecode/BUILD.gn

@@ -11,6 +11,7 @@ java_binary("java_bytecode_rewriter") {
   java_files = [
     "java/org/chromium/bytecode/AssertionEnablerClassAdapter.java",
     "java/org/chromium/bytecode/ByteCodeProcessor.java",
+    "java/org/chromium/bytecode/ClassPathValidator.java",
     "java/org/chromium/bytecode/CustomClassLoaderClassWriter.java",
     "java/org/chromium/bytecode/CustomResourcesClassAdapter.java",
     "java/org/chromium/bytecode/TypeUtils.java",

build/android/bytecode/java/org/chromium/bytecode/ByteCodeProcessor.java

@@ -27,6 +27,10 @@ import java.nio.file.Paths;
 import java.nio.file.StandardCopyOption;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import java.util.zip.CRC32;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
@@ -59,9 +63,12 @@ class ByteCodeProcessor {
         zipStream.closeEntry();
     }
 
-    private static void process(String inputJarPath, String outputJarPath, boolean shouldAssert,
-            boolean shouldUseCustomResources, boolean shouldUseThreadAnnotations,
-            ClassLoader classPathJarsClassLoader) {
+    private static void process(String inputJarPath, String outputJarPath, boolean verbose,
+            boolean isPrebuilt, boolean shouldAssert, boolean shouldUseCustomResources,
+            boolean shouldUseThreadAnnotations, boolean shouldCheckClassPath,
+            ClassLoader directClassPathClassLoader, ClassLoader fullClassPathClassLoader,
+            Set<String> jarsOnlyInFullClassPath) throws ClassPathValidator.ClassNotLoadedException {
+        ClassPathValidator validator = new ClassPathValidator();
         String tempJarPath = outputJarPath + TEMPORARY_FILE_SUFFIX;
         try (ZipInputStream inputStream = new ZipInputStream(
                      new BufferedInputStream(new FileInputStream(inputJarPath)));
@@ -80,13 +87,18 @@ class ByteCodeProcessor {
 
                 ClassReader reader = new ClassReader(readAllBytes(inputStream));
 
+                if (shouldCheckClassPath) {
+                    validator.validateClassPathsAndOutput(reader, directClassPathClassLoader,
+                            fullClassPathClassLoader, jarsOnlyInFullClassPath, isPrebuilt, verbose);
+                }
+
                 ClassWriter writer;
                 if (shouldUseCustomResources) {
                     // Use the COMPUTE_FRAMES flag to have asm figure out the stack map frames.
                     // This is necessary because GCMBaseIntentService in android_gcm_java contains
                     // incorrect stack map frames. This option slows down processing time by 2x.
                     writer = new CustomClassLoaderClassWriter(
-                            classPathJarsClassLoader, reader, COMPUTE_FRAMES);
+                            fullClassPathClassLoader, reader, COMPUTE_FRAMES);
                 } else {
                     writer = new ClassWriter(reader, 0);
                 }
@@ -109,7 +121,7 @@ class ByteCodeProcessor {
                 }
                 if (shouldUseCustomResources) {
                     chain = new CustomResourcesClassAdapter(chain, reader.getClassName(),
-                            reader.getSuperName(), classPathJarsClassLoader);
+                            reader.getSuperName(), fullClassPathClassLoader);
                 }
                 reader.accept(chain, 0);
                 byte[] patchedByteCode = writer.toByteArray();
@@ -125,6 +137,15 @@ class ByteCodeProcessor {
         } catch (IOException ioException) {
             throw new RuntimeException(ioException);
         }
+
+        if (validator.getNumClassPathErrors() > 0) {
+            System.err.println("Missing " + validator.getNumClassPathErrors()
+                    + " classes missing in direct classpath. To fix, add GN deps for:");
+            for (String s : validator.getClassPathMissingJars()) {
+                System.err.println(s);
+            }
+            System.exit(1);
+        }
     }
 
     private static byte[] readAllBytes(InputStream inputStream) throws IOException {
@@ -141,11 +162,12 @@ class ByteCodeProcessor {
      * Loads a list of jars and returns a ClassLoader capable of loading all classes found in the
      * given jars.
      */
-    private static ClassLoader loadJars(ArrayList<String> paths) {
+    static ClassLoader loadJars(Collection<String> paths) {
         URL[] jarUrls = new URL[paths.size()];
-        for (int i = 0; i < paths.size(); ++i) {
+        int i = 0;
+        for (String path : paths) {
             try {
-                jarUrls[i] = new File(paths.get(i)).toURI().toURL();
+                jarUrls[i++] = new File(path).toURI().toURL();
             } catch (MalformedURLException e) {
                 throw new RuntimeException(e);
             }
@@ -153,23 +175,44 @@ class ByteCodeProcessor {
         return new URLClassLoader(jarUrls);
     }
 
-    public static void main(String[] args) {
+    public static void main(String[] args) throws ClassPathValidator.ClassNotLoadedException {
         // Invoke this script using //build/android/gyp/bytecode_processor.py
-        String inputJarPath = args[0];
-        String outputJarPath = args[1];
-        boolean shouldAssert = args[2].equals("--enable-assert");
-        boolean shouldUseCustomResources = args[3].equals("--enable-custom-resources");
-        boolean shouldUseThreadAnnotations = args[4].equals("--enable-thread-annotations");
+        int currIndex = 0;
+        String inputJarPath = args[currIndex++];
+        String outputJarPath = args[currIndex++];
+        boolean verbose = args[currIndex++].equals("--verbose");
+        boolean isPrebuilt = args[currIndex++].equals("--is-prebuilt");
+        boolean shouldAssert = args[currIndex++].equals("--enable-assert");
+        boolean shouldUseCustomResources = args[currIndex++].equals("--enable-custom-resources");
+        boolean shouldUseThreadAnnotations =
+                args[currIndex++].equals("--enable-thread-annotations");
+        boolean shouldCheckClassPath = args[currIndex++].equals("--enable-check-class-path");
+        int sdkJarsLength = Integer.parseInt(args[currIndex++]);
+        List<String> sdkJarPaths =
+                Arrays.asList(Arrays.copyOfRange(args, currIndex, currIndex + sdkJarsLength));
+        currIndex += sdkJarsLength;
+
+        int directJarsLength = Integer.parseInt(args[currIndex++]);
+        ArrayList<String> directClassPathJarPaths = new ArrayList<>();
+        directClassPathJarPaths.add(inputJarPath);
+        directClassPathJarPaths.addAll(sdkJarPaths);
+        directClassPathJarPaths.addAll(
+                Arrays.asList(Arrays.copyOfRange(args, currIndex, currIndex + directJarsLength)));
+        currIndex += directJarsLength;
+        ClassLoader directClassPathClassLoader = loadJars(directClassPathJarPaths);
 
         // Load all jars that are on the classpath for the input jar for analyzing class hierarchy.
-        ClassLoader classPathJarsClassLoader = null;
-        if (shouldUseCustomResources) {
-            ArrayList<String> classPathJarsPaths = new ArrayList<>();
-            classPathJarsPaths.add(inputJarPath);
-            classPathJarsPaths.addAll(Arrays.asList(Arrays.copyOfRange(args, 4, args.length)));
-            classPathJarsClassLoader = loadJars(classPathJarsPaths);
-        }
-        process(inputJarPath, outputJarPath, shouldAssert, shouldUseCustomResources,
-                shouldUseThreadAnnotations, classPathJarsClassLoader);
+        Set<String> fullClassPathJarPaths = new HashSet<>();
+        fullClassPathJarPaths.clear();
+        fullClassPathJarPaths.add(inputJarPath);
+        fullClassPathJarPaths.addAll(sdkJarPaths);
+        fullClassPathJarPaths.addAll(
+                Arrays.asList(Arrays.copyOfRange(args, currIndex, args.length)));
+        ClassLoader fullClassPathClassLoader = loadJars(fullClassPathJarPaths);
+        fullClassPathJarPaths.removeAll(directClassPathJarPaths);
+
+        process(inputJarPath, outputJarPath, verbose, isPrebuilt, shouldAssert,
+                shouldUseCustomResources, shouldUseThreadAnnotations, shouldCheckClassPath,
+                directClassPathClassLoader, fullClassPathClassLoader, fullClassPathJarPaths);
     }
 }

build/android/bytecode/java/org/chromium/bytecode/ClassPathValidator.java

+// Copyright 2018 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.bytecode;
+
+import org.objectweb.asm.ClassReader;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Checks classpaths (given as ClassLoaders) by reading the constant pool of the class file and
+ * attempting to load every referenced class. If there are some that are unable to be found, it
+ * stores a helpful error message if it knows where it might find them, and exits the program if it
+ * can't find the class with any given classpath.
+ */
+public class ClassPathValidator {
+    private final Set<String> mClassPathMissingJars = new HashSet<>();
+    private int mNumClassPathErrors;
+
+    static class ClassNotLoadedException extends ClassNotFoundException {
+        private final String mClassName;
+
+        ClassNotLoadedException(String className, Throwable ex) {
+            super("Couldn't load " + className, ex);
+            mClassName = className;
+        }
+
+        public String getClassName() {
+            return mClassName;
+        }
+    }
+
+    private static void printAndQuit(ClassNotLoadedException e, ClassReader classReader,
+            boolean verbose) throws ClassNotLoadedException {
+        System.err.println("Class \"" + e.getClassName()
+                + "\" not found on any classpath. Used by class \"" + classReader.getClassName()
+                + "\"");
+        if (verbose) {
+            throw e;
+        }
+        System.exit(1);
+    }
+
+    private static void validateClass(ClassLoader classLoader, String className)
+            throws ClassNotLoadedException {
+        if (className.startsWith("[")) {
+            // Dealing with an array type which isn't encoded nicely in the constant pool.
+            // For example, [[Lorg/chromium/Class$1;
+            className = className.substring(className.lastIndexOf('[') + 1);
+            if (className.charAt(0) == 'L' && className.endsWith(";")) {
+                className = className.substring(1, className.length() - 1);
+            } else {
+                // Bailing out if we have an non-class array type.
+                // This could be something like [B
+                return;
+            }
+        }
+        if (className.matches(".*\\bR(\\$\\w+)?$")) {
+            // Resources in R.java files are not expected to be valid at this stage in the build.
+            return;
+        }
+        if (className.matches("^libcore\\b.*")) {
+            // libcore exists on devices, but is not included in the Android sdk as it is a private
+            // API.
+            return;
+        }
+        try {
+            classLoader.loadClass(className.replace('/', '.'));
+        } catch (ClassNotFoundException e) {
+            throw new ClassNotLoadedException(className, e);
+        } catch (NoClassDefFoundError e) {
+            // We assume that this is caused by another class that is not going to able to be
+            // loaded, so we will skip this and let that class fail with ClassNotFoundException.
+        }
+    }
+
+    /**
+     * Given a .class file, see if every class referenced in the main class' constant pool can be
+     * loaded by the given ClassLoader.
+     *
+     * @param classReader .class file interface for reading the constant pool.
+     * @param classLoader classpath you wish to validate.
+     * @throws ClassNotLoadedException thrown if it can't load a certain class.
+     */
+    private static void validateClassPath(ClassReader classReader, ClassLoader classLoader)
+            throws ClassNotLoadedException {
+        char[] charBuffer = new char[classReader.getMaxStringLength()];
+        // According to the Java spec, the constant pool is indexed from 1 to constant_pool_count -
+        // 1. See https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4
+        for (int i = 1; i < classReader.getItemCount(); i++) {
+            int offset = classReader.getItem(i);
+            // Class entries correspond to 7 in the constant pool
+            // https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4
+            if (offset > 0 && classReader.readByte(offset - 1) == 7) {
+                validateClass(classLoader, classReader.readUTF8(offset, charBuffer));
+            }
+        }
+    }
+
+    public void validateClassPathsAndOutput(ClassReader classReader,
+            ClassLoader directClassPathClassLoader, ClassLoader fullClassPathClassLoader,
+            Collection<String> jarsOnlyInFullClassPath, boolean isPrebuilt, boolean verbose)
+            throws ClassNotLoadedException {
+        if (isPrebuilt) {
+            // Prebuilts only need transitive dependencies checked, not direct dependencies.
+            try {
+                validateClassPath(classReader, fullClassPathClassLoader);
+            } catch (ClassNotLoadedException e) {
+                printAndQuit(e, classReader, verbose);
+            }
+        } else {
+            try {
+                validateClassPath(classReader, directClassPathClassLoader);
+            } catch (ClassNotLoadedException e) {
+                try {
+                    validateClass(fullClassPathClassLoader, e.getClassName());
+                } catch (ClassNotLoadedException d) {
+                    printAndQuit(d, classReader, verbose);
+                }
+                if (verbose) {
+                    System.err.println("Class \"" + e.getClassName()
+                            + "\" not found in direct dependencies,"
+                            + " but found in indirect dependiences.");
+                }
+                // Iterating through all jars that are in the full classpath but not the direct
+                // classpath to find which one provides the class we are looking for.
+                for (String s : jarsOnlyInFullClassPath) {
+                    try {
+                        ClassLoader smallLoader =
+                                ByteCodeProcessor.loadJars(Collections.singletonList(s));
+                        validateClass(smallLoader, e.getClassName());
+                        mClassPathMissingJars.add(s);
+                        mNumClassPathErrors++;
+                        break;
+                    } catch (ClassNotLoadedException f) {
+                    }
+                }
+            }
+        }
+    }
+
+    public int getNumClassPathErrors() {
+        return mNumClassPathErrors;
+    }
+
+    public Set<String> getClassPathMissingJars() {
+        return mClassPathMissingJars;
+    }
+}

build/android/gyp/bytecode_processor.py

@@ -25,20 +25,40 @@ def main(argv):
                       help='Path to the java binary wrapper script.')
   parser.add_argument('--input-jar', required=True)
   parser.add_argument('--output-jar', required=True)
-  parser.add_argument('--extra-classpath-jar', dest='extra_jars',
+  parser.add_argument('--direct-classpath-jars', required=True)
+  parser.add_argument('--sdk-classpath-jars', required=True)
+  parser.add_argument('--extra-classpath-jars', dest='extra_jars',
                       action='append', default=[],
                       help='Extra inputs, passed last to the binary script.')
+  parser.add_argument('-v', '--verbose', action='store_true')
+  _AddSwitch(parser, '--is-prebuilt')
   _AddSwitch(parser, '--enable-custom-resources')
   _AddSwitch(parser, '--enable-assert')
   _AddSwitch(parser, '--enable-thread-annotations')
+  _AddSwitch(parser, '--enable-check-class-path')
   args = parser.parse_args(argv)
+
+  sdk_jars = build_utils.ParseGnList(args.sdk_classpath_jars)
+  assert len(sdk_jars) > 0
+
+  direct_jars = build_utils.ParseGnList(args.direct_classpath_jars)
+  assert len(direct_jars) > 0
+
   extra_classpath_jars = []
   for a in args.extra_jars:
     extra_classpath_jars.extend(build_utils.ParseGnList(a))
 
-  cmd = [args.script, args.input_jar, args.output_jar, args.enable_assert,
-         args.enable_custom_resources,
-         args.enable_thread_annotations] + extra_classpath_jars
+  if args.verbose:
+    verbose = '--verbose'
+  else:
+    verbose = '--not-verbose'
+
+  cmd = ([
+      args.script, args.input_jar, args.output_jar, verbose, args.is_prebuilt,
+      args.enable_assert, args.enable_custom_resources,
+      args.enable_thread_annotations, args.enable_check_class_path,
+      str(len(sdk_jars))
+  ] + sdk_jars + [str(len(direct_jars))] + direct_jars + extra_classpath_jars)
   subprocess.check_call(cmd)
 
 

build/config/android/internal_rules.gni

@@ -1410,6 +1410,28 @@ if (enable_java_templates) {
 
     _desugar = defined(invoker.supports_android) && invoker.supports_android
     _emma_instrument = invoker.emma_instrument
+    _enable_bytecode_rewriter =
+        _enable_assert || _enable_custom_resources || _enable_thread_annotations
+    _is_prebuilt = defined(invoker.is_prebuilt) && invoker.is_prebuilt
+    _enable_bytecode_checks = defined(invoker.enable_bytecode_checks) &&
+                              invoker.enable_bytecode_checks
+
+    # Release builds don't have asserts enabled, so they often will not run the
+    # bytecode rewriter. We are okay with having release builds not run the
+    # bytecode checks at all, since the dependency errors can be caught in debug
+    # mode.
+    not_needed([
+                 "_is_prebuilt",
+                 "_enable_bytecode_checks",
+               ])
+    if (defined(invoker.enable_bytecode_rewriter)) {
+      not_needed([
+                   "_enable_assert",
+                   "_enable_custom_resources",
+                   "_enable_thread_annotations",
+                 ])
+      _enable_bytecode_rewriter = invoker.enable_bytecode_rewriter
+    }
 
     _jar_excluded_patterns = []
     if (defined(invoker.jar_excluded_patterns)) {
@@ -1427,8 +1449,7 @@ if (enable_java_templates) {
     _deps = []
     _previous_output_jar = _input_jar_path
 
-    if (_enable_assert || _enable_custom_resources ||
-        _enable_thread_annotations) {
+    if (_enable_bytecode_rewriter) {
       _java_bytecode_rewriter_target = "${target_name}__bytecode_rewrite"
       _java_bytecode_rewriter_input_jar = _previous_output_jar
       _java_bytecode_rewriter_output_jar =
@@ -1458,6 +1479,9 @@ if (enable_java_templates) {
           "--output-jar",
           rebase_path(_java_bytecode_rewriter_output_jar, root_build_dir),
         ]
+        if (_is_prebuilt) {
+          args += [ "--is-prebuilt" ]
+        }
         if (_enable_assert) {
           args += [ "--enable-assert" ]
         }
@@ -1467,10 +1491,15 @@ if (enable_java_templates) {
         if (_enable_thread_annotations) {
           args += [ "--enable-thread-annotations" ]
         }
+        if (_enable_bytecode_checks) {
+          args += [ "--enable-check-class-path" ]
+        }
         args += [
-          "--extra-classpath-jar",
+          "--direct-classpath-jars",
+          "@FileArg($_rebased_build_config:javac:classpath)",
+          "--sdk-classpath-jars",
           "@FileArg($_rebased_build_config:android:sdk_jars)",
-          "--extra-classpath-jar",
+          "--extra-classpath-jars",
           "@FileArg($_rebased_build_config:deps_info:javac_full_classpath)",
         ]
       }
@@ -3252,9 +3281,12 @@ if (enable_java_templates) {
         process_java_prebuilt(_process_prebuilt_target_name) {
           forward_variables_from(invoker,
                                  [
+                                   "enable_bytecode_checks",
+                                   "enable_bytecode_rewriter",
                                    "jar_excluded_patterns",
                                    "jar_included_patterns",
                                  ])
+          is_prebuilt = _is_prebuilt
           supports_android = _supports_android
           enable_build_hooks = _enable_build_hooks
           enable_build_hooks_android = _enable_build_hooks_android

third_party/accessibility_test_framework/BUILD.gn

@@ -5,6 +5,8 @@
 import("//build/config/android/rules.gni")
 
 android_java_prebuilt("accessibility_test_framework_java") {
+  # Uses wrong version of proto (not protolite).
+  enable_bytecode_checks = false
   testonly = true
   jar_path = "lib/accessibility-test-framework.jar"
   deps = [

third_party/espresso/BUILD.gn

@@ -60,6 +60,8 @@ android_java_prebuilt("espresso_intents_java") {
 }
 
 android_java_prebuilt("espresso_web_java") {
+  # Uses org/ccil/cowan/tagsoup/Parser which isn't in third_party.
+  enable_bytecode_checks = false
   testonly = true
   jar_path = "lib/espresso-web-release-no-dep.jar"
   deps = [

third_party/google-truth/BUILD.gn

@@ -5,6 +5,8 @@
 import("//build/config/android/rules.gni")
 
 java_prebuilt("google_truth_java") {
+  # Uses the difflib package, which doesn't exist in third_party.
+  enable_bytecode_checks = false
   testonly = true
   supports_android = true
 

third_party/netty4/BUILD.gn

@@ -8,6 +8,8 @@ import("//build/config/android/rules.gni")
 import("netty4.gni")
 
 android_java_prebuilt("netty_all_java") {
+  # Uses fasterxml/aalto which doesn't exist in third_party.
+  enable_bytecode_checks = false
   testonly = true
   jar_path = NETTY4_JAR_FILE
 }

tools/android/errorprone_plugin/BUILD.gn

@@ -16,6 +16,7 @@ java_library("errorprone_plugin_java") {
 
   # Necessary to avoid dependency cycle
   enable_errorprone = false
+  enable_bytecode_rewriter = false
 
   # So that we don't need to inject emma runtime into the compiler's classpath.
   emma_never_instrument = true